When using XSM Flask, passing DOMIND_INVALID will result in a NULL pointer
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@apertussolutions.com>
Reported-by: Chris Rogers <rogersc@ainfosec.com>
Fixes: 3ad3df1bd0aa ("xen: add new domctl get_domain_state")
---
 xen/common/domain.c | 22 +++++++++++++++++++---
 xen/common/domctl.c |  7 ++-----
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index de6fdf59236e..4886c59c874c 100644
--- 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;
 
@@ -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;
+                    dom++;
+                    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++;
     }
 
  out:
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 29a7726d32d0..2eedc639c72a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -860,12 +860,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         break;
 
     case XEN_DOMCTL_get_domain_state:
-        ret = xsm_get_domain_state(XSM_XS_PRIV, d);
-        if ( ret )
-            break;
-
-        copyback = 1;
         ret = get_domain_state(&op->u.get_domain_state, d, &op->domain);
+        if ( !ret )
+            copyback = 1;
         break;
 
     default:
-- 
2.39.5

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@apertussolutions.com>
> Reported-by: Chris Rogers <rogersc@ainfosec.com>
> 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

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@apertussolutions.com>
>> Reported-by: Chris Rogers <rogersc@ainfosec.com>
>> 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

On 18.02.2026 15:33, Daniel P. Smith wrote:
> On 2/17/26 04:34, Jan Beulich wrote:
>> On 16.02.2026 22:57, Daniel P. Smith wrote:
>>> --- 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?

No, that could then confuse a legitimate (for that domain) caller. IOW
you would still build upon the domctl lock serializing things. I think
you want to do the XSM check first, and only then use test_and_clear_bit().

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

I wouldn't outright nak use of plain int, but I'm putting in effort to remove
such undue uses of that type. Unsigned overflow is well-defined aiui, so I
see no reason why the variable can't remain "unsigned int".

Jan

On 2/18/26 10:03, Jan Beulich wrote:
> On 18.02.2026 15:33, Daniel P. Smith wrote:
>> On 2/17/26 04:34, Jan Beulich wrote:
>>> On 16.02.2026 22:57, Daniel P. Smith wrote:
>>>> --- 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?
> 
> No, that could then confuse a legitimate (for that domain) caller. IOW
> you would still build upon the domctl lock serializing things. I think
> you want to do the XSM check first, and only then use test_and_clear_bit().
> 

Currently, acquiring the struct domain pointer is inside the if 
condition. This would have to be moved outside the if condition to be 
able to do the access check before calling the bit operation. Which 
brings up the question for me, if it is reordered in this fashion, why 
not use clear_bit(), aiui it should be atomic as well.


>>>> @@ -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.
> 
> I wouldn't outright nak use of plain int, but I'm putting in effort to remove
> such undue uses of that type. Unsigned overflow is well-defined aiui, so I
> see no reason why the variable can't remain "unsigned int".

Honestly, I'm not sure why it was not domid_t in the first place. I 
can't keep all the rules, but I thought MISRA frowned on overflow usages 
(abuse?) like this. If you will ack it with it as a uint or as a 
domid_t, then I have no issue doing it this way.

v/r,
dps

On 18.02.2026 16:32, Daniel P. Smith wrote:
> On 2/18/26 10:03, Jan Beulich wrote:
>> On 18.02.2026 15:33, Daniel P. Smith wrote:
>>> On 2/17/26 04:34, Jan Beulich wrote:
>>>> On 16.02.2026 22:57, Daniel P. Smith wrote:
>>>>> --- 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?
>>
>> No, that could then confuse a legitimate (for that domain) caller. IOW
>> you would still build upon the domctl lock serializing things. I think
>> you want to do the XSM check first, and only then use test_and_clear_bit().
> 
> Currently, acquiring the struct domain pointer is inside the if 
> condition. This would have to be moved outside the if condition to be 
> able to do the access check before calling the bit operation. Which 
> brings up the question for me, if it is reordered in this fashion, why 
> not use clear_bit(), aiui it should be atomic as well.

The problem isn't with clear_bit() by itself (yes, it is atomic), but
with how things want doing here: The test-and-clear ensures data for a
given domain can only be retrieved exactly once (until the bit would be
set another time later). With test split from clear, multiple callers
could get data for the same domain, as in the window between test and
clear a 2nd caller can come in and also commit to returning data for
that domain. (Given the restriction on who can invoke this, one might
conclude no such race is possible, but that single domain could process
things on multiple vCPU-s, and would then wrongly be notified twice for
the same domain.)

>>>>> @@ -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.
>>
>> I wouldn't outright nak use of plain int, but I'm putting in effort to remove
>> such undue uses of that type. Unsigned overflow is well-defined aiui, so I
>> see no reason why the variable can't remain "unsigned int".
> 
> Honestly, I'm not sure why it was not domid_t in the first place. I 
> can't keep all the rules, but I thought MISRA frowned on overflow usages 
> (abuse?) like this. If you will ack it with it as a uint or as a 
> domid_t, then I have no issue doing it this way.

A patch without changing the type I would ack. A switch to domid_t, if indeed
correct (I simply didn't go check yet), likely would want to be a separate
change.

Jan

On 2/18/26 10:47, Jan Beulich wrote:
> On 18.02.2026 16:32, Daniel P. Smith wrote:
>> On 2/18/26 10:03, Jan Beulich wrote:
>>> On 18.02.2026 15:33, Daniel P. Smith wrote:
>>>> On 2/17/26 04:34, Jan Beulich wrote:
>>>>> On 16.02.2026 22:57, Daniel P. Smith wrote:
>>>>>> --- 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?
>>>
>>> No, that could then confuse a legitimate (for that domain) caller. IOW
>>> you would still build upon the domctl lock serializing things. I think
>>> you want to do the XSM check first, and only then use test_and_clear_bit().
>>
>> Currently, acquiring the struct domain pointer is inside the if
>> condition. This would have to be moved outside the if condition to be
>> able to do the access check before calling the bit operation. Which
>> brings up the question for me, if it is reordered in this fashion, why
>> not use clear_bit(), aiui it should be atomic as well.
> 
> The problem isn't with clear_bit() by itself (yes, it is atomic), but
> with how things want doing here: The test-and-clear ensures data for a
> given domain can only be retrieved exactly once (until the bit would be
> set another time later). With test split from clear, multiple callers
> could get data for the same domain, as in the window between test and
> clear a 2nd caller can come in and also commit to returning data for
> that domain. (Given the restriction on who can invoke this, one might
> conclude no such race is possible, but that single domain could process
> things on multiple vCPU-s, and would then wrongly be notified twice for
> the same domain.)

Thinking further, you are correct the rcu_read_lock will not be enough. 
The test_and_clear call will ensure this thread is the one that read and 
cleared the bit.

>>>>>> @@ -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.
>>>
>>> I wouldn't outright nak use of plain int, but I'm putting in effort to remove
>>> such undue uses of that type. Unsigned overflow is well-defined aiui, so I
>>> see no reason why the variable can't remain "unsigned int".
>>
>> Honestly, I'm not sure why it was not domid_t in the first place. I
>> can't keep all the rules, but I thought MISRA frowned on overflow usages
>> (abuse?) like this. If you will ack it with it as a uint or as a
>> domid_t, then I have no issue doing it this way.
> 
> A patch without changing the type I would ack. A switch to domid_t, if indeed
> correct (I simply didn't go check yet), likely would want to be a separate
> change.

Then I will leave it uint and init it to -1.

v/r,
dps