Re: [PATCH v2 5/7] xen: add new domctl get_changed_domain

[Jürgen Groß <jgross@xxxxxxxx>]

On 10.12.24 17:29, Jan Beulich wrote:
> On 10.12.2024 16:52, Jürgen Groß wrote:
> > On 09.12.24 18:04, Jan Beulich wrote:
> > > On 06.12.2024 14:02, Juergen Gross wrote:
> > > > --- a/xen/common/domain.c
> > > > +++ b/xen/common/domain.c
> > > > @@ -192,6 +192,54 @@ static void domain_changed_state(const struct domain *d)
> > > >        spin_unlock(&dom_state_changed_lock);
> > > >    }
> > > >    
> > > > +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
> > > > +                                  const struct domain *d)
> > > > +{
> > > > +    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
> > > > +    if ( d->is_shut_down )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
> > > > +    if ( d->is_dying == DOMDYING_dead )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> > > The public constant saying "dying" isn't quite in line with the internal
> > > constant saying "dead". It may well be that Xenstore only cares about the
> > > "dead" state, but then it would better be nemaed this way also in the
> > > public interface, I think.
> > Okay, I'll rename it to "XEN_DOMCTL_GETDOMSTATE_STATE_DEAD".
> Well, maybe have both DYING and DEAD, even if Xenstore right now needs only one?
Yes, might be interesting in the future.

> > > > @@ -866,6 +873,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > > > u_domctl)
> > > >                    __HYPERVISOR_domctl, "h", 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);
> > > > +        break;
> > > Especially with this being a stable interface, surely the two padding fields
> > > want checking to be zero on input (to possibly allow their future use for
> > > something input-ish). Then even the memset() in the function may not really
> > > be needed.
> > I'll add the check. Removing the memset() is a little bit doubtful, as this
> > might result in leaking hypervisor data e.g. in case a domain isn't existing
> > (this will copy the internal struct to the user even in the -ENOENT case).
> Which internal struct? The function is passed &op->... for both parameters.
> And op is fully copied from guest space.
Sigh. I shouldn't have answered so quickly while being deep into other
topics. :-(

While I agree that the caller _should_ pass these fields zeroed, I'm still
not sure we should rely on it. Do you insist on removing the memset()? If
not I'd rather keep it.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature