Re: [PATCH 03/10] efi_loader: reject a PE section whose end wraps around

[Heinrich Schuchardt]

On 10/30/26 07:13, Ana Bobillo wrote:
From: Ana Bobillo <anabobil...@gmail.com>
efi_load_pe() computes the end of each section as the sum of its
VirtualAddress and its size, both u32, so the sum wraps around before it
is widened to unsigned long. A section near 4 GiB then yields a small
virt_size, and the memset() and memcpy() of the section write far past
the allocated image.

Relative virtual addresses are 32 bit, so reject a section whose end
does not fit into 32 bits. barebox, whose EFI loader is based on this
file, fixed the same in b6598389d46d ("efi: loader: fix integer overflow
in PE virt_size calculation").

This runs before the image is rejected when Secure Boot fails to verify
it.

Fixes: cb149c66342c ("efi_loader: Add PE image loader")
Signed-off-by: Ana Bobillo <anabobil...@gmail.com>
---
  lib/efi_loader/efi_image_loader.c | 15 +++++++++++++--
  1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c 
b/lib/efi_loader/efi_image_loader.c
index 35cf3cd268f..b15a662e5d6 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -1062,9 +1062,20 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj 
*handle,
        /* Calculate upper virtual address boundary */
        for (i = num_sections - 1; i >= 0; i--) {
                IMAGE_SECTION_HEADER *sec = &sections[i];
+               u32 end;
  
-		virt_size = max_t(unsigned long, virt_size,
-                                 sec->VirtualAddress + section_size(sec));
+               /*
+                * Relative virtual addresses are 32 bit. A section whose end
+                * wraps around would make virt_size too small for the memset()
+                * and memcpy() below.

We use  unsigned long virt_size. So wrapping around may not occur in the 
original code on 64bit systems. I think the comment should be tweaked a bit.

+                */
+               if (__builtin_add_overflow(sec->VirtualAddress,
+                                          section_size(sec), &end)) {
+                       log_err("Section %d virtual address overflow\n", i);
+                       return EFI_LOAD_ERROR;
+               }
+
+               virt_size = max_t(unsigned long, virt_size, end);

The comment above says "Calculate upper virtual address boundary".
The parameter name virt_size only makes sense for a variable holding 
(max(end[]) - BaseOfCode). We should probably fix that misnomer in a 
future patch.

Best regards

Heinrich

        }
  
  	/* Read 32/64bit specific header bits */