[PATCH v2 1/1] efi_loader: overflow in efi_allocate_pages
On 32bit systems (pages << EFI_PAGE_SHIFT) may lead to an overflow which does not occur in 64bit arithmetics.
An overflow of (pages << EFI_PAGE_SHIFT) on 64bit systems should be treated as an error.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@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;
switch (type) { case EFI_ALLOCATE_ANY_PAGES:
Hi Heinrich,
On Sun, 30 Jul 2023 at 13:51, Heinrich Schuchardt heinrich.schuchardt@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@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
On 31.07.23 14:46, Ilias Apalodimas wrote:
Hi Heinrich,
On Sun, 30 Jul 2023 at 13:51, Heinrich Schuchardt heinrich.schuchardt@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@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
participants (2)
-
Heinrich Schuchardt -
Ilias Apalodimas