[PATCH] efi: Free malloc regions on exit

[PATCH] efi: Free malloc regions on exit

[Alexander Graf]

When we exit grub, we don't free all the memory that we allocated earlier
for our heap region. This can cause problems with setups where you try
to descend the boot order using "exit" entries, such as PXE -> HD boot
scenarios.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 grub-core/kern/efi/init.c |  1 +
 grub-core/kern/efi/mm.c   | 24 ++++++++++++++++++++++++
 include/grub/efi/efi.h    |  1 +
 3 files changed, 26 insertions(+)

diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index e9c85de..b848014 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -77,4 +77,5 @@ grub_efi_fini (void)
 {
   grub_efidisk_fini ();
   grub_console_fini ();
+  grub_efi_memory_fini ();
 }
diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 20a47aa..4cd5971 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -49,6 +49,12 @@ static grub_efi_uintn_t finish_desc_size;
 static grub_efi_uint32_t finish_desc_version;
 int grub_efi_is_finished = 0;
 
+struct efi_allocation {
+	grub_uint64_t start_addr;
+	grub_uint64_t pages;
+} efi_allocated_memory[16];
+unsigned int efi_allocated_memory_idx = 0;
+
 /* Allocate pages. Return the pointer to the first of allocated pages.  */
 void *
 grub_efi_allocate_pages (grub_efi_physical_address_t address,
@@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
 		    (void *) ((grub_addr_t) start),
 		    (unsigned) pages);
 
+      /* Track up to 16 regions that we allocate from */
+      if (efi_allocated_memory_idx < ARRAY_SIZE(efi_allocated_memory)) {
+        efi_allocated_memory[efi_allocated_memory_idx].start_addr = start;
+        efi_allocated_memory[efi_allocated_memory_idx].pages = pages;
+        efi_allocated_memory_idx++;
+      }
+
       grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
 
       required_pages -= pages;
@@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
     grub_fatal ("too little memory");
 }
 
+void
+grub_efi_memory_fini (void)
+{
+  unsigned int i;
+
+  for (i = 0; i < efi_allocated_memory_idx; i++) {
+    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
+                         efi_allocated_memory[i].pages);
+  }
+}
+
 #if 0
 /* Print the memory map.  */
 static void
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 0e6fd86..545e7ce 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -48,6 +48,7 @@ EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
 				      grub_efi_uintn_t *map_key,
 				      grub_efi_uintn_t *descriptor_size,
 				      grub_efi_uint32_t *descriptor_version);
+void grub_efi_memory_fini (void);
 grub_efi_loaded_image_t *EXPORT_FUNC(grub_efi_get_loaded_image) (grub_efi_handle_t image_handle);
 void EXPORT_FUNC(grub_efi_print_device_path) (grub_efi_device_path_t *dp);
 char *EXPORT_FUNC(grub_efi_get_filename) (grub_efi_device_path_t *dp);
-- 
1.8.5.6

Re: [PATCH] efi: Free malloc regions on exit

[Andrei Borzenkov]

19.05.2016 16:37, Alexander Graf пишет:
> When we exit grub, we don't free all the memory that we allocated earlier
> for our heap region. This can cause problems with setups where you try
> to descend the boot order using "exit" entries, such as PXE -> HD boot
> scenarios.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  grub-core/kern/efi/init.c |  1 +
>  grub-core/kern/efi/mm.c   | 24 ++++++++++++++++++++++++
>  include/grub/efi/efi.h    |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> index e9c85de..b848014 100644
> --- a/grub-core/kern/efi/init.c
> +++ b/grub-core/kern/efi/init.c
> @@ -77,4 +77,5 @@ grub_efi_fini (void)
>  {
>    grub_efidisk_fini ();
>    grub_console_fini ();
> +  grub_efi_memory_fini ();
>  }

