[7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

Message ID 20170612145341.3351-8-leif.lindholm@linaro.org
State Superseded
Headers show
Series
  • efi: improved correctness, arm unification, and cleanup
Related show

Commit Message

Leif Lindholm June 12, 2017, 2:53 p.m.
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

Comments

Daniel Kiper July 27, 2017, 3:29 p.m. | #1
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
Vladimir 'phcoder' Serbinenko July 27, 2017, 3:33 p.m. | #2
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
Daniel Kiper July 27, 2017, 3:44 p.m. | #3
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
Vladimir 'phcoder' Serbinenko July 27, 2017, 3:45 p.m. | #4
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
Daniel Kiper July 27, 2017, 4:06 p.m. | #5
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
Leif Lindholm July 28, 2017, 9:35 p.m. | #6
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
Daniel Kiper Aug. 1, 2017, 12:13 p.m. | #7
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

Patch

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),