Re: [Qemu-devel] [PATCH 1/5] multiboot: Reject kernels exceeding the address space

[Jack Schwartz]

On 03/15/18 08:54, Kevin Wolf wrote:
> Am 15.03.2018 um 06:19 hat Jack Schwartz geschrieben:
> > Hi Kevin.
> >
> > My comments are inline...
> >
> > On 2018-03-14 10:32, Kevin Wolf wrote:
> > > The code path with a manually set mh_load_addr in the Multiboot header
> > > checks that load_end_addr <= load_addr, but the path where load_end_addr
> > > is automatically detected if 0 is given in the header misses the
> > > corresponding check.
> > 1) The code checks that load_end_addr >= load_addr (before letting it
> > through).
> >
> > 2) load_end_addr is used only when it is read in as non-zero, so no check is
> > needed if zero.  (It gets debug-printed even when zero, but is used only to
> > calculate mb_load_size and only when non-zero.)
> Oops, good point. I'll change the start of the commit message as follows:
>
>      The code path with a manually set mh_load_end_addr in the Multiboot
>      header checks that mh_load_end_addr >= mh_load_addr, but the path where
>      mh_load_end_addr is 0 in the header and therefore automatically
>      calculated from the file size misses the corresponding check.
>
> Does this look better?

mb_load_size is calculated from the file size, not mh_load_end_addr, so
I think you mean mb_load_size rather than mh_load_end_addr.  Do you 
intend to say:

  The code path where mh_load_end_addr is non-zero in the Multiboot
  header checks that mh_load_end_addr >= mh_load_addr and so
  mb_load_size ischecked.  However, mb_load_size is not checked when
  calculated from thefile size, when mh_load_end_addr is 0.

Also, if this is what you intend to say, would the following code change 
be more ofwhat you want:

Remove this:

            mb_load_size = kernel_file_size - mb_kernel_text_offset;
        }
-       if (mb_load_size > UINT32_MAX - mh_load_addr) {
-           error_report("kernel does not fit in address space");
-           exit(1);
-       }
        if (mh_bss_end_addr) {

and instead do this a few lines further down:

           mb_kernel_size = mh_bss_end_addr - mh_load_addr;
        } else {
            mb_kernel_size = mb_load_size;
        }

+       if (mb_kernel_size > UINT32_MAX - mh_load_addr) {
+           error_report("kernel does not fit in address space");
+           exit(1);
+       }

        mb_debug("multiboot: header_addr = %#x", mh_header_addr);
        mb_debug("multiboot: load_addr = %#x", mh_load_addr);

The reason would be to include the bss area in the calculation, when
mh_bss_end_addr is non-zero.

    Thanks,
    Jack

> > > If the kernel binary size is larger than can fit in
> > > the address space after load_addr, we ended up with a kernel_size that
> > > is smaller than load_size, which means that we read the file into a too
> > > small buffer.
> > >
> > > Add a check to reject kernel files with such Multiboot headers.
> > Code itself looks fine.
> >
> > Modulo above comments:
> >      Reviewed-by: Jack Schwartz <address@hidden>
> Thanks for your review of the series!
>
> Kevin