Re: [PATCH v4 1/4] efi/boot.c: add file.need_to_free

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 14, 2020 at 07:50:10AM -0400, Trammell Hudson wrote:
> The config file, kernel, initrd, etc should only be freed if they
> are allocated with the UEFI allocator.
> 
> Signed-off-by: Trammell Hudson <hudson@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
>  xen/common/efi/boot.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 4022a672c9..7156139174 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -102,6 +102,7 @@ union string {
>  
>  struct file {
>      UINTN size;
> +    bool need_to_free;
>      union {
>          EFI_PHYSICAL_ADDRESS addr;
>          void *ptr;
> @@ -279,13 +280,13 @@ void __init noreturn blexit(const CHAR16 *str)
>      if ( !efi_bs )
>          efi_arch_halt();
>  
> -    if ( cfg.addr )
> +    if ( cfg.addr && cfg.need_to_free )
>          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -    if ( kernel.addr )
> +    if ( kernel.addr && kernel.need_to_free )
>          efi_bs->FreePages(kernel.addr, PFN_UP(kernel.size));
> -    if ( ramdisk.addr )
> +    if ( ramdisk.addr && ramdisk.need_to_free )
>          efi_bs->FreePages(ramdisk.addr, PFN_UP(ramdisk.size));
> -    if ( xsm.addr )
> +    if ( xsm.addr && xsm.need_to_free )
>          efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
>  
>      efi_arch_blexit();
> @@ -572,6 +573,7 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>                           HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>          ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>                                      PFN_UP(size), &file->addr);
> +        file->need_to_free = true;

Strictly speaking, don't you need to set need_to_free only if
AllocatePages has succeed? I guess it doesn't matter much because addr
would be zapped to 0 if allocation fails.

Thanks, Roger.