| Message ID | 20230430144646.13624-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM | expand |
On 30.04.2023 16:46, Juergen Gross wrote: > In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist > can fail if the last domain scanned isn't allowed to be accessed by > the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). > > Fix that by just ignoring scanned domains where xsm_getdomaininfo() > is returning an error, like it is effectively done when such a > situation occurs for a domain not being the last one scanned. > > Fixes: d046f361dc93 ("Xen Security Modules: XSM") > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Should have finished reading the list first, before sending the same patch again ... Jan
On 4/30/23 10:46, Juergen Gross wrote: > In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist > can fail if the last domain scanned isn't allowed to be accessed by > the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). > > Fix that by just ignoring scanned domains where xsm_getdomaininfo() > is returning an error, like it is effectively done when such a > situation occurs for a domain not being the last one scanned. > > Fixes: d046f361dc93 ("Xen Security Modules: XSM") > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > xen/common/sysctl.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c > index 02505ab044..0cbfe8bd44 100644 > --- a/xen/common/sysctl.c > +++ b/xen/common/sysctl.c > @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > if ( num_domains == op->u.getdomaininfolist.max_domains ) > break; > > - ret = xsm_getdomaininfo(XSM_HOOK, d); > - if ( ret ) > + if ( xsm_getdomaininfo(XSM_HOOK, d) ) > continue; > > getdomaininfo(d, &info); This change does not match the commit message. This says it fixes an issue, but unless I am totally missing something, this change is nothing more than formatting that drops the use of an intermediate variable. Please feel free to correct me if I am wrong here, otherwise I believe the commit message should be changed to reflect the code change. Second, as far as the problem description goes. The *only* time the call to xsm_getdomaininfo() at this location will return anything other than 0, is when FLASK is being used and a domain whose type is not allowed getdomaininfo is making the call. XSM_HOOK signals a no-op check for the default/dummy policy, and the SILO policy does not override the default/dummy policy for this check. V/r, Daniel P. Smith
On Tue, May 02, 2023 at 09:03:00AM -0400, Daniel P. Smith wrote: > On 4/30/23 10:46, Juergen Gross wrote: > > In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist > > can fail if the last domain scanned isn't allowed to be accessed by > > the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). > > > > Fix that by just ignoring scanned domains where xsm_getdomaininfo() > > is returning an error, like it is effectively done when such a > > situation occurs for a domain not being the last one scanned. > > > > Fixes: d046f361dc93 ("Xen Security Modules: XSM") > > Signed-off-by: Juergen Gross <jgross@suse.com> > > --- > > xen/common/sysctl.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c > > index 02505ab044..0cbfe8bd44 100644 > > --- a/xen/common/sysctl.c > > +++ b/xen/common/sysctl.c > > @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > > if ( num_domains == op->u.getdomaininfolist.max_domains ) > > break; > > - ret = xsm_getdomaininfo(XSM_HOOK, d); > > - if ( ret ) > > + if ( xsm_getdomaininfo(XSM_HOOK, d) ) > > continue; > > getdomaininfo(d, &info); > > > This change does not match the commit message. This says it fixes an issue, > but unless I am totally missing something, this change is nothing more than > formatting that drops the use of an intermediate variable. Please feel free > to correct me if I am wrong here, otherwise I believe the commit message > should be changed to reflect the code change. By dropping that intermediate variable it prevents returning an error as the result of the hypercall if xsm_getdomaininfo() for the last domain fails. Note that xsm_getdomaininfo() failing for other domains not the last one don't cause the return value of the hypercall to be an error code, because the variable containing the error gets overwritten by further loops. Regards, Roger.
On 02.05.23 15:03, Daniel P. Smith wrote: > On 4/30/23 10:46, Juergen Gross wrote: >> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >> can fail if the last domain scanned isn't allowed to be accessed by >> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >> >> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >> is returning an error, like it is effectively done when such a >> situation occurs for a domain not being the last one scanned. >> >> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> xen/common/sysctl.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >> index 02505ab044..0cbfe8bd44 100644 >> --- a/xen/common/sysctl.c >> +++ b/xen/common/sysctl.c >> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >> if ( num_domains == op->u.getdomaininfolist.max_domains ) >> break; >> - ret = xsm_getdomaininfo(XSM_HOOK, d); >> - if ( ret ) >> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >> continue; >> getdomaininfo(d, &info); > > > This change does not match the commit message. This says it fixes an issue, but > unless I am totally missing something, this change is nothing more than > formatting that drops the use of an intermediate variable. Please feel free to > correct me if I am wrong here, otherwise I believe the commit message should be > changed to reflect the code change. You are missing the fact that ret getting set by a failing xsm_getdomaininfo() call might result in the ret value being propagated to the sysctl caller. And this should not happen. So the fix is to NOT modify ret here. > Second, as far as the problem description goes. The *only* time the call to > xsm_getdomaininfo() at this location will return anything other than 0, is when > FLASK is being used and a domain whose type is not allowed getdomaininfo is > making the call. XSM_HOOK signals a no-op check for the default/dummy policy, > and the SILO policy does not override the default/dummy policy for this check. Your statement sounds as if xsm_getdomaininfo() would always return the same value for a given caller domain. Isn't that return value also depending on the domain specified via the second parameter? In case it isn't, why does that parameter even exist? Juergen
On 5/2/23 09:10, Roger Pau Monné wrote: > On Tue, May 02, 2023 at 09:03:00AM -0400, Daniel P. Smith wrote: >> On 4/30/23 10:46, Juergen Gross wrote: >>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >>> can fail if the last domain scanned isn't allowed to be accessed by >>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >>> >>> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >>> is returning an error, like it is effectively done when such a >>> situation occurs for a domain not being the last one scanned. >>> >>> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> xen/common/sysctl.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >>> index 02505ab044..0cbfe8bd44 100644 >>> --- a/xen/common/sysctl.c >>> +++ b/xen/common/sysctl.c >>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >>> if ( num_domains == op->u.getdomaininfolist.max_domains ) >>> break; >>> - ret = xsm_getdomaininfo(XSM_HOOK, d); >>> - if ( ret ) >>> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >>> continue; >>> getdomaininfo(d, &info); >> >> >> This change does not match the commit message. This says it fixes an issue, >> but unless I am totally missing something, this change is nothing more than >> formatting that drops the use of an intermediate variable. Please feel free >> to correct me if I am wrong here, otherwise I believe the commit message >> should be changed to reflect the code change. > > By dropping that intermediate variable it prevents returning an error > as the result of the hypercall if xsm_getdomaininfo() for the last > domain fails. Ah, understood. I missed ret is state tracking. > Note that xsm_getdomaininfo() failing for other domains not the last > one don't cause the return value of the hypercall to be an error > code, because the variable containing the error gets overwritten by > further loops. In the end, this is just addressing an issue that has not been seen by anyone and happened upon while debugging another issue. V/r, DPS
On 5/2/23 09:13, Juergen Gross wrote: > On 02.05.23 15:03, Daniel P. Smith wrote: >> On 4/30/23 10:46, Juergen Gross wrote: >>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >>> can fail if the last domain scanned isn't allowed to be accessed by >>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >>> >>> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >>> is returning an error, like it is effectively done when such a >>> situation occurs for a domain not being the last one scanned. >>> >>> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> xen/common/sysctl.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >>> index 02505ab044..0cbfe8bd44 100644 >>> --- a/xen/common/sysctl.c >>> +++ b/xen/common/sysctl.c >>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) >>> u_sysctl) >>> if ( num_domains == op->u.getdomaininfolist.max_domains ) >>> break; >>> - ret = xsm_getdomaininfo(XSM_HOOK, d); >>> - if ( ret ) >>> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >>> continue; >>> getdomaininfo(d, &info); >> >> >> This change does not match the commit message. This says it fixes an >> issue, but unless I am totally missing something, this change is >> nothing more than formatting that drops the use of an intermediate >> variable. Please feel free to correct me if I am wrong here, otherwise >> I believe the commit message should be changed to reflect the code >> change. > > You are missing the fact that ret getting set by a failing > xsm_getdomaininfo() > call might result in the ret value being propagated to the sysctl > caller. And > this should not happen. So the fix is to NOT modify ret here. You are correct, my apologies for that. >> Second, as far as the problem description goes. The *only* time the >> call to xsm_getdomaininfo() at this location will return anything >> other than 0, is when FLASK is being used and a domain whose type is >> not allowed getdomaininfo is making the call. XSM_HOOK signals a no-op >> check for the default/dummy policy, and the SILO policy does not >> override the default/dummy policy for this check. > > Your statement sounds as if xsm_getdomaininfo() would always return the > same > value for a given caller domain. Isn't that return value also depending > on the > domain specified via the second parameter? In case it isn't, why does that > parameter even exist? It would if the default action was something other than XSM_HOOK. Look at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 regardless of the src or dest domains. The function xsm_defualt_action() is the policy for both default/dummy and SILO with the exception for evntchn, grants, and argo checks for SILO. v/r, DPS
On 5/2/23 09:23, Daniel P. Smith wrote: > On 5/2/23 09:13, Juergen Gross wrote: >> On 02.05.23 15:03, Daniel P. Smith wrote: >>> On 4/30/23 10:46, Juergen Gross wrote: >>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >>>> can fail if the last domain scanned isn't allowed to be accessed by >>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >>>> >>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >>>> is returning an error, like it is effectively done when such a >>>> situation occurs for a domain not being the last one scanned. >>>> >>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> xen/common/sysctl.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >>>> index 02505ab044..0cbfe8bd44 100644 >>>> --- a/xen/common/sysctl.c >>>> +++ b/xen/common/sysctl.c >>>> @@ -89,8 +89,7 @@ long >>>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >>>> if ( num_domains == op->u.getdomaininfolist.max_domains ) >>>> break; >>>> - ret = xsm_getdomaininfo(XSM_HOOK, d); >>>> - if ( ret ) >>>> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >>>> continue; >>>> getdomaininfo(d, &info); >>> >>> >>> This change does not match the commit message. This says it fixes an >>> issue, but unless I am totally missing something, this change is >>> nothing more than formatting that drops the use of an intermediate >>> variable. Please feel free to correct me if I am wrong here, >>> otherwise I believe the commit message should be changed to reflect >>> the code change. >> >> You are missing the fact that ret getting set by a failing >> xsm_getdomaininfo() >> call might result in the ret value being propagated to the sysctl >> caller. And >> this should not happen. So the fix is to NOT modify ret here. > > You are correct, my apologies for that. > >>> Second, as far as the problem description goes. The *only* time the >>> call to xsm_getdomaininfo() at this location will return anything >>> other than 0, is when FLASK is being used and a domain whose type is >>> not allowed getdomaininfo is making the call. XSM_HOOK signals a >>> no-op check for the default/dummy policy, and the SILO policy does >>> not override the default/dummy policy for this check. >> >> Your statement sounds as if xsm_getdomaininfo() would always return >> the same >> value for a given caller domain. Isn't that return value also >> depending on the >> domain specified via the second parameter? In case it isn't, why does >> that >> parameter even exist? > > It would if the default action was something other than XSM_HOOK. Look > at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 > regardless of the src or dest domains. The function xsm_defualt_action() > is the policy for both default/dummy and SILO with the exception for > evntchn, grants, and argo checks for SILO. Sorry, one last clarification. xsm_default_action() is also what is used when XSM=n. The difference is that for XSM=n, xsm_default_action() is in-lined at the call site whereas with XSM=y and not using FLASK results in a function call xsm_default_action(). v/r, dps
On 02.05.23 15:23, Daniel P. Smith wrote: > On 5/2/23 09:13, Juergen Gross wrote: >> On 02.05.23 15:03, Daniel P. Smith wrote: >>> On 4/30/23 10:46, Juergen Gross wrote: >>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >>>> can fail if the last domain scanned isn't allowed to be accessed by >>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >>>> >>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >>>> is returning an error, like it is effectively done when such a >>>> situation occurs for a domain not being the last one scanned. >>>> >>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>> --- >>>> xen/common/sysctl.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >>>> index 02505ab044..0cbfe8bd44 100644 >>>> --- a/xen/common/sysctl.c >>>> +++ b/xen/common/sysctl.c >>>> @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >>>> if ( num_domains == op->u.getdomaininfolist.max_domains ) >>>> break; >>>> - ret = xsm_getdomaininfo(XSM_HOOK, d); >>>> - if ( ret ) >>>> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >>>> continue; >>>> getdomaininfo(d, &info); >>> >>> >>> This change does not match the commit message. This says it fixes an issue, >>> but unless I am totally missing something, this change is nothing more than >>> formatting that drops the use of an intermediate variable. Please feel free >>> to correct me if I am wrong here, otherwise I believe the commit message >>> should be changed to reflect the code change. >> >> You are missing the fact that ret getting set by a failing xsm_getdomaininfo() >> call might result in the ret value being propagated to the sysctl caller. And >> this should not happen. So the fix is to NOT modify ret here. > > You are correct, my apologies for that. No need to apologize. :-) >>> Second, as far as the problem description goes. The *only* time the call to >>> xsm_getdomaininfo() at this location will return anything other than 0, is >>> when FLASK is being used and a domain whose type is not allowed getdomaininfo >>> is making the call. XSM_HOOK signals a no-op check for the default/dummy >>> policy, and the SILO policy does not override the default/dummy policy for >>> this check. >> >> Your statement sounds as if xsm_getdomaininfo() would always return the same >> value for a given caller domain. Isn't that return value also depending on the >> domain specified via the second parameter? In case it isn't, why does that >> parameter even exist? > > It would if the default action was something other than XSM_HOOK. Look at line > 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 regardless of the src > or dest domains. The function xsm_defualt_action() is the policy for both > default/dummy and SILO with the exception for evntchn, grants, and argo checks > for SILO. Ah, okay. I didn't analyze all of the involved xsm code. Juergen
On 5/2/23 09:30, Juergen Gross wrote: > On 02.05.23 15:23, Daniel P. Smith wrote: >> On 5/2/23 09:13, Juergen Gross wrote: >>> On 02.05.23 15:03, Daniel P. Smith wrote: >>>> On 4/30/23 10:46, Juergen Gross wrote: >>>>> In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist >>>>> can fail if the last domain scanned isn't allowed to be accessed by >>>>> the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). >>>>> >>>>> Fix that by just ignoring scanned domains where xsm_getdomaininfo() >>>>> is returning an error, like it is effectively done when such a >>>>> situation occurs for a domain not being the last one scanned. >>>>> >>>>> Fixes: d046f361dc93 ("Xen Security Modules: XSM") >>>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>>>> --- >>>>> xen/common/sysctl.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c >>>>> index 02505ab044..0cbfe8bd44 100644 >>>>> --- a/xen/common/sysctl.c >>>>> +++ b/xen/common/sysctl.c >>>>> @@ -89,8 +89,7 @@ long >>>>> do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) >>>>> if ( num_domains == >>>>> op->u.getdomaininfolist.max_domains ) >>>>> break; >>>>> - ret = xsm_getdomaininfo(XSM_HOOK, d); >>>>> - if ( ret ) >>>>> + if ( xsm_getdomaininfo(XSM_HOOK, d) ) >>>>> continue; >>>>> getdomaininfo(d, &info); >>>> >>>> >>>> This change does not match the commit message. This says it fixes an >>>> issue, but unless I am totally missing something, this change is >>>> nothing more than formatting that drops the use of an intermediate >>>> variable. Please feel free to correct me if I am wrong here, >>>> otherwise I believe the commit message should be changed to reflect >>>> the code change. >>> >>> You are missing the fact that ret getting set by a failing >>> xsm_getdomaininfo() >>> call might result in the ret value being propagated to the sysctl >>> caller. And >>> this should not happen. So the fix is to NOT modify ret here. >> >> You are correct, my apologies for that. > > No need to apologize. :-) I believe it is proper to admit when you are wrong. >>>> Second, as far as the problem description goes. The *only* time the >>>> call to xsm_getdomaininfo() at this location will return anything >>>> other than 0, is when FLASK is being used and a domain whose type is >>>> not allowed getdomaininfo is making the call. XSM_HOOK signals a >>>> no-op check for the default/dummy policy, and the SILO policy does >>>> not override the default/dummy policy for this check. >>> >>> Your statement sounds as if xsm_getdomaininfo() would always return >>> the same >>> value for a given caller domain. Isn't that return value also >>> depending on the >>> domain specified via the second parameter? In case it isn't, why does >>> that >>> parameter even exist? >> >> It would if the default action was something other than XSM_HOOK. Look >> at line 82 of include/xsm/dummy.h. XSM_HOOK will always return 0 >> regardless of the src or dest domains. The function >> xsm_defualt_action() is the policy for both default/dummy and SILO >> with the exception for evntchn, grants, and argo checks for SILO. > > Ah, okay. I didn't analyze all of the involved xsm code. No worries! I am always willing to help in any way that I can. While I don't have the bandwidth to be proactive and keep up with everything on xen-devel, please do not hesitate to ask me or ping me on anything XSM related. I will gladly take a look and provide what insights I might have on your query. v/r, dps
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c index 02505ab044..0cbfe8bd44 100644 --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -89,8 +89,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) if ( num_domains == op->u.getdomaininfolist.max_domains ) break; - ret = xsm_getdomaininfo(XSM_HOOK, d); - if ( ret ) + if ( xsm_getdomaininfo(XSM_HOOK, d) ) continue; getdomaininfo(d, &info);
In case XSM is active, the handling of XEN_SYSCTL_getdomaininfolist can fail if the last domain scanned isn't allowed to be accessed by the calling domain (i.e. xsm_getdomaininfo(XSM_HOOK, d) is failing). Fix that by just ignoring scanned domains where xsm_getdomaininfo() is returning an error, like it is effectively done when such a situation occurs for a domain not being the last one scanned. Fixes: d046f361dc93 ("Xen Security Modules: XSM") Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/sysctl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)