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.
