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

["Philippe Mathieu-Daudé" <philmd@oss.qualcomm.com>]

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;
>   }