| Message ID | 20220909095012.4251-1-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xsm/flask: adjust print messages to use %pd | expand |
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 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 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
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
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
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
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; }
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(-)