Re: [Qemu-devel] [PATCH 2/3] multiboot: Fix offset of bootloader name

[Max Reitz]

On 2015-01-15 at 06:26, Kevin Wolf wrote:
> This fixes a bug introduced in commit 5eba5a66 ('Add bootloader name to
> multiboot implementation').
>
> The calculation of the bootloader name offset didn't consider space
> occupied by module command lines, so some unlucky module got its command
> line partially overwritten with a "qemu" string.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   hw/i386/multiboot.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index f86d351..1adbe9e 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -156,6 +156,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>       MultibootState mbs;
>       uint8_t bootinfo[MBI_SIZE];
>       uint8_t *mb_bootinfo_data;
> +    uint32_t cmdline_len;
>   
>       /* Ok, let's see if it is a multiboot image.
>          The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> @@ -258,27 +259,28 @@ int load_multiboot(FWCfgState *fw_cfg,
>       mbs.offset_mbinfo = mbs.mb_buf_size;
>   
>       /* Calculate space for cmdlines, bootloader name, and mb_mods */
> -    mbs.mb_buf_size += strlen(kernel_filename) + 1;
> -    mbs.mb_buf_size += strlen(kernel_cmdline) + 1;
> -    mbs.mb_buf_size += strlen(bootloader_name) + 1;
> +    cmdline_len = strlen(kernel_filename) + 1;
> +    cmdline_len += strlen(kernel_cmdline) + 1;
>       if (initrd_filename) {
>           const char *r = initrd_filename;
> -        mbs.mb_buf_size += strlen(r) + 1;
> +        cmdline_len += strlen(r) + 1;
>           mbs.mb_mods_avail = 1;
>           while (*(r = get_opt_value(NULL, 0, r))) {
>              mbs.mb_mods_avail++;
>              r++;
>           }
> -        mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
>       }
>   
> +    mbs.mb_buf_size += cmdline_len;
> +    mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
> +    mbs.mb_buf_size += strlen(bootloader_name) + 1;

I would have swapped these two lines to reflect their order in mb_buf, 
but it's fine either way.

> +
>       mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
>   
>       /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
>       mbs.mb_buf            = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
>       mbs.offset_cmdlines   = mbs.offset_mbinfo + mbs.mb_mods_avail * 
> MB_MOD_SIZE;
> -    mbs.offset_bootloader = mbs.offset_cmdlines + strlen(kernel_filename) + 1
> -                            + strlen(kernel_cmdline) + 1;
> +    mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
>   
>       if (initrd_filename) {
>           char *next_initrd, not_last;

Reviewed-by: Max Reitz <address@hidden>