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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2022 17:41, Daniel P. Smith wrote:
> 
> 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.

No, I don't. And I therefore think you further reply (left intact below)
also doesn't really apply. The last else only applies when sdom == NULL,
but the goal of my suggestion is to distinguish tdom == NULL from
tdom == sdom.

>  - 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

This isn't describing the behavior - tdom could also be NULL here. This
is the case I think wants expressing in a way different from sdom == tdom.

>   3. if sdom is not defined, then tdom must be defined, report only tdom
>      and sdom is implied to be cdom

There are also no assumptions - see the enclosing if(). cdom is printed
only if "a" is NULL (implying sdom and tdom to be NULL) or both sdom and
tdom are NULL.

Jan

> 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