Re: [PATCH] xsm/flask: adjust print messages to use %pd

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2022 13:34, Daniel P. Smith wrote:
> On 9/9/22 06:04, Jan Beulich wrote:
>> On 09.09.2022 11:50, Daniel P. Smith wrote:
>>> --- a/xen/xsm/flask/avc.c
>>> +++ b/xen/xsm/flask/avc.c
>>> @@ -566,14 +566,14 @@ void avc_audit(u32 ssid, u32 tsid, u16 tclass, u32 
>>> requested,
>>>       if ( a && (a->sdom || a->tdom) )
>>>       {
>>>           if ( a->sdom && a->tdom && a->sdom != a->tdom )
>>> -            avc_printk(&buf, "domid=%d target=%d ", a->sdom->domain_id, 
>>> a->tdom->domain_id);
>>> +            avc_printk(&buf, "source=%pd target=%dp ", a->sdom, a->tdom);
>>>           else if ( a->sdom )
>>> -            avc_printk(&buf, "domid=%d ", a->sdom->domain_id);
>>> +            avc_printk(&buf, "source=%pd ", a->sdom);
>>>           else
>>> -            avc_printk(&buf, "target=%d ", a->tdom->domain_id);
>>> +            avc_printk(&buf, "target=%pd ", a->tdom);
>>
>> Apart from switching to %pd to also replace "domid" by "source". That's
>> fine in the first case (where both domain IDs are logged), but in the
>> second case it's a little questionable. Wouldn't it be better to be
>> able to distinguish the tdom == NULL case from the tdom == sdom one,
>> perhaps by using "source" in the former case but "domid" in the latter
>> one?
> 
> Apologies as I am not quite following your question. Let me provide my 
> reasoning and if it doesn't address your question, then please help me 
> understand your concern.
> 
> The function avc_printk() allows for the incremental build up of an AVC 
> message. In this section, it is attempting to include the applicable 
> source and target that was used to render the AVC. With the switch to 
> %pd, the first and second lines would become "domid=d{id}". I personally 
> find that a bit redundant. Adding to that, in the context of this 
> function there is "sdom" which is source domain, "cdom" which is current 
> domain, and tdom which is target domain. The print statements using cdom 
> or tdom already denoted them with "current=" and "target=" respectively. 
> Whereas, sdom was prefixed with "domid=" in the print statements. To me, 
> it makes more sense to change the prefixes of sdom with "source=" to 
> accurately reflect the context of that domid.

Well, yes, perhaps "domain" would be better than "domid" with the change
to %pd. But I still think the middle of the three printk()s would better
distinguish tdom == NULL from tdom == sdom:

        else if ( a->sdom )
            avc_printk(&buf, "%s=%pd ", a->tdom ? "domain" : "source", a->sdom);

Jan