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 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:

> 
> Jan

-- 
Best regards,
Jiqian Chen.