Note that grub_efi_fini() is called not only during exit, but also by
grub_loader_boot (grub_machine_fini); and - at least, theoretically -
grub_loader_boot_func can fail and we return back to GRUB. Which leaves
us with heap pointing to already freed area. We probably cannot do
anything useful at this point anyway, but this may lead to corruption of
memory allocated by other EFI drivers.

May be it should be called explicitly only in exit path.

Also it is not called during chainload at all, which should have the
same problem (i.e. conceptually it does not matter whether we exit grub
and select next binary from EFI menu or simply try to chainload it from
grub).

> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 20a47aa..4cd5971 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -49,6 +49,12 @@ static grub_efi_uintn_t finish_desc_size;
>  static grub_efi_uint32_t finish_desc_version;
>  int grub_efi_is_finished = 0;
>  
> +struct efi_allocation {
> +	grub_uint64_t start_addr;
> +	grub_uint64_t pages;
> +} efi_allocated_memory[16];
> +unsigned int efi_allocated_memory_idx = 0;
> +
>  /* Allocate pages. Return the pointer to the first of allocated pages.  */
>  void *
>  grub_efi_allocate_pages (grub_efi_physical_address_t address,
> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
>  		    (void *) ((grub_addr_t) start),
>  		    (unsigned) pages);
>  
> +      /* Track up to 16 regions that we allocate from */
> +      if (efi_allocated_memory_idx < ARRAY_SIZE(efi_allocated_memory)) {
> +        efi_allocated_memory[efi_allocated_memory_idx].start_addr = start;
> +        efi_allocated_memory[efi_allocated_memory_idx].pages = pages;
> +        efi_allocated_memory_idx++;
> +      }
> +

Can we walk regions list instead? May be we could store original address
and size in region descriptor?

>        grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
>  
>        required_pages -= pages;

Hmm ... grub_mm_init_region may silently skip some regions. So this is
strictly speaking wrong (not related to your patch).

> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
>      grub_fatal ("too little memory");
>  }
>  
> +void
> +grub_efi_memory_fini (void)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < efi_allocated_memory_idx; i++) {
> +    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
> +                         efi_allocated_memory[i].pages);
> +  }
> +}
> +
>  #if 0
>  /* Print the memory map.  */
>  static void
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index 0e6fd86..545e7ce 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -48,6 +48,7 @@ EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
>  				      grub_efi_uintn_t *map_key,
>  				      grub_efi_uintn_t *descriptor_size,
>  				      grub_efi_uint32_t *descriptor_version);
> +void grub_efi_memory_fini (void);
>  grub_efi_loaded_image_t *EXPORT_FUNC(grub_efi_get_loaded_image) (grub_efi_handle_t image_handle);
>  void EXPORT_FUNC(grub_efi_print_device_path) (grub_efi_device_path_t *dp);
>  char *EXPORT_FUNC(grub_efi_get_filename) (grub_efi_device_path_t *dp);
> 

Re: [PATCH] efi: Free malloc regions on exit

[Michael Chang]

On Fri, May 20, 2016 at 06:56:21AM +0300, Andrei Borzenkov wrote:
> 19.05.2016 16:37, Alexander Graf пишет:
> > When we exit grub, we don't free all the memory that we allocated earlier
> > for our heap region. This can cause problems with setups where you try
> > to descend the boot order using "exit" entries, such as PXE -> HD boot
> > scenarios.
> > 
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> > ---
> >  grub-core/kern/efi/init.c |  1 +
> >  grub-core/kern/efi/mm.c   | 24 ++++++++++++++++++++++++
> >  include/grub/efi/efi.h    |  1 +
> >  3 files changed, 26 insertions(+)
> > 
> > diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
> > index e9c85de..b848014 100644
> > --- a/grub-core/kern/efi/init.c
> > +++ b/grub-core/kern/efi/init.c
> > @@ -77,4 +77,5 @@ grub_efi_fini (void)
> >  {
> >    grub_efidisk_fini ();
> >    grub_console_fini ();
> > +  grub_efi_memory_fini ();
> >  }
> 
> Note that grub_efi_fini() is called not only during exit, but also by
> grub_loader_boot (grub_machine_fini); and - at least, theoretically -
> grub_loader_boot_func can fail and we return back to GRUB. Which leaves
> us with heap pointing to already freed area. We probably cannot do
> anything useful at this point anyway, but this may lead to corruption of
> memory allocated by other EFI drivers.

