| Message ID | 20241107070415.694662-2-frolov@swemel.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386: fix NULL-dereference | expand |
+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
Am 7. November 2024 07:04:10 UTC schrieb Dmitry Frolov <frolov@swemel.ru>: >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); >+ } Since pc_nic_init() is passed both an ISA and a PCI bus I think the NULL dereference should be fixed there. Moreover, if pc_nic_init() is only invoked when there is a PCI bus, the "isapc" machine won't have a nic at all. How is this patch related to https://patchew.org/QEMU/20241105171813.3031969-1-peter.maydell@linaro.org ? That is, do we need both patches to really fix the issue? Best regards, Bernhard > #ifdef CONFIG_IDE_ISA > if (!pcmc->pci_enabled) { > DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
Am 7. November 2024 09:19:05 UTC schrieb Bernhard Beschow <shentey@gmail.com>: > > >Am 7. November 2024 07:04:10 UTC schrieb Dmitry Frolov <frolov@swemel.ru>: >>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); >>+ } > >Since pc_nic_init() is passed both an ISA and a PCI bus I think the NULL dereference should be fixed there. Moreover, if pc_nic_init() is only invoked when there is a PCI bus, the "isapc" machine won't have a nic at all. > >How is this patch related to https://patchew.org/QEMU/20241105171813.3031969-1-peter.maydell@linaro.org ? That is, do we need both patches to really fix the issue? I've had a closer look at the code and Peter's patch nails the problem. Best regards, Bernhard > >Best regards, >Bernhard > >> #ifdef CONFIG_IDE_ISA >> if (!pcmc->pci_enabled) { >> DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
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 > > >
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];
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(-)