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

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

[René Rebe]

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 <rene@exactcode.de>

--- 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);

-- 
  René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
  http://exactcode.de | http://t2-project.org | http://rene.rebe.name

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

[Laurent Desnogues]

On Tue, Feb 3, 2009 at 9:59 AM, René Rebe <rene@exactcode.de> wrote:
> 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 <rene@exactcode.de>
>
> --- 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))

That's wrong:  'header' size is 1024 bytes.


Laurent

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

[René Rebe]

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 <rene@exactcode.de>
>
> --- 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.

Sorry, will include that in the updated multi-boot patch.

-- 
  René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
  http://exactcode.de | http://t2-project.org | http://rene.rebe.name

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

[Daniel P. Berrange]

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 <rene@exactcode.de>
> >
> >--- 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.

Regards,
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 :|

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

[Alexander Graf]

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 <rene@exactcode.de>
>>>
>>> --- 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.

Alex

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

[Rene Rebe]

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 <rene@exactcode.de>
>>>>
>>>> --- 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.

Maybe he ment just sizeof(header) to avoid letting future changes
of the code let the definition and code get out of sync if the
header size to be read is changed again.

-- 
   René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
   http://exactcode.de | http://t2-project.org | http://rene.rebe.name

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

[Alexander Graf]

On 03.02.2009, at 13:31, Rene Rebe wrote:

> 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 <rene@exactcode.de>
>>>>>
>>>>> --- 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.
>
> Maybe he ment just sizeof(header) to avoid letting future changes
> of the code let the definition and code get out of sync if the
> header size to be read is changed again.

Ah, right. Sorry for the fuss - sounds good.

Alex

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 <rene@exactcode.de>
> >>>
> >>>--- 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 :|

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

[Rene Rebe]

Daniel P. Berrange wrote:
> 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 <rene@exactcode.de>
>>>>>
>>>>> --- 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

Just preventing this in the case it's changed in the future and
one place is forgotten.

I already changed the code to use the ARRAY_SIZE macro in my
working copy:

   http://svn.exactcode.de/t2/trunk/package/emulators/kvm/09-qemu-multiboot.patch

-- 
   René Rebe - ExactCODE GmbH - Europe, Germany, Berlin
   http://exactcode.de | http://t2-project.org | http://rene.rebe.name