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

[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 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