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.
