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 <ag...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
