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 <[email protected]>
Reported-by: Chris Rogers <[email protected]>
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
