Re: [PATCH v2] q800: fix segfault with invalid MacROM
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
Signed-off-by: Laurent Vivier
---
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
ATB,
Mark.
Re: [PATCH v2] q800: fix segfault with invalid MacROM
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
Signed-off-by: Laurent Vivier
---
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
[PATCH v2] q800: fix segfault with invalid MacROM
"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
Signed-off-by: Laurent Vivier
---
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 */
--
2.33.1
