* [PATCH] pci: fix overflow in snprintf string formatting
@ 2022-05-31 11:47 Claudio Fontana
2022-05-31 12:26 ` Ani Sinha
2022-05-31 14:20 ` Michael S. Tsirkin
0 siblings, 2 replies; 5+ messages in thread
From: Claudio Fontana @ 2022-05-31 11:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, qemu-devel, Dario Faggioli, Martin Liška,
Peter Maydell, Paolo Bonzini, Claudio Fontana, Dario Faggioli,
qemu-stable
the code in pcibus_get_fw_dev_path contained the potential for a
stack buffer overflow of 1 byte, potentially writing to the stack an
extra NUL byte.
This overflow could happen if the PCI slot is >= 0x10000000,
and the PCI function is >= 0x10000000, due to the size parameter
of snprintf being incorrectly calculated in the call:
if (PCI_FUNC(d->devfn))
snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
since the off obtained from a previous call to snprintf is added
instead of subtracted from the total available size of the buffer.
Without the accurate size guard from snprintf, we end up writing in the
worst case:
name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes
In order to provide something more robust, replace all of the code in
pcibus_get_fw_dev_path with a single call to g_strdup_printf,
so there is no need to rely on manual calculations.
Found by compiling QEMU with FORTIFY_SOURCE=3 as the error:
*** buffer overflow detected ***: terminated
Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff642c380 (LWP 121307)]
0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6
(gdb) bt
#0 0x00007ffff71ff55c in __pthread_kill_implementation () at /lib64/libc.so.6
#1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6
#2 0x00007ffff7195814 in abort () at /lib64/libc.so.6
#3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6
#4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6
#5 0x00007ffff7295c36 in () at /lib64/libc.so.6
#6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6
#7 0x0000555555b1c1fd in pcibus_get_fw_dev_path ()
#8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop ()
#9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop ()
#10 0x00005555559a6e5d in get_boot_device_path ()
#11 0x00005555559a712c in get_boot_devices_list ()
#12 0x0000555555b1a3d0 in fw_cfg_machine_reset ()
#13 0x0000555555bf4c2d in pc_machine_reset ()
#14 0x0000555555c66988 in qemu_system_reset ()
#15 0x0000555555a6dff6 in qdev_machine_creation_done ()
#16 0x0000555555c79186 in qmp_x_exit_preconfig.part ()
#17 0x0000555555c7b459 in qemu_init ()
#18 0x0000555555960a29 in main ()
Found-by: Dario Faggioli <Dario Faggioli <dfaggioli@suse.com>
Found-by: Martin Liška <martin.liska@suse.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
hw/pci/pci.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a9b37f8000..6e7015329c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
static char *pcibus_get_fw_dev_path(DeviceState *dev)
{
PCIDevice *d = (PCIDevice *)dev;
- char path[50], name[33];
- int off;
-
- off = snprintf(path, sizeof(path), "%s@%x",
- pci_dev_fw_name(dev, name, sizeof name),
- PCI_SLOT(d->devfn));
- if (PCI_FUNC(d->devfn))
- snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
- return g_strdup(path);
+ char name[33];
+ int has_func = !!PCI_FUNC(d->devfn);
+
+ return g_strdup_printf("%s@%x%s%.*x",
+ pci_dev_fw_name(dev, name, sizeof(name)),
+ PCI_SLOT(d->devfn),
+ has_func ? "," : "",
+ has_func,
+ PCI_FUNC(d->devfn));
}
static char *pcibus_get_dev_path(DeviceState *dev)
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: fix overflow in snprintf string formatting
2022-05-31 11:47 [PATCH] pci: fix overflow in snprintf string formatting Claudio Fontana
@ 2022-05-31 12:26 ` Ani Sinha
2022-05-31 12:45 ` Claudio Fontana
2022-05-31 14:20 ` Michael S. Tsirkin
1 sibling, 1 reply; 5+ messages in thread
From: Ani Sinha @ 2022-05-31 12:26 UTC (permalink / raw)
To: Claudio Fontana
Cc: Michael S. Tsirkin, Marcel Apfelbaum, qemu-devel, Dario Faggioli,
Martin Liška, Peter Maydell, Paolo Bonzini, Dario Faggioli,
qemu-stable
On Tue, May 31, 2022 at 5:20 PM Claudio Fontana <cfontana@suse.de> wrote:
>
> the code in pcibus_get_fw_dev_path contained the potential for a
> stack buffer overflow of 1 byte, potentially writing to the stack an
> extra NUL byte.
>
> This overflow could happen if the PCI slot is >= 0x10000000,
> and the PCI function is >= 0x10000000, due to the size parameter
> of snprintf being incorrectly calculated in the call:
>
> if (PCI_FUNC(d->devfn))
> snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>
> since the off obtained from a previous call to snprintf is added
> instead of subtracted from the total available size of the buffer.
>
> Without the accurate size guard from snprintf, we end up writing in the
> worst case:
>
> name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes
>
> In order to provide something more robust, replace all of the code in
> pcibus_get_fw_dev_path with a single call to g_strdup_printf,
> so there is no need to rely on manual calculations.
>
> Found by compiling QEMU with FORTIFY_SOURCE=3 as the error:
>
> *** buffer overflow detected ***: terminated
>
> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7ffff642c380 (LWP 121307)]
> 0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6
> (gdb) bt
> #0 0x00007ffff71ff55c in __pthread_kill_implementation () at /lib64/libc.so.6
> #1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6
> #2 0x00007ffff7195814 in abort () at /lib64/libc.so.6
> #3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6
> #4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6
> #5 0x00007ffff7295c36 in () at /lib64/libc.so.6
> #6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6
> #7 0x0000555555b1c1fd in pcibus_get_fw_dev_path ()
> #8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop ()
> #9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop ()
> #10 0x00005555559a6e5d in get_boot_device_path ()
> #11 0x00005555559a712c in get_boot_devices_list ()
> #12 0x0000555555b1a3d0 in fw_cfg_machine_reset ()
> #13 0x0000555555bf4c2d in pc_machine_reset ()
> #14 0x0000555555c66988 in qemu_system_reset ()
> #15 0x0000555555a6dff6 in qdev_machine_creation_done ()
> #16 0x0000555555c79186 in qmp_x_exit_preconfig.part ()
> #17 0x0000555555c7b459 in qemu_init ()
> #18 0x0000555555960a29 in main ()
>
> Found-by: Dario Faggioli <Dario Faggioli <dfaggioli@suse.com>
> Found-by: Martin Liška <martin.liska@suse.com>
I think Reported-by: is preferred.
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
> hw/pci/pci.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a9b37f8000..6e7015329c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
> static char *pcibus_get_fw_dev_path(DeviceState *dev)
> {
> PCIDevice *d = (PCIDevice *)dev;
> - char path[50], name[33];
> - int off;
> -
> - off = snprintf(path, sizeof(path), "%s@%x",
> - pci_dev_fw_name(dev, name, sizeof name),
> - PCI_SLOT(d->devfn));
> - if (PCI_FUNC(d->devfn))
> - snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> - return g_strdup(path);
> + char name[33];
> + int has_func = !!PCI_FUNC(d->devfn);
> +
> + return g_strdup_printf("%s@%x%s%.*x",
I was experimenting with printf dynamic precision field with hex and
it was not quite working as expected. In particular, with precision 0,
I was still able to print a single hex digit. That is the following
still outputs 5 in stderr :
> fprintf(stderr, "%.*x\n", 0, 5);
> + pci_dev_fw_name(dev, name, sizeof(name)),
> + PCI_SLOT(d->devfn),
> + has_func ? "," : "",
> + has_func,
> + PCI_FUNC(d->devfn));
> }
>
> static char *pcibus_get_dev_path(DeviceState *dev)
> --
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: fix overflow in snprintf string formatting
2022-05-31 12:26 ` Ani Sinha
@ 2022-05-31 12:45 ` Claudio Fontana
2022-05-31 13:09 ` Ani Sinha
0 siblings, 1 reply; 5+ messages in thread
From: Claudio Fontana @ 2022-05-31 12:45 UTC (permalink / raw)
To: Ani Sinha
Cc: Michael S. Tsirkin, Marcel Apfelbaum, qemu-devel, Dario Faggioli,
Martin Liška, Peter Maydell, Paolo Bonzini, Dario Faggioli,
qemu-stable
On 5/31/22 14:26, Ani Sinha wrote:
> On Tue, May 31, 2022 at 5:20 PM Claudio Fontana <cfontana@suse.de> wrote:
>>
>> the code in pcibus_get_fw_dev_path contained the potential for a
>> stack buffer overflow of 1 byte, potentially writing to the stack an
>> extra NUL byte.
>>
>> This overflow could happen if the PCI slot is >= 0x10000000,
>> and the PCI function is >= 0x10000000, due to the size parameter
>> of snprintf being incorrectly calculated in the call:
>>
>> if (PCI_FUNC(d->devfn))
>> snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>>
>> since the off obtained from a previous call to snprintf is added
>> instead of subtracted from the total available size of the buffer.
>>
>> Without the accurate size guard from snprintf, we end up writing in the
>> worst case:
>>
>> name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes
>>
>> In order to provide something more robust, replace all of the code in
>> pcibus_get_fw_dev_path with a single call to g_strdup_printf,
>> so there is no need to rely on manual calculations.
>>
>> Found by compiling QEMU with FORTIFY_SOURCE=3 as the error:
>>
>> *** buffer overflow detected ***: terminated
>>
>> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
>> [Switching to Thread 0x7ffff642c380 (LWP 121307)]
>> 0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6
>> (gdb) bt
>> #0 0x00007ffff71ff55c in __pthread_kill_implementation () at /lib64/libc.so.6
>> #1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6
>> #2 0x00007ffff7195814 in abort () at /lib64/libc.so.6
>> #3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6
>> #4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6
>> #5 0x00007ffff7295c36 in () at /lib64/libc.so.6
>> #6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6
>> #7 0x0000555555b1c1fd in pcibus_get_fw_dev_path ()
>> #8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop ()
>> #9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop ()
>> #10 0x00005555559a6e5d in get_boot_device_path ()
>> #11 0x00005555559a712c in get_boot_devices_list ()
>> #12 0x0000555555b1a3d0 in fw_cfg_machine_reset ()
>> #13 0x0000555555bf4c2d in pc_machine_reset ()
>> #14 0x0000555555c66988 in qemu_system_reset ()
>> #15 0x0000555555a6dff6 in qdev_machine_creation_done ()
>> #16 0x0000555555c79186 in qmp_x_exit_preconfig.part ()
>> #17 0x0000555555c7b459 in qemu_init ()
>> #18 0x0000555555960a29 in main ()
>>
>> Found-by: Dario Faggioli <Dario Faggioli <dfaggioli@suse.com>
>> Found-by: Martin Liška <martin.liska@suse.com>
>
> I think Reported-by: is preferred.
>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>> hw/pci/pci.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index a9b37f8000..6e7015329c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
>> static char *pcibus_get_fw_dev_path(DeviceState *dev)
>> {
>> PCIDevice *d = (PCIDevice *)dev;
>> - char path[50], name[33];
>> - int off;
>> -
>> - off = snprintf(path, sizeof(path), "%s@%x",
>> - pci_dev_fw_name(dev, name, sizeof name),
>> - PCI_SLOT(d->devfn));
>> - if (PCI_FUNC(d->devfn))
>> - snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>> - return g_strdup(path);
>> + char name[33];
>> + int has_func = !!PCI_FUNC(d->devfn);
>> +
>> + return g_strdup_printf("%s@%x%s%.*x",
>
> I was experimenting with printf dynamic precision field with hex and
> it was not quite working as expected. In particular, with precision 0,
> I was still able to print a single hex digit. That is the following
> still outputs 5 in stderr :
>
>> fprintf(stderr, "%.*x\n", 0, 5);
Hi Ani, both the precision and the value need to be zero to omit the value,
and this is what the patch relies on.
Ciao,
Claudio
>
>
>> + pci_dev_fw_name(dev, name, sizeof(name)),
>> + PCI_SLOT(d->devfn),
>> + has_func ? "," : "",
>> + has_func,
>> + PCI_FUNC(d->devfn));
>> }
>>
>> static char *pcibus_get_dev_path(DeviceState *dev)
>> --
>> 2.26.2
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: fix overflow in snprintf string formatting
2022-05-31 12:45 ` Claudio Fontana
@ 2022-05-31 13:09 ` Ani Sinha
0 siblings, 0 replies; 5+ messages in thread
From: Ani Sinha @ 2022-05-31 13:09 UTC (permalink / raw)
To: Claudio Fontana
Cc: Michael S. Tsirkin, Marcel Apfelbaum, qemu-devel, Dario Faggioli,
Martin Liška, Peter Maydell, Paolo Bonzini, Dario Faggioli,
qemu-stable
On Tue, May 31, 2022 at 6:15 PM Claudio Fontana <cfontana@suse.de> wrote:
>
> On 5/31/22 14:26, Ani Sinha wrote:
> > On Tue, May 31, 2022 at 5:20 PM Claudio Fontana <cfontana@suse.de> wrote:
> >>
> >> the code in pcibus_get_fw_dev_path contained the potential for a
> >> stack buffer overflow of 1 byte, potentially writing to the stack an
> >> extra NUL byte.
> >>
> >> This overflow could happen if the PCI slot is >= 0x10000000,
> >> and the PCI function is >= 0x10000000, due to the size parameter
> >> of snprintf being incorrectly calculated in the call:
> >>
> >> if (PCI_FUNC(d->devfn))
> >> snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> >>
> >> since the off obtained from a previous call to snprintf is added
> >> instead of subtracted from the total available size of the buffer.
> >>
> >> Without the accurate size guard from snprintf, we end up writing in the
> >> worst case:
> >>
> >> name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes
> >>
> >> In order to provide something more robust, replace all of the code in
> >> pcibus_get_fw_dev_path with a single call to g_strdup_printf,
> >> so there is no need to rely on manual calculations.
> >>
> >> Found by compiling QEMU with FORTIFY_SOURCE=3 as the error:
> >>
> >> *** buffer overflow detected ***: terminated
> >>
> >> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> >> [Switching to Thread 0x7ffff642c380 (LWP 121307)]
> >> 0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6
> >> (gdb) bt
> >> #0 0x00007ffff71ff55c in __pthread_kill_implementation () at /lib64/libc.so.6
> >> #1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6
> >> #2 0x00007ffff7195814 in abort () at /lib64/libc.so.6
> >> #3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6
> >> #4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6
> >> #5 0x00007ffff7295c36 in () at /lib64/libc.so.6
> >> #6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6
> >> #7 0x0000555555b1c1fd in pcibus_get_fw_dev_path ()
> >> #8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop ()
> >> #9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop ()
> >> #10 0x00005555559a6e5d in get_boot_device_path ()
> >> #11 0x00005555559a712c in get_boot_devices_list ()
> >> #12 0x0000555555b1a3d0 in fw_cfg_machine_reset ()
> >> #13 0x0000555555bf4c2d in pc_machine_reset ()
> >> #14 0x0000555555c66988 in qemu_system_reset ()
> >> #15 0x0000555555a6dff6 in qdev_machine_creation_done ()
> >> #16 0x0000555555c79186 in qmp_x_exit_preconfig.part ()
> >> #17 0x0000555555c7b459 in qemu_init ()
> >> #18 0x0000555555960a29 in main ()
> >>
> >> Found-by: Dario Faggioli <Dario Faggioli <dfaggioli@suse.com>
> >> Found-by: Martin Liška <martin.liska@suse.com>
> >
> > I think Reported-by: is preferred.
> >
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >> ---
> >> hw/pci/pci.c | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index a9b37f8000..6e7015329c 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
> >> static char *pcibus_get_fw_dev_path(DeviceState *dev)
> >> {
> >> PCIDevice *d = (PCIDevice *)dev;
> >> - char path[50], name[33];
> >> - int off;
> >> -
> >> - off = snprintf(path, sizeof(path), "%s@%x",
> >> - pci_dev_fw_name(dev, name, sizeof name),
> >> - PCI_SLOT(d->devfn));
> >> - if (PCI_FUNC(d->devfn))
> >> - snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> >> - return g_strdup(path);
> >> + char name[33];
> >> + int has_func = !!PCI_FUNC(d->devfn);
> >> +
> >> + return g_strdup_printf("%s@%x%s%.*x",
> >
> > I was experimenting with printf dynamic precision field with hex and
> > it was not quite working as expected. In particular, with precision 0,
> > I was still able to print a single hex digit. That is the following
> > still outputs 5 in stderr :
> >
> >> fprintf(stderr, "%.*x\n", 0, 5);
>
> Hi Ani, both the precision and the value need to be zero to omit the value,
> and this is what the patch relies on.
Ah ok. s/5/0 in the above fprintf does not indeed print anything.
>
> Ciao,
>
> Claudio
>
> >
> >
> >> + pci_dev_fw_name(dev, name, sizeof(name)),
> >> + PCI_SLOT(d->devfn),
> >> + has_func ? "," : "",
> >> + has_func,
> >> + PCI_FUNC(d->devfn));
> >> }
> >>
> >> static char *pcibus_get_dev_path(DeviceState *dev)
> >> --
> >> 2.26.2
> >>
> >>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pci: fix overflow in snprintf string formatting
2022-05-31 11:47 [PATCH] pci: fix overflow in snprintf string formatting Claudio Fontana
2022-05-31 12:26 ` Ani Sinha
@ 2022-05-31 14:20 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-05-31 14:20 UTC (permalink / raw)
To: Claudio Fontana
Cc: Marcel Apfelbaum, qemu-devel, Dario Faggioli, Martin Liška,
Peter Maydell, Paolo Bonzini, Dario Faggioli, qemu-stable
On Tue, May 31, 2022 at 01:47:07PM +0200, Claudio Fontana wrote:
> the code in pcibus_get_fw_dev_path contained the potential for a
> stack buffer overflow of 1 byte, potentially writing to the stack an
> extra NUL byte.
>
> This overflow could happen if the PCI slot is >= 0x10000000,
> and the PCI function is >= 0x10000000, due to the size parameter
> of snprintf being incorrectly calculated in the call:
>
> if (PCI_FUNC(d->devfn))
> snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
>
> since the off obtained from a previous call to snprintf is added
> instead of subtracted from the total available size of the buffer.
>
> Without the accurate size guard from snprintf, we end up writing in the
> worst case:
>
> name (32) + "@" (1) + SLOT (8) + "," (1) + FUNC (8) + term NUL (1) = 51 bytes
>
> In order to provide something more robust, replace all of the code in
> pcibus_get_fw_dev_path with a single call to g_strdup_printf,
> so there is no need to rely on manual calculations.
>
> Found by compiling QEMU with FORTIFY_SOURCE=3 as the error:
>
> *** buffer overflow detected ***: terminated
>
> Thread 1 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7ffff642c380 (LWP 121307)]
> 0x00007ffff71ff55c in __pthread_kill_implementation () from /lib64/libc.so.6
> (gdb) bt
> #0 0x00007ffff71ff55c in __pthread_kill_implementation () at /lib64/libc.so.6
> #1 0x00007ffff71ac6f6 in raise () at /lib64/libc.so.6
> #2 0x00007ffff7195814 in abort () at /lib64/libc.so.6
> #3 0x00007ffff71f279e in __libc_message () at /lib64/libc.so.6
> #4 0x00007ffff729767a in __fortify_fail () at /lib64/libc.so.6
> #5 0x00007ffff7295c36 in () at /lib64/libc.so.6
> #6 0x00007ffff72957f5 in __snprintf_chk () at /lib64/libc.so.6
> #7 0x0000555555b1c1fd in pcibus_get_fw_dev_path ()
> #8 0x0000555555f2bde4 in qdev_get_fw_dev_path_helper.constprop ()
> #9 0x0000555555f2bd86 in qdev_get_fw_dev_path_helper.constprop ()
> #10 0x00005555559a6e5d in get_boot_device_path ()
> #11 0x00005555559a712c in get_boot_devices_list ()
> #12 0x0000555555b1a3d0 in fw_cfg_machine_reset ()
> #13 0x0000555555bf4c2d in pc_machine_reset ()
> #14 0x0000555555c66988 in qemu_system_reset ()
> #15 0x0000555555a6dff6 in qdev_machine_creation_done ()
> #16 0x0000555555c79186 in qmp_x_exit_preconfig.part ()
> #17 0x0000555555c7b459 in qemu_init ()
> #18 0x0000555555960a29 in main ()
>
> Found-by: Dario Faggioli <Dario Faggioli <dfaggioli@suse.com>
> Found-by: Martin Liška <martin.liska@suse.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
Queued, thanks!
> ---
> hw/pci/pci.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a9b37f8000..6e7015329c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2640,15 +2640,15 @@ static char *pci_dev_fw_name(DeviceState *dev, char *buf, int len)
> static char *pcibus_get_fw_dev_path(DeviceState *dev)
> {
> PCIDevice *d = (PCIDevice *)dev;
> - char path[50], name[33];
> - int off;
> -
> - off = snprintf(path, sizeof(path), "%s@%x",
> - pci_dev_fw_name(dev, name, sizeof name),
> - PCI_SLOT(d->devfn));
> - if (PCI_FUNC(d->devfn))
> - snprintf(path + off, sizeof(path) + off, ",%x", PCI_FUNC(d->devfn));
> - return g_strdup(path);
> + char name[33];
> + int has_func = !!PCI_FUNC(d->devfn);
> +
> + return g_strdup_printf("%s@%x%s%.*x",
> + pci_dev_fw_name(dev, name, sizeof(name)),
> + PCI_SLOT(d->devfn),
> + has_func ? "," : "",
> + has_func,
> + PCI_FUNC(d->devfn));
> }
>
> static char *pcibus_get_dev_path(DeviceState *dev)
> --
> 2.26.2
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-31 14:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 11:47 [PATCH] pci: fix overflow in snprintf string formatting Claudio Fontana
2022-05-31 12:26 ` Ani Sinha
2022-05-31 12:45 ` Claudio Fontana
2022-05-31 13:09 ` Ani Sinha
2022-05-31 14:20 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.