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
