Re: [BUG] common/domctl: xsm update for get_domain_state access

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

On 2/17/26 04:34, Jan Beulich wrote:
> On 16.02.2026 22:57, Daniel P. Smith wrote:
> > When using XSM Flask, passing DOMIND_INVALID will result in a NULL pointer
> Nit: DOMID_INVALID
ack.


> > reference from the passing of NULL as the target domain to
> > xsm_get_domain_state(). Simply not invoking xsm_get_domain_state() when the
> > target domain is NULL opens the opportunity to circumvent the XSM
> > get_domain_state access check. This is due to the fact that the call to
> > xsm_domctl() for get_domain_state op is a no-op check, deferring to
> > xsm_get_domain_state().
> >
> > Modify the helper get_domain_state() to ensure the requesting domain has
> > get_domain_state access for the target domain, whether the target domain is
> > explicitly set or implicitly determined with a domain state search. In the case
> > of access not being allowed for a domain found during an implicit search, the
> > search will continue to the next domain whose state has changed.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > Reported-by: Chris Rogers <rogersc@xxxxxxxxxxxx>
> > Fixes: 3ad3df1bd0aa ("xen: add new domctl get_domain_state")
> Nit: Fixes: first (or at least ahead of S-o-b) and other tags chronologically
> ordered, please.
>
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -210,7 +210,7 @@ static void set_domain_state_info(struct 
> > xen_domctl_get_domain_state *info,
> >   int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain 
> > *d,
> >                        domid_t *domid)
> >   {
> > -    unsigned int dom;
> > +    unsigned int dom = 0;
> >       int rc = -ENOENT;
> >       struct domain *hdl;
> >   
> > @@ -219,6 +219,10 @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d,
> >   
> >       if ( d )
> >       {
> > +        rc = xsm_get_domain_state(XSM_XS_PRIV, d);
> > +        if ( rc )
> > +            return rc;
> > +
> >           set_domain_state_info(info, d);
> >   
> >           return 0;
> > @@ -238,10 +242,10 @@ int get_domain_state(struct xen_domctl_get_domain_state 
> > *info, struct domain *d,
> >   
> >       while ( dom_state_changed )
> >       {
> > -        dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
> > +        dom = find_next_bit(dom_state_changed, DOMID_MASK + 1, dom);
> >           if ( dom >= DOMID_FIRST_RESERVED )
> >               break;
> > -        if ( test_and_clear_bit(dom, dom_state_changed) )
> > +        if ( test_bit(dom, dom_state_changed) )
> >           {
> >               *domid = dom;
> This is problematic wrt other work (already talked about in the distant past,
> but sadly only making little progress) towards trying to pull some of the
> sub-ops out of the domctl-locked region. This subop is one of the prime
> candidates, yet only if the test_and_clear_bit() remains here.
Okay, but we can't be clearing the bit if the src domain doesn't have 
access. When considering that xsm_domctl() does a no-op check for 
XEN_DOMCTL_get_domain_state, deferring to xsm_get_domain_state(), then 
any domain could invoke the OP with DOMID_INVALID and clear the bit 
before access is checked.
If you want to ensure atomic operations on the bit field, while I am not 
a fan of this, a combination with set_bit() could be done. Let the 
test_and_clear_bit() remain and then if access check fails, use 
set_bit() to put it back. Would that be sufficient for your objective?
> > @@ -249,6 +253,15 @@ int get_domain_state(struct xen_domctl_get_domain_state 
> > *info, struct domain *d,
> >   
> >               if ( d )
> >               {
> > +                rc = xsm_get_domain_state(XSM_XS_PRIV, d);
> > +                if ( rc )
> > +                {
> > +                    rcu_unlock_domain(d);
> > +                    rc = -ENOENT;
> As you don't otherwise use xsm_get_domain_state()'s return value, the need
> for this assignment can be eliminated by putting the function call straight
> in the if(). Then again, to address the remark above, overall code structure
> will need to change quite a bit anyway (so the remark here may be moot).
I can drop the use of rc here and inline it.

> > +                    dom++;
> It may be nice to eliminate the need to have this in two places (here and ...
>
> > +                    continue;
> > +                }
> > +
> >                   set_domain_state_info(info, d);
> >   
> >                   rcu_unlock_domain(d);
> > @@ -256,10 +269,13 @@ int get_domain_state(struct xen_domctl_get_domain_state 
> > *info, struct domain *d,
> >               else
> >                   memset(info, 0, sizeof(*info));
> >   
> > +            clear_bit(dom, dom_state_changed);
> >               rc = 0;
> >   
> >               break;
> >           }
> > +
> > +        dom++;
> >       }
> ... here), by having the variable's initializer be -1 and then using dom + 1
> in the find_next_bit() invocation.
If you want this way, then there are two options, make dom no longer 
unsigned or be willing to allow unsigned int overflow. If we go with the 
former, If you agree, I would leave it as an int as that should cover 
the range of valid domids.
v/r,
dps