Re: [Qemu-devel] [PATCH] fix loading tiny kernels

[Daniel P. Berrange]

On Tue, Feb 03, 2009 at 12:09:42PM +0100, Alexander Graf wrote:
> 
> On 03.02.2009, at 11:30, Daniel P. Berrange wrote:
> 
> >On Tue, Feb 03, 2009 at 10:06:10AM +0100, Ren? Rebe wrote:
> >>I babbled:
> >>>Further testing / polishing the multi-boot kernel loading support  
> >>>I found
> >>>the existing code fails to load unusual small kernels, less than  
> >>>8192
> >>>bytes -
> >>>for example the example multi-boot kernel shipped within GRUB that
> >>>compiles to just 7121 bytes on my system.
> >>>
> >>>Signed-off-by: René Rebe <address@hidden>
> >>>
> >>>--- hw/pc.c     (revision 6501)
> >>>+++ hw/pc.c     (working copy)
> >>>@@ -554,7 +989,7 @@
> >>>  /* load the kernel header */
> >>>  f = fopen(kernel_filename, "rb");
> >>>  if (!f || !(kernel_size = get_file_size(f)) ||
> >>>-    fread(header, 1, 1024, f) != 1024) {
> >>>+    fread(header, 1, MIN(8192, kernel_size), f) != MIN(8192,
> >>>kernel_size)) {
> >>>  fprintf(stderr, "qemu: could not load kernel '%s'\n",
> >>>      kernel_filename);
> >>>  exit(1);
> >>>
> >>Ah,  sorry - mix in the series. This only applies to the multi-boot  
> >>series
> >>which increases the header read to 8192 bytes.
> >
> >Regardless, this code should not hardcode the size like this. It  
> >should
> >use sizeof(header) instead of 1024 or 8192, thus avoiding the  
> >potential
> >bug.
> 
> You don't really know sizeof(header), do you? Header could be the  
> Linux header or the Multiboot header which is by definition allowed to  
> sit somewhere within the first 8192 bytes.

I meant in terms of making sure we didn't overflow the header variable
which is allocated on the stack. So instead of

    uint8_t header[1024];
    ...
    fread(header, 1, 1024, f);

You'd have

    uint8_t header[1024];
    ...
    fread(header, 1, sizeof(header), f);

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|