[PATCH v2 1/1] efi_loader: overflow in efi_allocate_pages

[Heinrich Schuchardt]

On 31.07.23 14:46, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Sun, 30 Jul 2023 at 13:51, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 32bit systems (pages << EFI_PAGE_SHIFT) may lead to an overflow which
>> does not occur in 64bit arithmetics.
> 
> You mean this cant happen in 32 bits but can in 64bit right?

'pages << EFI_PAGE_SHIFT' can overflow both on 32bit and on 64bit.

'(u64)pages << EFI_PAGE_SHIFT' can only overflow on 64bit.

Adding (u64) is the fix for 32bit.

> 
>>
>> An overflow of (pages << EFI_PAGE_SHIFT) on 64bit systems should be treated
>> as an error.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> v2:
>>          %s/size/sizeof/
>>   lib/efi_loader/efi_memory.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index e2ca78d935..9de6cf6010 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -487,7 +487,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>>                                  enum efi_memory_type memory_type,
>>                                  efi_uintn_t pages, uint64_t *memory)
>>   {
>> -       u64 len = pages << EFI_PAGE_SHIFT;
>> +       u64 len;
>>          efi_status_t ret;
>>          uint64_t addr;
>>
>> @@ -497,6 +497,11 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>>                  return EFI_INVALID_PARAMETER;
>>          if (!memory)
>>                  return EFI_INVALID_PARAMETER;
>> +       len = (u64)pages << EFI_PAGE_SHIFT;
>> +       /* Catch possible overflow on 64bit systems */
>> +       if (sizeof(efi_uintn_t) == sizeof(u64) &&
>> +           (len >> EFI_PAGE_SHIFT) != (u64)pages)
>> +               return EFI_OUT_OF_RESOURCES;
> 
> EFI_INVALID_PARAMETER is better here, since no allocation actually takes place.

The overflow occurs if a user requested more pages than fit into the 
64bit address space.

According to the UEFI 2.10 EFI_OUT_OF_RESOURCES is to be used for all 
cases where "The pages could not be allocated".

EFI_INVALID_PARAMETER is only foreseen for very specific exceptions.

Best regards

Heinrich

> 
>>
>>          switch (type) {
>>          case EFI_ALLOCATE_ANY_PAGES:
>> --
>> 2.40.1
>>
> 
> Regards
> /Ilias