Patchwork [v2] multiboot: Fix bss segment support

login
register
mail settings
Submitter Göran Weinholt
Date Dec. 20, 2011, 11:53 a.m.
Message ID <87pqfja28l.fsf@industria.weinholt.se>
Download mbox | patch
Permalink /patch/132405/
State New
Headers show

Comments

Göran Weinholt - Dec. 20, 2011, 11:53 a.m.
Alexander Graf <agraf@suse.de> writes:

> On 19.12.2011, at 23:01, Anthony Liguori wrote:
>
>     On 12/19/2011 11:35 AM, Alexander Graf wrote:
>
>         On 24.07.2011, at 17:55, Göran Weinholt wrote:
>
>             Multiboot images can specify a bss segment. The boot loader must
>             clear
>        
>             the memory of the bss and ensure that no modules or structures are
>        
>             allocated inside it. Several fields are provided in the Multiboot
>        
>             header that were previously not used properly. The header is now
>             used
>        
>             to determine how much data should be read from the image and how
>             much
>        
>             memory should be reserved to the bss segment.
>
>         This patch breaks the OSX booter:
>
>           http://people.exactcode.de/~rene/mac/boot
>
>     How is this licensed?  Is there source available?
>
> Yes, it's the XNU booter, but binaries are easier to debug when it comes to
> reading out the bootloader info:
>
>   http://tgwbd.org/darwin/boot.html
>
> I don't know if the binary above is 1:1 built from those sources, but it's
> definitely close enough.

I was unable to build that program from source, but I found a pre-built
binary that triggered the same bug. Please test the following patch.


Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero

There are two special cases in the address fields of the multiboot
format. If mh_load_end_addr is zero then the whole image file should
be loaded and if mh_bss_end_addr is zero then there is no bss segment.
With this change it is again possible to boot kernels where these
fields are zero.

Signed-off-by: Göran Weinholt <goran@weinholt.se>
---
 hw/multiboot.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
Alexander Graf - Dec. 20, 2011, 2:07 p.m.
On 20.12.2011, at 12:53, Göran Weinholt wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 19.12.2011, at 23:01, Anthony Liguori wrote:
>> 
>>    On 12/19/2011 11:35 AM, Alexander Graf wrote:
>> 
>>        On 24.07.2011, at 17:55, Göran Weinholt wrote:
>> 
>>            Multiboot images can specify a bss segment. The boot loader must
>>            clear
>> 
>>            the memory of the bss and ensure that no modules or structures are
>> 
>>            allocated inside it. Several fields are provided in the Multiboot
>> 
>>            header that were previously not used properly. The header is now
>>            used
>> 
>>            to determine how much data should be read from the image and how
>>            much
>> 
>>            memory should be reserved to the bss segment.
>> 
>>        This patch breaks the OSX booter:
>> 
>>          http://people.exactcode.de/~rene/mac/boot
>> 
>>    How is this licensed?  Is there source available?
>> 
>> Yes, it's the XNU booter, but binaries are easier to debug when it comes to
>> reading out the bootloader info:
>> 
>>  http://tgwbd.org/darwin/boot.html
>> 
>> I don't know if the binary above is 1:1 built from those sources, but it's
>> definitely close enough.
> 
> I was unable to build that program from source, but I found a pre-built
> binary that triggered the same bug. Please test the following patch.
> 
> 
> Subject: [PATCH] multiboot: mh_load_end_addr and mh_bss_end_addr may be zero
> 
> There are two special cases in the address fields of the multiboot
> format. If mh_load_end_addr is zero then the whole image file should
> be loaded and if mh_bss_end_addr is zero then there is no bss segment.
> With this change it is again possible to boot kernels where these
> fields are zero.
> 
> Signed-off-by: Göran Weinholt <goran@weinholt.se>

Yes, this patch makes things work again :). Thanks a lot!

The only thing I could nitpick on would be the coding style - checkpatch.pl complains :). Could you please resend with braces?
Justin, Please also queue this for 1.0-stable when it comes in its final form.

Tested-by: Alexander Graf <agraf@suse.de>


Alex

> ---
> hw/multiboot.c |   13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/multiboot.c b/hw/multiboot.c
> index b4484a3..a43d56c 100644
> --- a/hw/multiboot.c
> +++ b/hw/multiboot.c
> @@ -202,10 +202,21 @@ int load_multiboot(void *fw_cfg,
>         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
>         mh_load_addr = ldl_p(header+i+16);
>         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> -        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
> +        uint32_t mb_load_size;
> +
> +        /* A load end address of zero indicates that the whole file
> +         * should be loaded. */
> +        if (!mh_load_end_addr)
> +            mh_load_end_addr = kernel_file_size + mh_load_addr;
> +
> +        /* A bss end address of zero indicates that there is no bss
> +         * segment. */
> +        if (!mh_bss_end_addr)
> +            mh_bss_end_addr = mh_load_end_addr;

WARNING: braces {} are necessary for all arms of this statement
#83: FILE: hw/multiboot.c:209:
+        if (!mh_load_end_addr)
[...]

WARNING: braces {} are necessary for all arms of this statement
#88: FILE: hw/multiboot.c:214:
+        if (!mh_bss_end_addr)
[...]

Patch

diff --git a/hw/multiboot.c b/hw/multiboot.c
index b4484a3..a43d56c 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -202,10 +202,21 @@  int load_multiboot(void *fw_cfg,
         uint32_t mh_bss_end_addr = ldl_p(header+i+24);
         mh_load_addr = ldl_p(header+i+16);
         uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
-        uint32_t mb_load_size = mh_load_end_addr - mh_load_addr;
+        uint32_t mb_load_size;
+
+        /* A load end address of zero indicates that the whole file
+         * should be loaded. */
+        if (!mh_load_end_addr)
+            mh_load_end_addr = kernel_file_size + mh_load_addr;
+
+        /* A bss end address of zero indicates that there is no bss
+         * segment. */
+        if (!mh_bss_end_addr)
+            mh_bss_end_addr = mh_load_end_addr;
 
         mh_entry_addr = ldl_p(header+i+28);
         mb_kernel_size = mh_bss_end_addr - mh_load_addr;
+        mb_load_size = mh_load_end_addr - mh_load_addr;
 
         /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
         uint32_t mh_mode_type = ldl_p(header+i+32);