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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 9/9/22 08:10, Jan Beulich wrote:
> 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);

Okay, I see you are trying to reduce away the last "else", but I have
several concerns about doing this suggestion.

 - The biggest concern is the fact that in the past, a domain referred
to strictly as "domain" or "domid" in an AVC has always implied it was
the source. At the same time, the target domain has always been
referenced as "target". This suggestion would completely flip that
implied understanding around. In part, this change was to move source
from being implied to being explicitly reported. The end result is it
then makes source explicit as it is for current and target.

 - AFAICT the suggestion is not logically equivalent. The current form
checks first if sdom is defined, then prints it. If sdom is not defined,
then it is presumed that tdom will be defined, and will then print it.
AIUI, the suggestion will lose the case where sdom is not defined.

 - I haven't went to confirm this, but I believe the logic here is based
on an understanding of when sdom and tdom are defined. Specifically, the
expected situations are,
  1. sdom and tdom are defined and not equal, report both
  2. if sdom and tdom are defined and equal, report only sdom as tdom
       is implied to be the same
  3. if sdom is not defined, then tdom must be defined, report only tdom
     and sdom is implied to be cdom

Finally, as I was typing this up, I had a realization that I may not be
able to relabel the reference. It is believed at some point you could
feed Xen AVCs to audit2allow to generate an allow rule for the AVC.
Though recent versions do not appear to work, so I am going to try to
find a day or two to dig in and determine what influence this might have
on the change.

v/r,
dps