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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-02-19 07:15, Daniel P. Smith wrote:
> On 2/18/26 18:04, Jason Andryuk wrote:
> > > 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);
> > With the initial NULL deref issue, I wondered if this wouldn't be 
> > better off modified to drop the d param - 
> > xsm_get_domain_state(XSM_XS_PRIV) - and changing flask permissions like:
> > allow dom0_t xen_t:xen get_domain_state;
> >
> > That would gate the external call, and individual domain permissions 
> > could be checked with xsm_getdomaininfo(), or a new hook if you don't 
> > want to re-use.
> > But as your approach avoids passing NULL, it seems okay to me.  It 
> > also doesn't change the flask policy, which is nice.
> That's a plain nack from me. Whether it is viewed as a pipe dream or 
> not, my goal continues to be to work towards enabling the ability to 
> have a truly disaggregated platform. In the original architecture, it 
> was envisioned to have multiple toolstack domains, each responsible for 
> a distinct set of domains. In terms of implementation, that would mean 
> multiple xenstored instances, each with a purview over a subset of domains.
I don't think what I wrote is at odds with your pipe dream.

My main concern is passing NULL as a domain.  I think that is wrong, 
beyond the fault seen on ARM.  In domain_target_sid(), I think the NULL 
dst was mistakenly matched to dom0's NULL src->target.  src->target_sid 
is 0 from zalloc, which is not otherwise initialized and invalid.  That 
is returned.  Later when sidtab_search() can't find it, it is remapped 
to SECINITSID_UNLABELED and returned.  That silent remapping is dubious, 
and it points toward unlabled_t should never be allowed in any rule. 
Maybe it should remap unknown sids to a dedicated invalid_t, but maybe 
invalid_t is already supposed to be that?
Regards,
Jason