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

[Ilias Apalodimas]

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?

>
> 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.

>
>         switch (type) {
>         case EFI_ALLOCATE_ANY_PAGES:
> --
> 2.40.1
>

Regards
/Ilias