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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.04.2026 08:48, Jan Beulich wrote:
> 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.

Actually, no separate flag looks to be needed. If for the cfg file we set
->need_to_free, we know we allocated the one extra byte.

Jan