From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1fIaDx-0007Ra-3h for mharc-grub-devel@gnu.org; Tue, 15 May 2018 09:42:41 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fIaDu-0007PK-9L for grub-devel@gnu.org; Tue, 15 May 2018 09:42:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fIaDq-00037z-Ag for grub-devel@gnu.org; Tue, 15 May 2018 09:42:38 -0400 Received: from boksu.net-space.pl ([185.15.1.105]:54915) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1fIaDp-000364-Qx for grub-devel@gnu.org; Tue, 15 May 2018 09:42:34 -0400 Received: (from localhost user: 'dkiper' uid#4000 fake: STDIN (dkiper@boksu.net-space.pl)) by router-fw-old.local.net-space.pl id S1839590AbeEONm3 (ORCPT ); Tue, 15 May 2018 15:42:29 +0200 Date: Tue, 15 May 2018 15:42:29 +0200 From: Daniel Kiper To: alexander.boettcher@genode-labs.com Cc: grub-devel@gnu.org, dkiper@net-space.pl Subject: Re: [PATCH] mbi: use per segment a separate relocator chunk Message-ID: <20180515134229.GC16845@router-fw-old.local.net-space.pl> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.3.28i X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 185.15.1.105 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 May 2018 13:42:39 -0000 On Mon, May 14, 2018 at 09:02:00PM +0200, Alexander Boettcher wrote: > >From 8a0296d040a42950cd64e733f7997203975bc184 Mon Sep 17 00:00:00 2001 > From: Alexander Boettcher > Date: Fri, 13 Apr 2018 23:37:01 +0200 > Subject: [PATCH] mbi: use per segment a separate relocator chunk > > if elf is non relocatable. s/elf/ELF file/ > If the segments are not dense packed, the original code set up a huge > relocator chunk comprising all segments. > > During the final relocation in grub_relocator_prepare_relocs, the chunk > is moved to its desired place and overrides memory which are actually not > covered/touched by the specified segments. > > The overriden memory may contain device memory (vga text mode e.g.), which > leads to strange boot behaviour. I assume that a given ELF PHDR address/size does not cover VGA memory or anything like that, so, I am not sure what exactly overwrites this region. grub_memset() in current line 161 at some point? > Signed-off-by: Alexander Boettcher > --- > grub-core/loader/multiboot_elfxx.c | 57 +++++++++++++++++++++++--------------- > 1 file changed, 35 insertions(+), 22 deletions(-) > > diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c > index 67daf59..539c94a 100644 > --- a/grub-core/loader/multiboot_elfxx.c > +++ b/grub-core/loader/multiboot_elfxx.c > @@ -57,9 +57,9 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) > char *phdr_base; > grub_err_t err; > grub_relocator_chunk_t ch; > - grub_uint32_t load_offset, load_size; > + grub_uint32_t load_offset = 0, load_size; > int i; > - void *source; > + void *source = NULL; It seems to me that this change is not needed. I am thinking about "void *source = NULL;". > if (ehdr->e_ident[EI_MAG0] != ELFMAG0 > || ehdr->e_ident[EI_MAG1] != ELFMAG1 > @@ -83,6 +83,7 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) > #define phdr(i) ((Elf_Phdr *) (phdr_base + (i) * ehdr->e_phentsize)) > > mld->link_base_addr = ~0; > + mld->load_base_addr = ~0; > > /* Calculate lowest and highest load address. */ > for (i = 0; i < ehdr->e_phnum; i++) > @@ -97,10 +98,14 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) > return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border"); > #endif > > - load_size = highest_load - mld->link_base_addr; > + grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, " > + "relocatable=%d\n", mld->link_base_addr, > + mld->load_base_addr, mld->relocatable); mld->load_base_addr is always ~0 here, so, it does not make sense to display it here. Though I think that mld->link_base_addr and mld->relocatable should be shown here. Maybe other values from mld which are know here, i.e. align, preference, avoid_efi_boot_services too... > if (mld->relocatable) > { > + load_size = highest_load - mld->link_base_addr; > + > if (load_size > mld->max_addr || mld->min_addr > mld->max_addr - load_size) > return grub_error (GRUB_ERR_BAD_OS, "invalid min/max address and/or load size"); > > @@ -108,27 +113,19 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) > mld->min_addr, mld->max_addr - load_size, > load_size, mld->align ? mld->align : 1, > mld->preference, mld->avoid_efi_boot_services); > - } > - else > - err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch, > - mld->link_base_addr, load_size); > > - if (err) > - { > - grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n"); > - return err; > - } > - > - mld->load_base_addr = get_physical_target_address (ch); > - source = get_virtual_current_address (ch); > + if (err) > + { > + grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n"); > + return err; > + } > > - grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, " > - "load_size=0x%x, relocatable=%d\n", mld->link_base_addr, > - mld->load_base_addr, load_size, mld->relocatable); > + mld->load_base_addr = get_physical_target_address (ch); > + source = get_virtual_current_address (ch); > > - if (mld->relocatable) > - grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n", > - (long) mld->align, mld->preference, mld->avoid_efi_boot_services); > + grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, avoid_efi_boot_services=%d\n", > + (long) mld->align, mld->preference, mld->avoid_efi_boot_services); > + } I think that this grub_dprintf() should be moved... > /* Load every loadable segment in memory. */ > for (i = 0; i < ehdr->e_phnum; i++) > @@ -139,7 +136,23 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld) > grub_dprintf ("multiboot_loader", "segment %d: paddr=0x%lx, memsz=0x%lx, vaddr=0x%lx\n", > i, (long) phdr(i)->p_paddr, (long) phdr(i)->p_memsz, (long) phdr(i)->p_vaddr); > > - load_offset = phdr(i)->p_paddr - mld->link_base_addr; > + if (mld->relocatable) > + load_offset = phdr(i)->p_paddr - mld->link_base_addr; > + else > + { > + err = grub_relocator_alloc_chunk_addr (GRUB_MULTIBOOT (relocator), &ch, > + phdr(i)->p_paddr, phdr(i)->p_memsz); > + > + if (err) > + { > + grub_dprintf ("multiboot_loader", "Cannot allocate memory for OS image\n"); > + return err; > + } > + > + mld->load_base_addr = grub_min (mld->load_base_addr, get_physical_target_address (ch)); > + > + source = get_virtual_current_address (ch); > + } > > if (phdr(i)->p_filesz != 0) > { ... further below, just behind this loop. And mld->load_base_addr should be shown here too. Daniel