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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

Jan