Re: [PATCH 1/1] xen/efi: Config parsing: Free the same page count as allocated.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2026 18:13, Bernhard Kaindl wrote:
> When reading the config file or the config section, we may need to append
> a terminating NUL byte to the config buffer if the last byte is not a
> control character.
> 
> This may cause an additional page to be allocated by efi_bs->AllocatePages()
> if the config file or config section size is a multiple of the page size.
> 
> For this case, increment file->size by one so the number of pages to be
> freed by efi_bs->FreePages() is the same as the number of pages allocated
> by efi_bs->AllocatePages() when the additional byte is allocated.
> 
> I moved the dcache flush after the NUL termination so the flushed range
> covers the final buffer contents.
> 
> I didn't add a dcache flush for the copied cfg buffer in read_section():
> that buffer is created by memcpy() and then consumed only by normal CPU
> reads in the EFI loader, so there is no non-coherent producer or other
> observer that would require cache maintenance.
> 
> Fixes: df75f77092c1 ("EFI: avoid OOB config file reads")
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>
> 
> ----
> PS: The same fix applies to its backport to 4.21 and other branches,
> for example:
> 
> stable-4.21:
> Fixes: feb99494bf59 ("EFI: avoid OOB config file reads")
> 
> stable-4.20:
> Fixes: 05b8f716aa32 ("EFI: avoid OOB config file reads")
> 
> PPS: A review using GPT-5.4 (just a data point for review) confirms what
> I've found by manual code inspection:
> 
>> The fix in boot.c:882-886 and boot.c:913-928 is consistent with the existing
>> free sites at boot.c:790-797, boot.c:1564-1567, and boot.c:1638-1641: Once 
>> the
>> config buffer gets an extra terminating byte, using the incremented size for
>> PFN_UP during FreePages is the right fix.
>>
>> I also checked the parser helpers at boot.c:584-641; they already operate on
>> a bounded buffer and treat NUL/control bytes as terminators, so the synthetic
>> extra byte does not create an obvious parsing regression.

To avoid something non-obvious was why I deliberately left file->size
un-incremented. But yes, I failed to recall that ->FreePages() takes a
size.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -858,7 +858,8 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>      what = L"Allocation";
>      file->addr = min(1UL << (32 + PAGE_SHIFT),
>                       HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
> -    /* For config files allocate an extra byte to put a NUL there. */
> +
> +    /* For config file buffers, allocate space for the terminating NUL byte 
> */
>      ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>                                  PFN_UP(size + (file == &cfg)), &file->addr);
>      if ( EFI_ERROR(ret) )

I don't really see why this or the other comment you alter would need
touching. But of course it'll be the EFI maintainers to judge.

> @@ -877,10 +878,12 @@ static bool __init read_file(EFI_FILE_HANDLE 
> dir_handle, CHAR16 *name,
>  
>      FileHandle->Close(FileHandle);
>  
> -    efi_arch_flush_dcache_area(file->ptr, file->size);
> -
>      if ( file == &cfg )
> -        file->str[file->size] = 0;
> +    {
> +        file->str[file->size] = 0; /* NUL-terminate the config data buffer. 
> */
> +        file->size += 1;           /* Free the same page count as allocated. 
> */

_If_ we want to go with incrementing file->size, then I don't see why
it can't be done using the increment operator. That specifically exists
as a shorthand for "+= 1".

As indicated above, I'm not convinced though that we really want to
alter file->size. I'd instead add 1 in the few instances of
"efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size))". Sadly the adding of 1
is conditional in read_section(), so maybe a "need_to_increment"
boolean wants adding next to the need_to_free one? Then a small helper
function may be warranted for those few instances of freeing cfg.addr.

> +    }
> +    efi_arch_flush_dcache_area(file->ptr, file->size);

A separating blank line wants maintaining here. But is this change
really needed at all? Besides being unrelated to the purpose of the
patch, the flushing - aiui - exists so that buffers handed on to
domains (Dom0) are in proper state. The config file isn't exposed to
Dom0, though. (Furthermore I question the effectiveness of this flush:
Speculation and prefetching can easily lead to the data being brought
back into the cache. Such flushing likely requires unmapping first,
and the concept of "unmapping" is a questionable one in the first
place while still running in boot services context. Cc-ing the Arm
maintainers for possible input.)

Jan