| Message ID | 20220107105049.961489-1-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] q800: fix segfault with invalid MacROM | expand |
On 07/01/2022 11.50, Laurent Vivier wrote: > "qemu-system-m68k -M q800 -bios /dev/null" crashes with a segfault > in q800_init(). > This happens because the code doesn't check that rom_ptr() returned > a non-NULL pointer . > > To avoid NULL pointer, don't allow 0 sized file and use bios_size with > rom_ptr(). > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/m68k/q800.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index e4c7c9b88ad0..55dfe5036f40 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -672,12 +672,13 @@ static void q800_init(MachineState *machine) > > /* Remove qtest_enabled() check once firmware files are in the tree */ > if (!qtest_enabled()) { > - if (bios_size < 0 || bios_size > MACROM_SIZE) { > + if (bios_size <= 0 || bios_size > MACROM_SIZE) { > error_report("could not load MacROM '%s'", bios_name); > exit(1); > } > > - ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE); > + ptr = rom_ptr(MACROM_ADDR, bios_size); > + assert(ptr != NULL); > stl_phys(cs->as, 0, ldl_p(ptr)); /* reset initial SP */ > stl_phys(cs->as, 4, > MACROM_ADDR + ldl_p(ptr + 4)); /* reset initial PC */ Looks nicer than v1, indeed. Reviewed-by: Thomas Huth <thuth@redhat.com>
On 07/01/2022 10:50, Laurent Vivier wrote: > "qemu-system-m68k -M q800 -bios /dev/null" crashes with a segfault > in q800_init(). > This happens because the code doesn't check that rom_ptr() returned > a non-NULL pointer . > > To avoid NULL pointer, don't allow 0 sized file and use bios_size with > rom_ptr(). > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > hw/m68k/q800.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c > index e4c7c9b88ad0..55dfe5036f40 100644 > --- a/hw/m68k/q800.c > +++ b/hw/m68k/q800.c > @@ -672,12 +672,13 @@ static void q800_init(MachineState *machine) > > /* Remove qtest_enabled() check once firmware files are in the tree */ > if (!qtest_enabled()) { > - if (bios_size < 0 || bios_size > MACROM_SIZE) { > + if (bios_size <= 0 || bios_size > MACROM_SIZE) { > error_report("could not load MacROM '%s'", bios_name); > exit(1); > } > > - ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE); > + ptr = rom_ptr(MACROM_ADDR, bios_size); > + assert(ptr != NULL); > stl_phys(cs->as, 0, ldl_p(ptr)); /* reset initial SP */ > stl_phys(cs->as, 4, > MACROM_ADDR + ldl_p(ptr + 4)); /* reset initial PC */ I think technically the second parameter of rom_ptr() is supposed to be the absolute upper bound of the accessible region so the existing MACROM_SIZE feels better being a constant rather than a variable such as bios_size. But then again bios_size should never be greater than MACROM_SIZE because of the check beforehand so it shouldn't matter in practice. Anyhow regardless of what you decide: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c index e4c7c9b88ad0..55dfe5036f40 100644 --- a/hw/m68k/q800.c +++ b/hw/m68k/q800.c @@ -672,12 +672,13 @@ static void q800_init(MachineState *machine) /* Remove qtest_enabled() check once firmware files are in the tree */ if (!qtest_enabled()) { - if (bios_size < 0 || bios_size > MACROM_SIZE) { + if (bios_size <= 0 || bios_size > MACROM_SIZE) { error_report("could not load MacROM '%s'", bios_name); exit(1); } - ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE); + ptr = rom_ptr(MACROM_ADDR, bios_size); + assert(ptr != NULL); stl_phys(cs->as, 0, ldl_p(ptr)); /* reset initial SP */ stl_phys(cs->as, 4, MACROM_ADDR + ldl_p(ptr + 4)); /* reset initial PC */
"qemu-system-m68k -M q800 -bios /dev/null" crashes with a segfault in q800_init(). This happens because the code doesn't check that rom_ptr() returned a non-NULL pointer . To avoid NULL pointer, don't allow 0 sized file and use bios_size with rom_ptr(). Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756 Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Laurent Vivier <laurent@vivier.eu> --- hw/m68k/q800.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)