Re: [RFC XEN PATCH v4 2/5] x86/pvh: Allow (un)map_pirq when caller isn't DOMID_SELF

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2024 10:15, Chen, Jiqian wrote:
> On 2024/1/8 16:47, Jan Beulich wrote:
>> On 06.01.2024 01:46, Stefano Stabellini wrote:
>>> On Fri, 5 Jan 2024, Jiqian Chen wrote:
>>>> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd, 
>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>  
>>>>      switch ( cmd )
>>>>      {
>>>> -    case PHYSDEVOP_map_pirq:
>>>> -    case PHYSDEVOP_unmap_pirq:
>>>> +    case PHYSDEVOP_map_pirq: {
>>>> +        physdev_map_pirq_t map;
>>>> +
>>>> +        if ( copy_from_guest(&map, arg, 1) != 0 )
>>>> +            return -EFAULT;
>>>> +
>>>> +        if ( !has_pirq(currd) && map.domid == DOMID_SELF )
>>>> +            return -ENOSYS;
>>>
>>> This looks OK to me although there is already another copy_from_guest in
>>> do_physdev_op, but I don't see an easy way to make it better.
>>
>> How can double reads of hypercall args ever be okay? The new check clearly
>> needs to be inserted in the code path where the structure is being read
>> already anyway.
> I also tried to add this check in PHYSDEVOP_map_pirq in physdev.c, but pv has 
> no flag X86_EMU_USE_PIRQ too.
> If want to add it into physdev.c and combine Stefano's opinions, this check 
> may be like:
> 
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 47c4da0af7e1..c38d4d405726 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -303,11 +303,19 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>      case PHYSDEVOP_map_pirq: {
>          physdev_map_pirq_t map;
>          struct msi_info msi;
> +        struct domain *d;
> 
>          ret = -EFAULT;
>          if ( copy_from_guest(&map, arg, 1) != 0 )
>              break;
> 
> +        d = rcu_lock_domain_by_any_id(map.domid);
> +        if ( d == NULL )
> +            return -ESRCH;
> +        if ( !is_pv_domain(d) && !has_pirq(d) )
> +            return -ENOSYS;
> +        rcu_unlock_domain(d);
> +
>          switch ( map.type )
>          {
>          case MAP_PIRQ_TYPE_MSI_SEG:

Well, yes, perhaps kind of like that, but with rcu_unlock_domain() called
on the error 2nd return path as well, and without abusing ENOSYS.

Jan