I think grub_machine_fini is called without GRUB_LOADER_FLAG_NORETURN flag set
in above-mentioned case so that it should be fine. 

Thanks,
Michael

> 
> May be it should be called explicitly only in exit path.
> 
> Also it is not called during chainload at all, which should have the
> same problem (i.e. conceptually it does not matter whether we exit grub
> and select next binary from EFI menu or simply try to chainload it from
> grub).
> 
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 20a47aa..4cd5971 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -49,6 +49,12 @@ static grub_efi_uintn_t finish_desc_size;
> >  static grub_efi_uint32_t finish_desc_version;
> >  int grub_efi_is_finished = 0;
> >  
> > +struct efi_allocation {
> > +	grub_uint64_t start_addr;
> > +	grub_uint64_t pages;
> > +} efi_allocated_memory[16];
> > +unsigned int efi_allocated_memory_idx = 0;
> > +
> >  /* Allocate pages. Return the pointer to the first of allocated pages.  */
> >  void *
> >  grub_efi_allocate_pages (grub_efi_physical_address_t address,
> > @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
> >  		    (void *) ((grub_addr_t) start),
> >  		    (unsigned) pages);
> >  
> > +      /* Track up to 16 regions that we allocate from */
> > +      if (efi_allocated_memory_idx < ARRAY_SIZE(efi_allocated_memory)) {
> > +        efi_allocated_memory[efi_allocated_memory_idx].start_addr = start;
> > +        efi_allocated_memory[efi_allocated_memory_idx].pages = pages;
> > +        efi_allocated_memory_idx++;
> > +      }
> > +
> 
> Can we walk regions list instead? May be we could store original address
> and size in region descriptor?
> 
> >        grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
> >  
> >        required_pages -= pages;
> 
> Hmm ... grub_mm_init_region may silently skip some regions. So this is
> strictly speaking wrong (not related to your patch).
> 
> > @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
> >      grub_fatal ("too little memory");
> >  }
> >  
> > +void
> > +grub_efi_memory_fini (void)
> > +{
> > +  unsigned int i;
> > +
> > +  for (i = 0; i < efi_allocated_memory_idx; i++) {
> > +    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
> > +                         efi_allocated_memory[i].pages);
> > +  }
> > +}
> > +
> >  #if 0
> >  /* Print the memory map.  */
> >  static void
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index 0e6fd86..545e7ce 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -48,6 +48,7 @@ EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
> >  				      grub_efi_uintn_t *map_key,
> >  				      grub_efi_uintn_t *descriptor_size,
> >  				      grub_efi_uint32_t *descriptor_version);
> > +void grub_efi_memory_fini (void);
> >  grub_efi_loaded_image_t *EXPORT_FUNC(grub_efi_get_loaded_image) (grub_efi_handle_t image_handle);
> >  void EXPORT_FUNC(grub_efi_print_device_path) (grub_efi_device_path_t *dp);
> >  char *EXPORT_FUNC(grub_efi_get_filename) (grub_efi_device_path_t *dp);
> > 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Free malloc regions on exit

[Andrei Borzenkov]

