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

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

On 16.12.24 16:18, Jan Beulich wrote:
> On 16.12.2024 16:03, Juergen Gross wrote:
> > On 16.12.24 11:41, Jan Beulich wrote:
> > > On 13.12.2024 17:24, Juergen Gross wrote:
> > > > --- a/xen/common/domain.c
> > > > +++ b/xen/common/domain.c
> > > > @@ -193,6 +193,57 @@ 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_dying )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> > > > +    if ( d->is_dying == DOMDYING_dead )
> > > > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DEAD;
> > > > +    info->unique_id = d->unique_id;
> > > > +}
> > > > +
> > > > +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain 
> > > > *d,
> > > > +                     domid_t *domid)
> > > > +{
> > > > +    unsigned int dom;
> > > > +
> > > > +    if ( info->pad0 || info->pad1 )
> > > > +        return -EINVAL;
> > > > +
> > > > +    if ( d )
> > > > +    {
> > > > +        set_domain_state_info(info, d);
> > > > +
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> > > > +            DOMID_FIRST_RESERVED )
> > > > +    {
> > > > +        if ( test_and_clear_bit(dom, dom_state_changed) )
> > > For these two accesses to dom_state_changed don't you need to hold the
> > > lock patch 4 introduces? Also didn't you say you'd constrain the new
> > > sub-op to the sole domain having VIRQ_DOM_EXEC bound (which, ftaod,
> > > isn't enough to eliminate the race)?
> > Just to be more specific regarding the race: I guess you mean that a domain
> > having registered for the VIRQ doesn't mean the calling component being in
> > that domain really is the one associated with the VIRQ.
> >
> > While being true, even today it is possible for one dom0 user process to
> > "steal" a VIRQ from another process by using dirty tricks via the privcmd
> > driver.
> >
> > In the end a process having the access rights to use the privcmd driver must
> > be trusted to not disturb other processes with the same rights.
> Of course, but that's not exactly what I was getting at. I was trying to
> point out that the vIRQ check alone is still insufficient to avoid potential
> crashes here, by one vCPU calling here while another unbinds the vIRQ. Taking
> the lock is required for Xen's safety; checking the vIRQ is bound is an extra
> policy enforcement.
Ah, okay. I think V4 will cover your scenario.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature