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 <address@hidden>
> > ---
> >  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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel