Re: [PATCH] hw/i386: fix NULL-dereference

[Bernhard Beschow]

Am 7. November 2024 09:04:16 UTC schrieb Zhao Liu <zhao1.liu@intel.com>:
>+Philippe for ISAPC
>
>On Thu, Nov 07, 2024 at 10:04:10AM +0300, Dmitry Frolov wrote:
>> Date: Thu,  7 Nov 2024 10:04:10 +0300
>> From: Dmitry Frolov <frolov@swemel.ru>
>> Subject: [PATCH] hw/i386: fix NULL-dereference
>> 
>> If pcmc->pci_enabled is false, pcms->pcibus is NULL and is passed
>> to pc_nic_init() where it is being dereferenced.
>> 
>> Found making check with enabled sanitizers.
>> 
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>> ---
>>  hw/i386/pc_piix.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 2bf6865d40..2a92d2dbb7 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -313,9 +313,9 @@ static void pc_init1(MachineState *machine, const char 
>> *pci_type)
>>      /* init basic PC hardware */
>>      pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
>>                           !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
>> -
>> -    pc_nic_init(pcmc, isa_bus, pcms->pcibus);
>> -
>> +    if (pcmc->pci_enabled) {
>> +        pc_nic_init(pcmc, isa_bus, pcms->pcibus);
>> +    }
>>  #ifdef CONFIG_IDE_ISA
>>      if (!pcmc->pci_enabled) {
>>          DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>
>In principle, I think the fix is right. Currently only ISAPC's
>pci_enabled is false.
>
>I think ISAPC shouldn't need nic, so it's safe. Is this right, Phil? :)

The isapc machine has an ne2k_isa nic by default. I'd rather preserve that 
behavior, especially as we have Peter's fix which does that.

Best regards,
Bernhard

>
>The potential issue lies in pci_init_nic_devices() with "&bus->qbus".
>Although "bus" (which is pcibus here) is NULL, the compiler seems to
>optimize this, making &bus->qbus also NULL. Therefore, I did not
>encounter any errors when attempting to start ISAPC.
>
>Thanks,
>Zhao
>
>
>