20.05.2016 07:34, Michael Chang пишет:
> On Fri, May 20, 2016 at 06:56:21AM +0300, Andrei Borzenkov wrote:
>> 19.05.2016 16:37, Alexander Graf пишет:
>>> When we exit grub, we don't free all the memory that we allocated earlier
>>> for our heap region. This can cause problems with setups where you try
>>> to descend the boot order using "exit" entries, such as PXE -> HD boot
>>> scenarios.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  grub-core/kern/efi/init.c |  1 +
>>>  grub-core/kern/efi/mm.c   | 24 ++++++++++++++++++++++++
>>>  include/grub/efi/efi.h    |  1 +
>>>  3 files changed, 26 insertions(+)
>>>
>>> diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
>>> index e9c85de..b848014 100644
>>> --- a/grub-core/kern/efi/init.c
>>> +++ b/grub-core/kern/efi/init.c
>>> @@ -77,4 +77,5 @@ grub_efi_fini (void)
>>>  {
>>>    grub_efidisk_fini ();
>>>    grub_console_fini ();
>>> +  grub_efi_memory_fini ();
>>>  }
>>
>> Note that grub_efi_fini() is called not only during exit, but also by
>> grub_loader_boot (grub_machine_fini); and - at least, theoretically -
>> grub_loader_boot_func can fail and we return back to GRUB. Which leaves
>> us with heap pointing to already freed area. We probably cannot do
>> anything useful at this point anyway, but this may lead to corruption of
>> memory allocated by other EFI drivers.
> 
> I think grub_machine_fini is called without GRUB_LOADER_FLAG_NORETURN flag set
> in above-mentioned case so that it should be fine. 
> 

Well, there are calls both with and without GRUB_LOADER_FLAG_NORETURN.
It is true that for EFI platform all *existing* calls are without, but
nothing really forces it. I am a bit uneasy about it. Ideally I'd prefer
clean code path that frees memory immediately before existing GRUB.

> Thanks,
> Michael
> 
>>
>> May be it should be called explicitly only in exit path.
>>
>> Also it is not called during chainload at all, which should have the
>> same problem (i.e. conceptually it does not matter whether we exit grub
>> and select next binary from EFI menu or simply try to chainload it from
>> grub).
>>

No comments?

RE: [PATCH] efi: Free malloc regions on exit

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Grub-devel [mailto:grub-devel-bounces+elliott=hpe.com@gnu.org]
> On Behalf Of Alexander Graf
> Sent: Thursday, May 19, 2016 8:38 AM
> Subject: [PATCH] efi: Free malloc regions on exit
...
> +struct efi_allocation {

If no other file is using this, mark it as static.

> +	grub_uint64_t start_addr;

That should be grub_efi_physical_address_t.

> +	grub_uint64_t pages;

That should be grub_efi_uint64_t (the parameter type
for add_memory_regions) or grub_efi_uintn_t (the parameter
type for grub_efi_allocate_pages and grub_efi_free_pages).

> +} efi_allocated_memory[16];

Why 16 and not some other number?  Consider a #define and making
the comment about 16 in add_memory_regions more generic.

> +unsigned int efi_allocated_memory_idx = 0;

The C language definition guarantees that global variables
are initialized to 0.


> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
>  		    (void *) ((grub_addr_t) start),
>  		    (unsigned) pages);
> 
> +      /* Track up to 16 regions that we allocate from */
> +      if (efi_allocated_memory_idx <
> ARRAY_SIZE(efi_allocated_memory)) {
> +        efi_allocated_memory[efi_allocated_memory_idx].start_addr =
> start;
> +        efi_allocated_memory[efi_allocated_memory_idx].pages =
> pages;
> +        efi_allocated_memory_idx++;
> +      }
> +

Consider printing if the magic number is exceeded, rather than
silently changing the behavior.

>        grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
> 
>        required_pages -= pages;
> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
>      grub_fatal ("too little memory");
>  }
> 
> +void
> +grub_efi_memory_fini (void)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < efi_allocated_memory_idx; i++) {
> +    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
> +                         efi_allocated_memory[i].pages);
> +  }
> +}

Consider clearing efi_allocated_memory_idx after that, since 
this function cannot be sure it's the last one to use a
global variable.

