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

[Juergen Gross <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature