If the config file is builtin, cfg.addr will be zero but Xen
unconditionally calls FreePages() on the address.

Xen may also call FreePages() with a zero address if blexit() is called
after this point since cfg.need_to_free is not set to false.

The UEFI specification does not say whether calling FreePages() with a
zero address is allowed so let's be cautious and use cfg.need_to_free
properly.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/efi/boot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 778a39cc48e6..50ff1d1bd225 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1534,8 +1534,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
 
         efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
 
-        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 ( dir_handle )
             dir_handle->Close(dir_handle);
-- 
2.50.1

On 8/5/25 12:32, Ross Lagerwall wrote:
> If the config file is builtin, cfg.addr will be zero but Xen
> unconditionally calls FreePages() on the address.
> 
> Xen may also call FreePages() with a zero address if blexit() is called
> after this point since cfg.need_to_free is not set to false.
> 
> The UEFI specification does not say whether calling FreePages() with a
> zero address is allowed so let's be cautious and use cfg.need_to_free
> properly.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>   xen/common/efi/boot.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 778a39cc48e6..50ff1d1bd225 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1534,8 +1534,11 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>   
>           efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
>   
> -        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 ( dir_handle )
>               dir_handle->Close(dir_handle);

Acked-by Daniel P. Smith <dpsmith@apertussolutions.com>

On 05/08/2025 5:32 pm, Ross Lagerwall wrote:
> If the config file is builtin, cfg.addr will be zero but Xen
> unconditionally calls FreePages() on the address.
>
> Xen may also call FreePages() with a zero address if blexit() is called
> after this point since cfg.need_to_free is not set to false.
>
> The UEFI specification does not say whether calling FreePages() with a
> zero address is allowed so let's be cautious and use cfg.need_to_free
> properly.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

On 05.08.2025 18:32, Ross Lagerwall wrote:
> If the config file is builtin, cfg.addr will be zero but Xen
> unconditionally calls FreePages() on the address.
> 
> Xen may also call FreePages() with a zero address if blexit() is called
> after this point since cfg.need_to_free is not set to false.
> 
> The UEFI specification does not say whether calling FreePages() with a
> zero address is allowed so let's be cautious and use cfg.need_to_free
> properly.

Well, no, this paragraph makes no sense. Of course this is allowed, but
not as no-op behavior (like free(NULL) would be), but to free memory
starting at 0.

> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

This pretty clearly wants a Fixes: tag, or maybe it even needs to be two.
I've checked the original code in 4.2, and things were consistent there,
afaics. So breakage was introduced perhaps in one or two of the many
re-works.

Jan

On Wed, Aug 6, 2025 at 7:16 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.08.2025 18:32, Ross Lagerwall wrote:
> > If the config file is builtin, cfg.addr will be zero but Xen
> > unconditionally calls FreePages() on the address.
> >
> > Xen may also call FreePages() with a zero address if blexit() is called
> > after this point since cfg.need_to_free is not set to false.
> >
> > The UEFI specification does not say whether calling FreePages() with a
> > zero address is allowed so let's be cautious and use cfg.need_to_free
> > properly.
>
> Well, no, this paragraph makes no sense. Of course this is allowed, but
> not as no-op behavior (like free(NULL) would be), but to free memory
> starting at 0.

Fair enough. This paragraph could simply be dropped then.

>
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> This pretty clearly wants a Fixes: tag, or maybe it even needs to be two.
> I've checked the original code in 4.2, and things were consistent there,
> afaics. So breakage was introduced perhaps in one or two of the many
> re-works.
>

Fixes: 8a71d50ed40b ("efi: Enable booting unified
hypervisor/kernel/initrd images")
Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free")

Do you want an updated patch or can these tweaks be done while committing?

Thanks,
Ross

On 08.08.2025 12:14, Ross Lagerwall wrote:
> On Wed, Aug 6, 2025 at 7:16 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 05.08.2025 18:32, Ross Lagerwall wrote:
>>> If the config file is builtin, cfg.addr will be zero but Xen
>>> unconditionally calls FreePages() on the address.
>>>
>>> Xen may also call FreePages() with a zero address if blexit() is called
>>> after this point since cfg.need_to_free is not set to false.
>>>
>>> The UEFI specification does not say whether calling FreePages() with a
>>> zero address is allowed so let's be cautious and use cfg.need_to_free
>>> properly.
>>
>> Well, no, this paragraph makes no sense. Of course this is allowed, but
>> not as no-op behavior (like free(NULL) would be), but to free memory
>> starting at 0.
> 
> Fair enough. This paragraph could simply be dropped then.
> 
>>
>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> This pretty clearly wants a Fixes: tag, or maybe it even needs to be two.
>> I've checked the original code in 4.2, and things were consistent there,
>> afaics. So breakage was introduced perhaps in one or two of the many
>> re-works.
>>
> 
> Fixes: 8a71d50ed40b ("efi: Enable booting unified
> hypervisor/kernel/initrd images")
> Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free")
> 
> Do you want an updated patch or can these tweaks be done while committing?

If the maintainers have no other requests that require a v2, I'm sure this
can be done while committing.

Jan