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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan