Re: [PATCH] xen/sysctl: fix XEN_SYSCTL_getdomaininfolist handling with XSM

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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