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

[Rene Rebe <rene@exactcode.de>]

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