* [PATCH] hw/i386: fix NULL-dereference @ 2024-11-07 7:04 Dmitry Frolov 2024-11-07 9:04 ` Zhao Liu 2024-11-07 9:19 ` Bernhard Beschow 0 siblings, 2 replies; 5+ messages in thread From: Dmitry Frolov @ 2024-11-07 7:04 UTC (permalink / raw) To: pbonzini; +Cc: sdl.qemu, qemu-devel, Dmitry Frolov 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]; -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/i386: fix NULL-dereference 2024-11-07 7:04 [PATCH] hw/i386: fix NULL-dereference Dmitry Frolov @ 2024-11-07 9:04 ` Zhao Liu 2024-11-07 19:10 ` Bernhard Beschow 2024-11-07 9:19 ` Bernhard Beschow 1 sibling, 1 reply; 5+ messages in thread From: Zhao Liu @ 2024-11-07 9:04 UTC (permalink / raw) To: Dmitry Frolov; +Cc: pbonzini, sdl.qemu, qemu-devel, Philippe Mathieu-Daudé +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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/i386: fix NULL-dereference 2024-11-07 9:04 ` Zhao Liu @ 2024-11-07 19:10 ` Bernhard Beschow 0 siblings, 0 replies; 5+ messages in thread From: Bernhard Beschow @ 2024-11-07 19:10 UTC (permalink / raw) To: qemu-devel, Zhao Liu, Dmitry Frolov Cc: pbonzini, sdl.qemu, Philippe Mathieu-Daudé 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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/i386: fix NULL-dereference 2024-11-07 7:04 [PATCH] hw/i386: fix NULL-dereference Dmitry Frolov 2024-11-07 9:04 ` Zhao Liu @ 2024-11-07 9:19 ` Bernhard Beschow 2024-11-07 19:09 ` Bernhard Beschow 1 sibling, 1 reply; 5+ messages in thread From: Bernhard Beschow @ 2024-11-07 9:19 UTC (permalink / raw) To: qemu-devel, Dmitry Frolov, pbonzini, Peter Maydell; +Cc: sdl.qemu 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]; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/i386: fix NULL-dereference 2024-11-07 9:19 ` Bernhard Beschow @ 2024-11-07 19:09 ` Bernhard Beschow 0 siblings, 0 replies; 5+ messages in thread From: Bernhard Beschow @ 2024-11-07 19:09 UTC (permalink / raw) To: qemu-devel, Dmitry Frolov, pbonzini, Peter Maydell; +Cc: sdl.qemu 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]; ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-11-07 19:21 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-07 7:04 [PATCH] hw/i386: fix NULL-dereference Dmitry Frolov 2024-11-07 9:04 ` Zhao Liu 2024-11-07 19:10 ` Bernhard Beschow 2024-11-07 9:19 ` Bernhard Beschow 2024-11-07 19:09 ` Bernhard Beschow
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).