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

[Juergen Gross <jgross@xxxxxxxx>]

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.

> 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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature