[PATCH] hw/net/vmxnet3: Do not abort if guest provides bad interrupt numbers

[PATCH] hw/net/vmxnet3: Do not abort if guest provides bad interrupt numbers

[Thomas Huth]

From: Thomas Huth <thuth@redhat.com>

vmxnet3_validate_interrupts() currently aborts via hw_error() if
the guest provided bad interrupt numbers. This should not happen,
QEMU should rather refuse to activate the device in this case instead.
Thus propagate the error to the callers to handle it more gracefully
there.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/539
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/net/vmxnet3.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 8569484b2f2..1c5d1d740cc 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1336,32 +1336,46 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
         || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
 }
 
-static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
+static bool vmxnet3_validate_irq_idx(const char *type, bool is_msix, int idx)
 {
     int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
+
     if (idx >= max_ints) {
-        hw_error("Bad interrupt index: %d\n", idx);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "vmxnet3: Bad %s queue interrupt index: %d\n",
+                      type, idx);
+        return false;
     }
+
+    return true;
 }
 
-static void vmxnet3_validate_interrupts(VMXNET3State *s)
+static bool vmxnet3_validate_interrupts(VMXNET3State *s)
 {
     int i;
 
     VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
-    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
+    if (!vmxnet3_validate_irq_idx("event", s->msix_used, s->event_int_idx)) {
+        return false;
+    }
 
     for (i = 0; i < s->txq_num; i++) {
         int idx = s->txq_descr[i].intr_idx;
         VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        if (!vmxnet3_validate_irq_idx("TX", s->msix_used, idx)) {
+            return false;
+        }
     }
 
     for (i = 0; i < s->rxq_num; i++) {
         int idx = s->rxq_descr[i].intr_idx;
         VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
-        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
+        if (!vmxnet3_validate_irq_idx("RX", s->msix_used, idx)) {
+            return false;
+        }
     }
+
+    return true;
 }
 
 static bool vmxnet3_validate_queues(VMXNET3State *s)
@@ -1554,7 +1568,10 @@ static void vmxnet3_activate_device(VMXNET3State *s)
                sizeof(s->rxq_descr[i].rxq_stats));
     }
 
-    vmxnet3_validate_interrupts(s);
+    if (!vmxnet3_validate_interrupts(s)) {
+        vmxnet3_reset(s);
+        return;
+    }
 
     /* Make sure everything is in place before device activation */
     smp_wmb();
@@ -2392,7 +2409,9 @@ static int vmxnet3_post_load(void *opaque, int version_id)
     if (!vmxnet3_validate_queues(s)) {
         return -1;
     }
-    vmxnet3_validate_interrupts(s);
+    if (!vmxnet3_validate_interrupts(s)) {
+        return -1;
+    }
 
     return 0;
 }
-- 
2.55.0

Re: [PATCH] hw/net/vmxnet3: Do not abort if guest provides bad interrupt numbers

[Philippe Mathieu-Daudé]

Hi Thomas,

On 30/7/26 16:39, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> vmxnet3_validate_interrupts() currently aborts via hw_error() if
> the guest provided bad interrupt numbers. This should not happen,
> QEMU should rather refuse to activate the device in this case instead.
> Thus propagate the error to the callers to handle it more gracefully
> there.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/539
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/net/vmxnet3.c | 35 +++++++++++++++++++++++++++--------
>   1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 8569484b2f2..1c5d1d740cc 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1336,32 +1336,46 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, int intx)
>           || intx == pci_get_byte(s->parent_obj.config + PCI_INTERRUPT_PIN) - 1;
>   }
>   
> -static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
> +static bool vmxnet3_validate_irq_idx(const char *type, bool is_msix, int idx)
>   {
>       int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
> +
>       if (idx >= max_ints) {
> -        hw_error("Bad interrupt index: %d\n", idx);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "vmxnet3: Bad %s queue interrupt index: %d\n",
> +                      type, idx);
> +        return false;
>       }
> +
> +    return true;
>   }
>   
> -static void vmxnet3_validate_interrupts(VMXNET3State *s)
> +static bool vmxnet3_validate_interrupts(VMXNET3State *s)
>   {
>       int i;
>   
>       VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
> -    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
> +    if (!vmxnet3_validate_irq_idx("event", s->msix_used, s->event_int_idx)) {
> +        return false;
> +    }
>   
>       for (i = 0; i < s->txq_num; i++) {
>           int idx = s->txq_descr[i].intr_idx;
>           VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +        if (!vmxnet3_validate_irq_idx("TX", s->msix_used, idx)) {
> +            return false;
> +        }
>       }
>   
>       for (i = 0; i < s->rxq_num; i++) {
>           int idx = s->rxq_descr[i].intr_idx;
>           VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
> +        if (!vmxnet3_validate_irq_idx("RX", s->msix_used, idx)) {
> +            return false;
> +        }
>       }
> +
> +    return true;
>   }
>   
>   static bool vmxnet3_validate_queues(VMXNET3State *s)
> @@ -1554,7 +1568,10 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>                  sizeof(s->rxq_descr[i].rxq_stats));
>       }
>   
> -    vmxnet3_validate_interrupts(s);
> +    if (!vmxnet3_validate_interrupts(s)) {
> +        vmxnet3_reset(s);
> +        return;

We definitively want to return here to not set s->device_active,
but calling vmxnet3_reset() in the middle of a command handler is
surprising. Maybe call it in the caller?

-- >8 --
@@ -1554,7 +1554,9 @@ static void vmxnet3_activate_device(VMXNET3State *s)
                 sizeof(s->rxq_descr[i].rxq_stats));
      }

-    vmxnet3_validate_interrupts(s);
+    if (!vmxnet3_validate_interrupts(s)) {
+        return false;
+    }

      /* Make sure everything is in place before device activation */
      smp_wmb();
@@ -1584,7 +1586,9 @@ static void vmxnet3_handle_command(VMXNET3State 
*s, uint64_t cmd)

      case VMXNET3_CMD_ACTIVATE_DEV:
          VMW_CBPRN("Set: Activating vmxnet3 device");
-        vmxnet3_activate_device(s);
+        if (!vmxnet3_activate_device(s)) {
+            vmxnet3_reset(s);
+        }
          break;

      case VMXNET3_CMD_UPDATE_RX_MODE:
---

Regardless this patch fixes what it aims to, and -- while I don't
have much knowledge of this device -- it looks reasonable.

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

> +    }
>   
>       /* Make sure everything is in place before device activation */
>       smp_wmb();
> @@ -2392,7 +2409,9 @@ static int vmxnet3_post_load(void *opaque, int version_id)
>       if (!vmxnet3_validate_queues(s)) {
>           return -1;
>       }
> -    vmxnet3_validate_interrupts(s);
> +    if (!vmxnet3_validate_interrupts(s)) {
> +        return -1;
> +    }
>   
>       return 0;
>   }

Re: [PATCH] hw/net/vmxnet3: Do not abort if guest provides bad interrupt numbers

[Thomas Huth]

On 31/07/2026 08.03, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 30/7/26 16:39, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> vmxnet3_validate_interrupts() currently aborts via hw_error() if
>> the guest provided bad interrupt numbers. This should not happen,
>> QEMU should rather refuse to activate the device in this case instead.
>> Thus propagate the error to the callers to handle it more gracefully
>> there.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/539
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   hw/net/vmxnet3.c | 35 +++++++++++++++++++++++++++--------
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
>> index 8569484b2f2..1c5d1d740cc 100644
>> --- a/hw/net/vmxnet3.c
>> +++ b/hw/net/vmxnet3.c
>> @@ -1336,32 +1336,46 @@ static bool vmxnet3_verify_intx(VMXNET3State *s, 
>> int intx)
>>           || intx == pci_get_byte(s->parent_obj.config + 
>> PCI_INTERRUPT_PIN) - 1;
>>   }
>> -static void vmxnet3_validate_interrupt_idx(bool is_msix, int idx)
>> +static bool vmxnet3_validate_irq_idx(const char *type, bool is_msix, int 
>> idx)
>>   {
>>       int max_ints = is_msix ? VMXNET3_MAX_INTRS : VMXNET3_MAX_NMSIX_INTRS;
>> +
>>       if (idx >= max_ints) {
>> -        hw_error("Bad interrupt index: %d\n", idx);
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "vmxnet3: Bad %s queue interrupt index: %d\n",
>> +                      type, idx);
>> +        return false;
>>       }
>> +
>> +    return true;
>>   }
>> -static void vmxnet3_validate_interrupts(VMXNET3State *s)
>> +static bool vmxnet3_validate_interrupts(VMXNET3State *s)
>>   {
>>       int i;
>>       VMW_CFPRN("Verifying event interrupt index (%d)", s->event_int_idx);
>> -    vmxnet3_validate_interrupt_idx(s->msix_used, s->event_int_idx);
>> +    if (!vmxnet3_validate_irq_idx("event", s->msix_used, s- 
>> >event_int_idx)) {
>> +        return false;
>> +    }
>>       for (i = 0; i < s->txq_num; i++) {
>>           int idx = s->txq_descr[i].intr_idx;
>>           VMW_CFPRN("Verifying TX queue %d interrupt index (%d)", i, idx);
>> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +        if (!vmxnet3_validate_irq_idx("TX", s->msix_used, idx)) {
>> +            return false;
>> +        }
>>       }
>>       for (i = 0; i < s->rxq_num; i++) {
>>           int idx = s->rxq_descr[i].intr_idx;
>>           VMW_CFPRN("Verifying RX queue %d interrupt index (%d)", i, idx);
>> -        vmxnet3_validate_interrupt_idx(s->msix_used, idx);
>> +        if (!vmxnet3_validate_irq_idx("RX", s->msix_used, idx)) {
>> +            return false;
>> +        }
>>       }
>> +
>> +    return true;
>>   }
>>   static bool vmxnet3_validate_queues(VMXNET3State *s)
>> @@ -1554,7 +1568,10 @@ static void vmxnet3_activate_device(VMXNET3State *s)
>>                  sizeof(s->rxq_descr[i].rxq_stats));
>>       }
>> -    vmxnet3_validate_interrupts(s);
>> +    if (!vmxnet3_validate_interrupts(s)) {
>> +        vmxnet3_reset(s);
>> +        return;
> 
> We definitively want to return here to not set s->device_active,
> but calling vmxnet3_reset() in the middle of a command handler is
> surprising. Maybe call it in the caller?
I've added it here as precautionary measure, but looking at the code flow 
again, it's likely not really necessary here: vmxnet3_deactivate_device() 
does not do anything at all since device_active has not been set yet. And 
all the other early returns in vmxnet3_activate_device() also don't do this.

So let's simply drop that vmxnet3_reset() from the patch. I'll send a v2...

  Thomas