* [PATCH] mbi: use per segment a separate relocator chunk @ 2018-05-14 19:02 Alexander Boettcher 2018-05-15 13:42 ` Daniel Kiper 0 siblings, 1 reply; 6+ messages in thread From: Alexander Boettcher @ 2018-05-14 19:02 UTC (permalink / raw) To: grub-devel [-- Attachment #1: Type: text/plain, Size: 216 bytes --] -- Alexander Boettcher Genode Labs http://www.genode-labs.com - http://www.genode.org Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth [-- Attachment #2: 0001-mbi-use-per-segment-a-separate-relocator-chunk.patch --] [-- Type: text/x-patch, Size: 4873 bytes --] From 8a0296d040a42950cd64e733f7997203975bc184 Mon Sep 17 00:00:00 2001 From: Alexander Boettcher <alexander.boettcher@genode-labs.com> Date: Fri, 13 Apr 2018 23:37:01 +0200 Subject: [PATCH] mbi: use per segment a separate relocator chunk if elf is non relocatable. 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. Signed-off-by: Alexander Boettcher <alexander.boettcher@genode-labs.com> --- 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; 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); 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); + } /* 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) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mbi: use per segment a separate relocator chunk 2018-05-14 19:02 [PATCH] mbi: use per segment a separate relocator chunk Alexander Boettcher @ 2018-05-15 13:42 ` Daniel Kiper 2018-05-15 19:10 ` Alexander Boettcher 2018-06-05 19:59 ` Alexander Boettcher 0 siblings, 2 replies; 6+ messages in thread From: Daniel Kiper @ 2018-05-15 13:42 UTC (permalink / raw) To: alexander.boettcher; +Cc: grub-devel, dkiper 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 <alexander.boettcher@genode-labs.com> > 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 <alexander.boettcher@genode-labs.com> > --- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mbi: use per segment a separate relocator chunk 2018-05-15 13:42 ` Daniel Kiper @ 2018-05-15 19:10 ` Alexander Boettcher 2018-05-15 19:18 ` Alexander Boettcher 2018-06-05 19:59 ` Alexander Boettcher 1 sibling, 1 reply; 6+ messages in thread From: Alexander Boettcher @ 2018-05-15 19:10 UTC (permalink / raw) To: The development of GNU GRUB, Daniel Kiper On 15.05.2018 15:42, Daniel Kiper wrote: > 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 <alexander.boettcher@genode-labs.com> >> 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, No. > so, I am not sure what exactly overwrites this region. > grub_memset() in current line 161 at some point? No. During grub_relocator_prepare_reloc the overwrite happens, if i'm not wrong. An (artificial) example, imagine two ELF PHDRs, e.g. [0x8000-0x9000) and [0x2000000-0x2100000). Without this patch grub calculates one relocator chunk of size 0x20f8000 (0x2100000 - 0x8000) and places it at some higher memory, e.g. [0x3000000 - 0x30f8000). During the invocation of grub_relocator_prepare_reloc the memory gets copied from [0x3000000-0x30f8000) to [0x8000-0x2100000) and now the VGA memory area got overwritten and you have a lot of blinking symbols on the screen. > >> Signed-off-by: Alexander Boettcher <alexander.boettcher@genode-labs.com> >> --- >> 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 > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > -- Alexander Boettcher Genode Labs http://www.genode-labs.com - http://www.genode.org Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mbi: use per segment a separate relocator chunk 2018-05-15 19:10 ` Alexander Boettcher @ 2018-05-15 19:18 ` Alexander Boettcher 2018-05-16 10:22 ` Daniel Kiper 0 siblings, 1 reply; 6+ messages in thread From: Alexander Boettcher @ 2018-05-15 19:18 UTC (permalink / raw) To: The development of GNU GRUB, Daniel Kiper On 15.05.2018 21:10, Alexander Boettcher wrote: >> I assume that a given ELF PHDR address/size does not cover VGA memory or >> anything like that, > > No. > >> so, I am not sure what exactly overwrites this region. >> grub_memset() in current line 161 at some point? > > No. During grub_relocator_prepare_reloc the overwrite happens, if i'm > not wrong. > > An (artificial) example, imagine two ELF PHDRs, e.g. > > [0x8000-0x9000) and > [0x2000000-0x2100000). > > Without this patch grub calculates one relocator chunk of size 0x20f8000 > (0x2100000 - 0x8000) and places it at some higher memory, e.g. > [0x3000000 - 0x30f8000). During the invocation of Must be [0x3000000-0x50f8000) > grub_relocator_prepare_reloc the memory gets copied from > > [0x3000000-0x30f8000) to [0x8000-0x2100000) Must be [0x3000000-0x50f8000) to [0x8000-0x2100000) Sorry. -- Alexander Boettcher Genode Labs http://www.genode-labs.com - http://www.genode.org Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mbi: use per segment a separate relocator chunk 2018-05-15 19:18 ` Alexander Boettcher @ 2018-05-16 10:22 ` Daniel Kiper 0 siblings, 0 replies; 6+ messages in thread From: Daniel Kiper @ 2018-05-16 10:22 UTC (permalink / raw) To: Alexander Boettcher; +Cc: The development of GNU GRUB, Daniel Kiper On Tue, May 15, 2018 at 09:18:18PM +0200, Alexander Boettcher wrote: > On 15.05.2018 21:10, Alexander Boettcher wrote: > >>I assume that a given ELF PHDR address/size does not cover VGA memory or > >>anything like that, > > > >No. > > > >>so, I am not sure what exactly overwrites this region. > >>grub_memset() in current line 161 at some point? > > > >No. During grub_relocator_prepare_reloc the overwrite happens, if i'm > >not wrong. > > > >An (artificial) example, imagine two ELF PHDRs, e.g. > > > > ??[0x8000-0x9000) and > > ??[0x2000000-0x2100000). > > > >Without this patch grub calculates one relocator chunk of size 0x20f8000 > >(0x2100000 - 0x8000) and places it at some higher memory, e.g. > >[0x3000000 - 0x30f8000). During the invocation of > > Must be [0x3000000-0x50f8000) > > >grub_relocator_prepare_reloc the memory gets copied from > > > >[0x3000000-0x30f8000) to [0x8000-0x2100000) > > Must be [0x3000000-0x50f8000) to [0x8000-0x2100000) It seems to me that it happens a bit later. AIUI grub_relocator_prepare_reloc() prepare movers which are executed when main GRUB code is left, e.g relst() call from grub_relocator32_boot(). And the movers do bad job. Well, they were told to do so... Anyway, I think that the comment should be a bit more clear about it. Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mbi: use per segment a separate relocator chunk 2018-05-15 13:42 ` Daniel Kiper 2018-05-15 19:10 ` Alexander Boettcher @ 2018-06-05 19:59 ` Alexander Boettcher 1 sibling, 0 replies; 6+ messages in thread From: Alexander Boettcher @ 2018-06-05 19:59 UTC (permalink / raw) To: The development of GNU GRUB; +Cc: Daniel Kiper On 15.05.2018 15:42, Daniel Kiper wrote: > On Mon, May 14, 2018 at 09:02:00PM +0200, Alexander Boettcher wrote: >> 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;". > The compiler complains if I remove the NULL assignment with: error: ‘source’ may be used uninitialized in this function [-Werror=maybe-uninitialized] grub_memset ((grub_uint8_t *) source + load_offset + phdr(i)->p_filesz, 0, I try to incorporate your further comments and re-send the patch. -- Alexander Boettcher Genode Labs http://www.genode-labs.com - http://www.genode.org Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-05 20:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-14 19:02 [PATCH] mbi: use per segment a separate relocator chunk Alexander Boettcher 2018-05-15 13:42 ` Daniel Kiper 2018-05-15 19:10 ` Alexander Boettcher 2018-05-15 19:18 ` Alexander Boettcher 2018-05-16 10:22 ` Daniel Kiper 2018-06-05 19:59 ` Alexander Boettcher
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.