Print messages from flask use an inconsistent format when printing the domain
id. The %pd conversion specifier provides a consistent way to format for the
domain id and aligns with the rest of the hypervisor code.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/xsm/flask/avc.c   | 8 ++++----
 xen/xsm/flask/hooks.c | 3 +--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
index 4a75ec97e2..4a86681c81 100644
--- 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);
     }
     else if ( cdom )
-        avc_printk(&buf, "domid=%d ", cdom->domain_id);
+        avc_printk(&buf, "current=%pd ", cdom);
     switch ( a ? a->type : 0 ) {
     case AVC_AUDIT_DATA_DEV:
         avc_printk(&buf, "device=%#lx ", a->device);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 8bd56644ef..a79281bdb0 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -281,8 +281,7 @@ static int cf_check flask_evtchn_interdomain(
     rc = security_transition_sid(sid1, sid2, SECCLASS_EVENT, &newsid);
     if ( rc )
     {
-        printk("security_transition_sid failed, rc=%d, Dom%d\n",
-               -rc, d2->domain_id);
+        printk("security_transition_sid failed, rc=%d, %pd\n", -rc, d2);
         return rc;
     }
 
-- 
2.20.1

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?

Jan

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.

v/r,
dps

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

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

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

Hi Daniel,

> -----Original Message-----
> Subject: [PATCH] xsm/flask: adjust print messages to use %pd
> 
> Print messages from flask use an inconsistent format when printing the
> domain
> id. The %pd conversion specifier provides a consistent way to format for the
> domain id and aligns with the rest of the hypervisor code.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/xsm/flask/avc.c   | 8 ++++----
>  xen/xsm/flask/hooks.c | 3 +--
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
> index 4a75ec97e2..4a86681c81 100644
> --- 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);

I guess this should be target=%pd?

With this fixed:
Reviewed-by: Henry Wang <Henry.Wang@arm.com>

This should also be fine to merge in 4.17, but following the discussion with
Julien and Jan I think providing a Release ack would lead to confusion...

Kind regards,
Henry


>          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);
>      }
>      else if ( cdom )
> -        avc_printk(&buf, "domid=%d ", cdom->domain_id);
> +        avc_printk(&buf, "current=%pd ", cdom);
>      switch ( a ? a->type : 0 ) {
>      case AVC_AUDIT_DATA_DEV:
>          avc_printk(&buf, "device=%#lx ", a->device);
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 8bd56644ef..a79281bdb0 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -281,8 +281,7 @@ static int cf_check flask_evtchn_interdomain(
>      rc = security_transition_sid(sid1, sid2, SECCLASS_EVENT, &newsid);
>      if ( rc )
>      {
> -        printk("security_transition_sid failed, rc=%d, Dom%d\n",
> -               -rc, d2->domain_id);
> +        printk("security_transition_sid failed, rc=%d, %pd\n", -rc, d2);
>          return rc;
>      }
> 
> --
> 2.20.1
> 

On 9/9/22 05:56, Henry Wang wrote:
> Hi Daniel,
> 
>> -----Original Message-----
>> Subject: [PATCH] xsm/flask: adjust print messages to use %pd
>>
>> Print messages from flask use an inconsistent format when printing the
>> domain
>> id. The %pd conversion specifier provides a consistent way to format for the
>> domain id and aligns with the rest of the hypervisor code.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/xsm/flask/avc.c   | 8 ++++----
>>   xen/xsm/flask/hooks.c | 3 +--
>>   2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
>> index 4a75ec97e2..4a86681c81 100644
>> --- 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);
> 
> I guess this should be target=%pd?

Erg! Turns out there is no conversion specifier validation occurring for 
avc_printk(), thus the typo slipped through and not caught by the 
compiler. I will fix and also add the printf annotation to avc_printk() 
to help ensure conversion specifier and parameter types match.

> With this fixed:
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks!

> This should also be fine to merge in 4.17, but following the discussion with
> Julien and Jan I think providing a Release ack would lead to confusion...

I was hoping it would go in, but understand if it is kept out. I have a 
list of clean-ups of varying sizes for XSM, with this one actually being 
requested by someone else. I figured it was a simple one that could be 
done quickly and might be worth trying to get it into the release.

v/r,
dps

Hi Daniel,

> -----Original Message-----
> From: Daniel P. Smith <dpsmith@apertussolutions.com>
> > This should also be fine to merge in 4.17, but following the discussion with
> > Julien and Jan I think providing a Release ack would lead to confusion...
> 
> I was hoping it would go in, but understand if it is kept out. I have a
> list of clean-ups of varying sizes for XSM, with this one actually being
> requested by someone else. I figured it was a simple one that could be
> done quickly and might be worth trying to get it into the release.

Sorry I should have been more clear. I am not providing this release ack
tag is not because I am blocking this patch, but following the discussion
the release tag is supposed to take effect after the code freeze (Sep. 30).

As I believe you are the XSM maintainer so if this patch and the rest of
the cleanup/bugfix patches can be properly reviewed, these should go
into the release of course.

Kind regards,
Henry

> 
> v/r,
> dps