The first change is, I believe, addressing the regression spotted by
osstest. The second change is simply a result of me going over the
involved code in, effectively, a re-review of the original changes.

1: EFI/Arm64: don't clobber DTB pointer
2: EFI: further "need_to_free" adjustments

Jan

read_section() needs to be more careful: efi_arch_use_config_file()
may have found a DTB file (but without modules), and there may be no DTB
specified in the EFI config file. In this case the pointer to the blob
must not be overwritten with NULL when no ".dtb" section is present
either.

Fixes: 8a71d50ed40b ("efi: Enable booting unified hypervisor/kernel/initrd images")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -637,11 +637,14 @@ static bool __init read_section(const EF
                                 const CHAR16 *name, struct file *file,
                                 const char *options)
 {
-    file->ptr = pe_find_section(image->ImageBase, image->ImageSize,
-                                name, &file->size);
-    if ( !file->ptr )
+    const void *ptr = pe_find_section(image->ImageBase, image->ImageSize,
+                                      name, &file->size);
+
+    if ( !ptr )
         return false;
 
+    file->ptr = ptr;
+
     handle_file_info(name, file, options);
 
     return true;

On 14/10/2020 11:42, Jan Beulich wrote:
> read_section() needs to be more careful: efi_arch_use_config_file()
> may have found a DTB file (but without modules), and there may be no DTB
> specified in the EFI config file. In this case the pointer to the blob
> must not be overwritten with NULL when no ".dtb" section is present
> either.
>
> Fixes: 8a71d50ed40b ("efi: Enable booting unified hypervisor/kernel/initrd images")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

When processing "chain" directives, the previously loaded config file
gets freed. This needs to be recorded accordingly such that no error
path would try to free the same block of memory a 2nd time.

Furthermore, neither .addr nor .size being zero has any meaning towards
the need to free an allocated chunk anymore. Drop (from read_file()) and
replace (in Arm's efi_arch_use_config_file(), to sensibly retain the
comment) respective assignments.

Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -591,7 +591,7 @@ static bool __init efi_arch_use_config_f
 
     fdt = lookup_fdt_config_table(SystemTable);
     dtbfile.ptr = fdt;
-    dtbfile.size = 0;  /* Config table memory can't be freed, so set size to 0 */
+    dtbfile.need_to_free = false; /* Config table memory can't be freed. */
     if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 )
     {
         /*
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -601,10 +601,7 @@ static bool __init read_file(EFI_FILE_HA
                                     PFN_UP(size), &file->addr);
     }
     if ( EFI_ERROR(ret) )
-    {
-        file->addr = 0;
         what = what ?: L"Allocation";
-    }
     else
     {
         file->need_to_free = true;
@@ -1271,8 +1268,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
             name.s = get_value(&cfg, "global", "chain");
             if ( !name.s )
                 break;
-            efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
-            cfg.addr = 0;
+            if ( cfg.need_to_free )
+            {
+                efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
+                cfg.need_to_free = false;
+            }
             if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
             {
                 PrintStr(L"Chained configuration file '");

On Wed, 14 Oct 2020, Jan Beulich wrote:
> When processing "chain" directives, the previously loaded config file
> gets freed. This needs to be recorded accordingly such that no error
> path would try to free the same block of memory a 2nd time.
> 
> Furthermore, neither .addr nor .size being zero has any meaning towards
> the need to free an allocated chunk anymore. Drop (from read_file()) and
> replace (in Arm's efi_arch_use_config_file(), to sensibly retain the
> comment) respective assignments.
> 
> Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -591,7 +591,7 @@ static bool __init efi_arch_use_config_f
>  
>      fdt = lookup_fdt_config_table(SystemTable);
>      dtbfile.ptr = fdt;
> -    dtbfile.size = 0;  /* Config table memory can't be freed, so set size to 0 */
> +    dtbfile.need_to_free = false; /* Config table memory can't be freed. */
>      if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 )
>      {
>          /*
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -601,10 +601,7 @@ static bool __init read_file(EFI_FILE_HA
>                                      PFN_UP(size), &file->addr);
>      }
>      if ( EFI_ERROR(ret) )
> -    {
> -        file->addr = 0;
>          what = what ?: L"Allocation";
> -    }
>      else
>      {
>          file->need_to_free = true;
> @@ -1271,8 +1268,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY
>              name.s = get_value(&cfg, "global", "chain");
>              if ( !name.s )
>                  break;
> -            efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> -            cfg.addr = 0;
> +            if ( cfg.need_to_free )
> +            {
> +                efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> +                cfg.need_to_free = false;
> +            }
>              if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
>              {
>                  PrintStr(L"Chained configuration file '");
>