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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/1/8 17:25, Jan Beulich wrote:
> 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.
Ok, will call rcu_unlock_domain on 2nd error path, and change ENOSYS to 
EOPNOTSUPP.

> 
> Jan

-- 
Best regards,
Jiqian Chen.