hw/i386: fix NULL-dereference

[PATCH] hw/i386: fix NULL-dereference

Posted by Dmitry Frolov 2 weeks, 2 days ago

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

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

Posted by Bernhard Beschow 2 weeks, 2 days ago


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];

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

Posted by Bernhard Beschow 2 weeks, 2 days ago


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];

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

Posted by Zhao Liu 2 weeks, 2 days ago

+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

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

Posted by Bernhard Beschow 2 weeks, 2 days ago


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