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

On 31/07/2026 08.03, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 30/7/26 16:39, Thomas Huth wrote:
>> From: Thomas Huth <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>   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