| Message ID | 20170612145341.3351-8-leif.lindholm@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: improved correctness, arm unification, and cleanup | expand |
On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote: > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may > not return regions with execute ability. Since modules are loaded onto > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in > order to permit execution on systems with this feature enabled. > > Closes: 50420 > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > grub-core/kern/efi/mm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > index 7b1763bc5..f27a48e68 100644 > --- a/grub-core/kern/efi/mm.c > +++ b/grub-core/kern/efi/mm.c > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map, > pages = required_pages; > } > > - addr = grub_efi_allocate_pages (start, pages); > + addr = grub_efi_allocate_pages_real (start, pages, > + GRUB_EFI_ALLOCATE_ADDRESS, > + GRUB_EFI_LOADER_CODE); Is it really needed? Is any module exectued in place or does it contain code ready to run out of the box? Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote: > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote: > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may > > not return regions with execute ability. Since modules are loaded onto > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in > > order to permit execution on systems with this feature enabled. > > > > Closes: 50420 > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > --- > > grub-core/kern/efi/mm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > index 7b1763bc5..f27a48e68 100644 > > --- a/grub-core/kern/efi/mm.c > > +++ b/grub-core/kern/efi/mm.c > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > *memory_map, > > pages = required_pages; > > } > > > > - addr = grub_efi_allocate_pages (start, pages); > > + addr = grub_efi_allocate_pages_real (start, pages, > > + GRUB_EFI_ALLOCATE_ADDRESS, > > + GRUB_EFI_LOADER_CODE); > > Is it really needed? Is any module exectued in place or does it contain > code ready to run out of the box? > All the modules are loaded to heap > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko wrote: > On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote: > > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote: > > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may > > > not return regions with execute ability. Since modules are loaded onto > > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in > > > order to permit execution on systems with this feature enabled. > > > > > > Closes: 50420 > > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > > --- > > > grub-core/kern/efi/mm.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > > index 7b1763bc5..f27a48e68 100644 > > > --- a/grub-core/kern/efi/mm.c > > > +++ b/grub-core/kern/efi/mm.c > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > > *memory_map, > > > pages = required_pages; > > > } > > > > > > - addr = grub_efi_allocate_pages (start, pages); > > > + addr = grub_efi_allocate_pages_real (start, pages, > > > + GRUB_EFI_ALLOCATE_ADDRESS, > > > + GRUB_EFI_LOADER_CODE); > > > > Is it really needed? Is any module exectued in place or does it contain > > code ready to run out of the box? > > > All the modules are loaded to heap I do not see why modules have to be loaded into memory region with GRUB_EFI_LOADER_CODE type. Kernels are different things and I can agree that they should be loaded into GRUB_EFI_LOADER_CODE. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jul 27, 2017, 17:44 Daniel Kiper <dkiper@net-space.pl> wrote: > On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko > wrote: > > On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote: > > > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote: > > > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA > may > > > > not return regions with execute ability. Since modules are loaded > onto > > > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in > > > > order to permit execution on systems with this feature enabled. > > > > > > > > Closes: 50420 > > > > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > > > --- > > > > grub-core/kern/efi/mm.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > > > index 7b1763bc5..f27a48e68 100644 > > > > --- a/grub-core/kern/efi/mm.c > > > > +++ b/grub-core/kern/efi/mm.c > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > > > *memory_map, > > > > pages = required_pages; > > > > } > > > > > > > > - addr = grub_efi_allocate_pages (start, pages); > > > > + addr = grub_efi_allocate_pages_real (start, pages, > > > > + GRUB_EFI_ALLOCATE_ADDRESS, > > > > + GRUB_EFI_LOADER_CODE); > > > > > > Is it really needed? Is any module exectued in place or does it contain > > > code ready to run out of the box? > > > > > All the modules are loaded to heap > > I do not see why modules have to be loaded into memory region with > GRUB_EFI_LOADER_CODE type. He means grub modules not initrd or multiboot modules > > Kernels > are different things and I can > agree that they should be loaded into GRUB_EFI_LOADER_CODE. > > Daniel > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jul 27, 2017 at 03:45:48PM +0000, Vladimir 'phcoder' Serbinenko wrote: > On Thu, Jul 27, 2017, 17:44 Daniel Kiper <dkiper@net-space.pl> wrote: > > On Thu, Jul 27, 2017 at 03:33:06PM +0000, Vladimir 'phcoder' Serbinenko > > wrote: > > > On Thu, Jul 27, 2017, 17:30 Daniel Kiper <dkiper@net-space.pl> wrote: > > > > On Mon, Jun 12, 2017 at 03:53:41PM +0100, Leif Lindholm wrote: > > > > > With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA > > may > > > > > not return regions with execute ability. Since modules are loaded > > onto > > > > > the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in > > > > > order to permit execution on systems with this feature enabled. > > > > > > > > > > Closes: 50420 > > > > > > > > > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> > > > > > --- > > > > > grub-core/kern/efi/mm.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > > > > index 7b1763bc5..f27a48e68 100644 > > > > > --- a/grub-core/kern/efi/mm.c > > > > > +++ b/grub-core/kern/efi/mm.c > > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > > > > *memory_map, > > > > > pages = required_pages; > > > > > } > > > > > > > > > > - addr = grub_efi_allocate_pages (start, pages); > > > > > + addr = grub_efi_allocate_pages_real (start, pages, > > > > > + GRUB_EFI_ALLOCATE_ADDRESS, > > > > > + GRUB_EFI_LOADER_CODE); > > > > > > > > Is it really needed? Is any module exectued in place or does it contain > > > > code ready to run out of the box? > > > > > > > All the modules are loaded to heap > > > > I do not see why modules have to be loaded into memory region with > > GRUB_EFI_LOADER_CODE type. > > He means grub modules not initrd or multiboot modules Ahhh... Right, then it should be correct. Though I would double check it applies to GRUB2 modules only. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jul 27, 2017 at 06:06:18PM +0200, Daniel Kiper wrote: > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > > > > > index 7b1763bc5..f27a48e68 100644 > > > > > > --- a/grub-core/kern/efi/mm.c > > > > > > +++ b/grub-core/kern/efi/mm.c > > > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > > > > > *memory_map, > > > > > > pages = required_pages; > > > > > > } > > > > > > > > > > > > - addr = grub_efi_allocate_pages (start, pages); > > > > > > + addr = grub_efi_allocate_pages_real (start, pages, > > > > > > + GRUB_EFI_ALLOCATE_ADDRESS, > > > > > > + GRUB_EFI_LOADER_CODE); > > > > > > > > > > Is it really needed? Is any module exectued in place or does it contain > > > > > code ready to run out of the box? > > > > > > > > > All the modules are loaded to heap > > > > > > I do not see why modules have to be loaded into memory region with > > > GRUB_EFI_LOADER_CODE type. > > > > He means grub modules not initrd or multiboot modules > > Ahhh... Right, then it should be correct. Though I would double > check it applies to GRUB2 modules only. It applies to any executable code running until an operating system takes over. Don't get me wrong - this is more of a workaround than a fix for the grub module loading. But properly separating code/data and ro/rw regions is a much more invasive change which deserves more discussion. / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Fri, Jul 28, 2017 at 10:35:38PM +0100, Leif Lindholm wrote: > On Thu, Jul 27, 2017 at 06:06:18PM +0200, Daniel Kiper wrote: > > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c > > > > > > > index 7b1763bc5..f27a48e68 100644 > > > > > > > --- a/grub-core/kern/efi/mm.c > > > > > > > +++ b/grub-core/kern/efi/mm.c > > > > > > > @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t > > > > > > *memory_map, > > > > > > > pages = required_pages; > > > > > > > } > > > > > > > > > > > > > > - addr = grub_efi_allocate_pages (start, pages); > > > > > > > + addr = grub_efi_allocate_pages_real (start, pages, > > > > > > > + GRUB_EFI_ALLOCATE_ADDRESS, > > > > > > > + GRUB_EFI_LOADER_CODE); > > > > > > > > > > > > Is it really needed? Is any module exectued in place or does it contain > > > > > > code ready to run out of the box? > > > > > > > > > > > All the modules are loaded to heap > > > > > > > > I do not see why modules have to be loaded into memory region with > > > > GRUB_EFI_LOADER_CODE type. > > > > > > He means grub modules not initrd or multiboot modules > > > > Ahhh... Right, then it should be correct. Though I would double > > check it applies to GRUB2 modules only. > > It applies to any executable code running until an operating system > takes over. It looks that I was misunderstood. I hope that this change only influence allocations for GRUB2 modules and does not change allocation behavior for others, i.e. OS kernels, Multiboot proto, Multiboo2 proto, etc. > Don't get me wrong - this is more of a workaround than a fix for the > grub module loading. But properly separating code/data and ro/rw > regions is a much more invasive change which deserves more discussion. Yep, you are right. Added to TODO list. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c index 7b1763bc5..f27a48e68 100644 --- a/grub-core/kern/efi/mm.c +++ b/grub-core/kern/efi/mm.c @@ -404,7 +404,9 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map, pages = required_pages; } - addr = grub_efi_allocate_pages (start, pages); + addr = grub_efi_allocate_pages_real (start, pages, + GRUB_EFI_ALLOCATE_ADDRESS, + GRUB_EFI_LOADER_CODE); if (! addr) grub_fatal ("cannot allocate conventional memory %p with %u pages", (void *) ((grub_addr_t) start),
With upcoming changes to EDK2, allocations of type EFI_LOADER_DATA may not return regions with execute ability. Since modules are loaded onto the heap, change the heap allocation type to GRUB_EFI_LOADER_CODE in order to permit execution on systems with this feature enabled. Closes: 50420 Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org> --- grub-core/kern/efi/mm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 2.11.0 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel