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