Clearing start_addr might help prevent stale pointer
reference bugs.  Pointers that are no longer valid are 
dangerous to leave around.


---
Robert Elliott, HPE Persistent Memory

Re: [PATCH] efi: Free malloc regions on exit

[Alexander Graf]

On 05/24/2016 06:56 AM, Elliott, Robert (Persistent Memory) wrote:
>> -----Original Message-----
>> From: Grub-devel [mailto:grub-devel-bounces+elliott=hpe.com@gnu.org]
>> On Behalf Of Alexander Graf
>> Sent: Thursday, May 19, 2016 8:38 AM
>> Subject: [PATCH] efi: Free malloc regions on exit
> ...
>> +struct efi_allocation {
> If no other file is using this, mark it as static.

Good idea.

>
>> +	grub_uint64_t start_addr;
> That should be grub_efi_physical_address_t.
>
>> +	grub_uint64_t pages;
> That should be grub_efi_uint64_t (the parameter type
> for add_memory_regions) or grub_efi_uintn_t (the parameter
> type for grub_efi_allocate_pages and grub_efi_free_pages).

Yup for the type changes.

>
>> +} efi_allocated_memory[16];
> Why 16 and not some other number?  Consider a #define and making
> the comment about 16 in add_memory_regions more generic.

I've added some helpful comment above and moved it into a #define.

>
>> +unsigned int efi_allocated_memory_idx = 0;
> The C language definition guarantees that global variables
> are initialized to 0.

Yes, but multiple other global variables in the file also initialize 
with 0. Last time I checked gcc just puts those variables into bss 
either way, so you don't really lose anything by explicitly saying = 0 
here. It does increase readability imho though to have it.

>
>
>> @@ -408,6 +414,13 @@ add_memory_regions (grub_efi_memory_descriptor_t
>> *memory_map,
>>   		    (void *) ((grub_addr_t) start),
>>   		    (unsigned) pages);
>>
>> +      /* Track up to 16 regions that we allocate from */
>> +      if (efi_allocated_memory_idx <
>> ARRAY_SIZE(efi_allocated_memory)) {
>> +        efi_allocated_memory[efi_allocated_memory_idx].start_addr =
>> start;
>> +        efi_allocated_memory[efi_allocated_memory_idx].pages =
>> pages;
>> +        efi_allocated_memory_idx++;
>> +      }
>> +
> Consider printing if the magic number is exceeded, rather than
> silently changing the behavior.

Good idea. I don't think anyone really cares enough - there should still 
be enough ram getting free'd on exit if memory is that heavily 
segmented. But I suppose firmware authors want to be aware that their 
memory map is a mess, and this could tell them :).

>
>>         grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
>>
>>         required_pages -= pages;
>> @@ -419,6 +432,17 @@ add_memory_regions (grub_efi_memory_descriptor_t
>> *memory_map,
>>       grub_fatal ("too little memory");
>>   }
>>
>> +void
>> +grub_efi_memory_fini (void)
>> +{
>> +  unsigned int i;
>> +
>> +  for (i = 0; i < efi_allocated_memory_idx; i++) {
>> +    grub_efi_free_pages (efi_allocated_memory[i].start_addr,
>> +                         efi_allocated_memory[i].pages);
>> +  }
>> +}
> Consider clearing efi_allocated_memory_idx after that, since
> this function cannot be sure it's the last one to use a
> global variable.

I'm not sure it's incredibly helpful (since this is a _fini function 
that basically denotes the end of grub's life), but sure.

>
> Clearing start_addr might help prevent stale pointer
> reference bugs.  Pointers that are no longer valid are
> dangerous to leave around.

That one however I don't agree with. Array elements are only really 
valid if their counter considers them as valid. If we initialize the 
counter to 0 that means the whole array should automatically be 
considered unused.

Thanks a lot for the review :).


Alex