From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBnus-0003GL-HE for qemu-devel@nongnu.org; Thu, 15 Jan 2015 12:09:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBnun-0003kr-Ed for qemu-devel@nongnu.org; Thu, 15 Jan 2015 12:09:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60428) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBnun-0003ka-7l for qemu-devel@nongnu.org; Thu, 15 Jan 2015 12:09:01 -0500 Message-ID: <54B7F419.4060707@redhat.com> Date: Thu, 15 Jan 2015 12:08:41 -0500 From: Max Reitz MIME-Version: 1.0 References: <1421321204-17764-1-git-send-email-kwolf@redhat.com> <1421321204-17764-3-git-send-email-kwolf@redhat.com> In-Reply-To: <1421321204-17764-3-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] multiboot: Fix offset of bootloader name List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, sir@cmpwn.com, stefanha@redhat.com 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 > --- > 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