Re: [PATCH] q800: fix segfault with invalid MacROM

[Laurent Vivier]

Le 07/01/2022 à 10:47, BALATON Zoltan a écrit :
> On Fri, 7 Jan 2022, Laurent Vivier wrote:
> > Le 07/01/2022 à 09:15, Mark Cave-Ayland a écrit :
> > > On 06/01/2022 12:22, Laurent Vivier wrote:
> > >
> > > > "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
> > > > in q800_init().
> > > > This happens because the code doesn't check that rom_ptr() returned
> > > > a non-NULL pointer .
> > > >
> > > > 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 | 8 +++++++-
> > > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> > > > index e4c7c9b88ad0..6261716c8f7e 100644
> > > > --- a/hw/m68k/q800.c
> > > > +++ b/hw/m68k/q800.c
> > > > @@ -672,10 +672,16 @@ 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 == -1) {
> > > >                   error_report("could not load MacROM '%s'", bios_name);
> > > >                   exit(1);
> > > >               }
> > > > +            if (bios_size != MACROM_SIZE) {
> > > > +                error_report("Invalid size for MacROM '%s': %d bytes,"
> > > > +                             " expected %d bytes", bios_name, bios_size,
> > > > +                             MACROM_SIZE);
> > > > +                exit(1);
> > > > +            }
> > > >               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
> > > >               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
> > >
> > > The patch does fix the issue, but it seems a little odd that you can't use -bios 
> > > path/to/m68k-binary to boot with an arbitrary sized binary which could be useful for reproducers 
> > > such as https://gitlab.com/qemu-project/qemu/-/issues/360.
> > >
> > > How easy would it be to add the extra rom_ptr() NULL check instead?
> >
> > I was thinking that a smaller binary can be padded to 1 MB for use because on a real hardware the 
> > size of the ROM cannot be arbitrary.
> >
> > But it seems reasonable to check only for the NULL pointer rather than the size, I'm going to send 
> > a v2.
>
> Instead of adding !rom_ptr as well, isn't it enough to change to
> bios_size <= 0 in the existing check?

I agree. And to change rom_ptr(MACROM_ADDR, MACROM_SIZE) to 
rom_ptr(MACROM_ADDR, bios_size)

Thanks,
Laurent