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

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

On 2/23/26 13:01, Jason Andryuk wrote:
> 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?
Understand, which in my initial approach I considered instead of setting 
the d to NULL for DOMID_INVALID, I was considered changing it to 
dom_xen. This effectively says you must have permission to wildcard 
query xen for domain states. I opted not to include it as I was able to 
rework it so that NULL was never used for xsm_get_domain_state().
Your last statement is correct, unlabeled_t literally means it is an 
unknown object. Therefore, writing any rules for unlabeled_t is dubious 
at a minimum, but more likely an invalid security statements. The 
suggestion to write the rule was not meant to be the solution, it was 
just a means to help allow people to boot their systems for the time being.
v/r,
dps