[PATCH 00/10] efi_loader: check PE images before using them

[PATCH 00/10] efi_loader: check PE images before using them

[Ana Bobillo]

U-Boot takes the offsets and sizes of a PE/COFF image from its headers
and uses them before it knows whether the image may run. With Secure
Boot on, efi_load_pe() authenticates the image first but goes on
copying and relocating it when authentication fails, and only then
returns EFI_SECURITY_VIOLATION; efi_image_parse(), which collects the
regions to hash, runs before the signature is checked, and so does the
walk of the certificate table. TCG2 measured boot parses every image as
well. So an image that will be refused, or one that is only measured,
can make U-Boot read or write outside its buffers, or hang.

This series checks those fields against the image where they are read,
along the lines of EDK2's PeCoffLoaderLib and DxeImageVerificationLib:

- patch 1 has efi_check_pe() check for a 64bit NT header again, as its
  comment says;
- patch 2 checks the headers, the sections and the certificate table in
  efi_image_parse(), and fails on errors of efi_image_region_add();
- patches 3-5 have efi_load_pe() reject, before allocating the image, a
  section whose end wraps around, section data past the end of the file
  and headers that do not fit into the image;
- patch 6 checks the relocation table against the image;
- patch 7 rejects a certificate table holding a certificate shorter than
  its header, which made the walk loop forever, or longer than the rest
  of the table;
- patch 8 refuses to load an image whose TCG2 measurement failed, as
  EDK2 does, instead of starting it unmeasured;
- patches 9 and 10 add unit tests and a Secure Boot test.

barebox, whose EFI loader is based on this file, made several of the
same changes this year; patches 2, 3 and 4 follow them and say so.

Valid images are unaffected: the unit tests check PE32 and PE32+ images
with 16, 6 or 5 data directories and an unpadded last section, the EFI
selftests and the Secure Boot tests pass, and each patch builds on its
own. Tested on sandbox.

CI: Azure build 13710 passed, all jobs, with this series on
https://github.com/u-boot/u-boot/pull/1064, a CI-only pull request,
merged with main 03e45557e1f:
https://dev.azure.com/u-boot/u-boot/_build/results?buildId=13710

Ana Bobillo (10):
  efi_loader: check the NT header size in efi_check_pe()
  efi_loader: check the PE image ranges in efi_image_parse()
  efi_loader: reject a PE section whose end wraps around
  efi_loader: check PE section raw data against the image size
  efi_loader: check that the PE headers fit into the loaded image
  efi_loader: check the relocation table against the image size
  efi_loader: check certificate lengths in efi_image_authenticate()
  efi_loader: do not load an image that cannot be measured
  test: lib: add tests for the PE image checks
  test: py: test invalid certificate lengths in signed images

 lib/efi_loader/efi_image_loader.c             | 368 +++++++++++----
 test/lib/Makefile                             |   2 +-
 test/lib/efi_image_loader.c                   | 442 ++++++++++++++++++
 test/py/tests/test_efi_secboot/conftest.py    |  31 ++
 test/py/tests/test_efi_secboot/test_signed.py |  36 ++
 5 files changed, 781 insertions(+), 98 deletions(-)
 create mode 100644 test/lib/efi_image_loader.c


base-commit: 03e45557e1f38a39a0c80fe30483130ff376cf92

[PATCH 01/10] efi_loader: check the NT header size in efi_check_pe()

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_check_pe() is meant to check that the image holds a 64bit NT header,
which is longer than a 32bit one, as its comment says. Since commit
5dad05a0e61d ("efi_loader: carve out efi_check_pe()") it checks for a
32bit one only, so the callers read the optional header fields and data
directories of a PE32+ image past the end of a truncated image. The
addition of e_lfanew can also wrap around on 32bit systems.

Check for the size of the 64bit NT header again, without wrapping.

Fixes: 5dad05a0e61d ("efi_loader: carve out efi_check_pe()")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f9a2d2df405..e31493d5135 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -849,7 +849,8 @@ efi_status_t efi_check_pe(void *buffer, size_t size, void **nt_header)
 	 * least one section header follows we only need to check for the length
 	 * of the 64bit header which is longer than the 32bit header.
 	 */
-	if (size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32))
+	if (dos->e_lfanew > size ||
+	    size - dos->e_lfanew < sizeof(IMAGE_NT_HEADERS64))
 		return EFI_INVALID_PARAMETER;
 	nt = (IMAGE_NT_HEADERS32 *)((u8 *)buffer + dos->e_lfanew);
 

Re: [PATCH 01/10] efi_loader: check the NT header size in efi_check_pe()

[Ilias Apalodimas]

On Fri, 30 Oct 2026 at 08:13, Ana Bobillo <anabobillo3@gmail.com> wrote:
>
> From: Ana Bobillo <anabobillo3@gmail.com>
>
> efi_check_pe() is meant to check that the image holds a 64bit NT header,
> which is longer than a 32bit one, as its comment says. Since commit
> 5dad05a0e61d ("efi_loader: carve out efi_check_pe()") it checks for a
> 32bit one only, so the callers read the optional header fields and data
> directories of a PE32+ image past the end of a truncated image. The
> addition of e_lfanew can also wrap around on 32bit systems.
>
> Check for the size of the 64bit NT header again, without wrapping.
>
> Fixes: 5dad05a0e61d ("efi_loader: carve out efi_check_pe()")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
> ---
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>


>  lib/efi_loader/efi_image_loader.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f9a2d2df405..e31493d5135 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -849,7 +849,8 @@ efi_status_t efi_check_pe(void *buffer, size_t size, void **nt_header)
>          * least one section header follows we only need to check for the length
>          * of the 64bit header which is longer than the 32bit header.
>          */
> -       if (size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32))
> +       if (dos->e_lfanew > size ||
> +           size - dos->e_lfanew < sizeof(IMAGE_NT_HEADERS64))
>                 return EFI_INVALID_PARAMETER;
>         nt = (IMAGE_NT_HEADERS32 *)((u8 *)buffer + dos->e_lfanew);
>

Re: [PATCH 01/10] efi_loader: check the NT header size in efi_check_pe()

[Heinrich Schuchardt]

On 10/30/26 07:13, Ana Bobillo wrote:
> From: Ana Bobillo <anabobillo3@gmail.com>
> 
> efi_check_pe() is meant to check that the image holds a 64bit NT header,
> which is longer than a 32bit one, as its comment says. Since commit
> 5dad05a0e61d ("efi_loader: carve out efi_check_pe()") it checks for a
> 32bit one only, so the callers read the optional header fields and data
> directories of a PE32+ image past the end of a truncated image. The
> addition of e_lfanew can also wrap around on 32bit systems.
> 
> Check for the size of the 64bit NT header again, without wrapping.
> 
> Fixes: 5dad05a0e61d ("efi_loader: carve out efi_check_pe()")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index f9a2d2df405..e31493d5135 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -849,7 +849,8 @@ efi_status_t efi_check_pe(void *buffer, size_t size, void **nt_header)
>   	 * least one section header follows we only need to check for the length
>   	 * of the 64bit header which is longer than the 32bit header.
>   	 */
> -	if (size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32))
> +	if (dos->e_lfanew > size ||
> +	    size - dos->e_lfanew < sizeof(IMAGE_NT_HEADERS64))
>   		return EFI_INVALID_PARAMETER;
>   	nt = (IMAGE_NT_HEADERS32 *)((u8 *)buffer + dos->e_lfanew);
>   

Thank you for the fix which looks correct.

We continue to use

IMAGE_NT_HEADERS32 *nt;

though we check against IMAGE_NT_HEADERS64.

Shouldn't we be more consistent and use the same type everywhere in the 
function?

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH 01/10] efi_loader: check the NT header size in efi_check_pe()

[Ana Bobillo]

Hi Heinrich,

On 10/30/26 12:15, Heinrich Schuchardt wrote:
> We continue to use
>
> IMAGE_NT_HEADERS32 *nt;
>
> though we check against IMAGE_NT_HEADERS64.
>
> Shouldn't we be more consistent and use the same type everywhere in the
> function?

Yes. efi_check_pe() only reads the signature, which sits at the same
offset in both, so in v2 I'll declare nt as IMAGE_NT_HEADERS64 * and
check the size against sizeof(*nt).

Thanks for the review,
Ana

[PATCH 02/10] efi_loader: check the PE image ranges in efi_image_parse()

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_image_parse() collects the regions of an image to be hashed for
Secure Boot and for TCG2 measurements. It takes the header size, the
section table, each section's raw data and the certificate table from
the image without checking them against the image size:

- SizeOfHeaders and each section's PointerToRawData and SizeOfRawData
  are added as regions even when they run past the end of the image,
  which is then read out of bounds when the regions are hashed;
- the section table is read wherever SizeOfOptionalHeader and
  NumberOfSections put it;
- the end of the certificate table is computed in 32 bits, so a large
  offset wraps around and passes the check;
- errors of efi_image_region_add() are ignored.

All of this happens before the signature is verified.

Call efi_check_pe() first, check each offset and size against the image
size without wrapping, add regions only through a checked helper and fail
on its errors, and require the file alignment to be a power of two. The
padding of a section to the file alignment is hashed only as far as the
image goes, so an image whose last section is not padded is still
accepted. Check the certificate table against the size of
WIN_CERTIFICATE, not of a pointer to it.

barebox, whose EFI loader is based on this file, added the same checks in
4ec1f7166778 ("efi: validate PE parser input ranges"); this is a port to
U-Boot. Unlike there, an optional header with fewer than 16 data
directories is still accepted if it holds the certificate table entry it
declares: U-Boot's own EFI applications have six.

Fixes: 4540dabdcaca ("efi_loader: image_loader: support image authentication")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 268 +++++++++++++++++++++---------
 1 file changed, 190 insertions(+), 78 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index e31493d5135..35cf3cd268f 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -393,6 +393,69 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
 	}
 }
 
+/**
+ * pe_range_ok() - check that a range lies within the image
+ * @len:	size of the image
+ * @off:	offset of the range from the start of the image
+ * @size:	size of the range
+ *
+ * Return:	true if [@off, @off + @size) lies within [0, @len)
+ */
+static bool pe_range_ok(size_t len, size_t off, size_t size)
+{
+	return off <= len && size <= len - off;
+}
+
+/**
+ * pe_align_size() - round up a section size to the file alignment
+ * @size:	size of the raw data of a section
+ * @align:	file alignment, a power of two
+ * @aligned:	on return @size rounded up to @align
+ *
+ * Return:	true on success, false if @align is not a power of two or
+ *		the rounded size overflows
+ */
+static bool pe_align_size(u32 size, u32 align, size_t *aligned)
+{
+	size_t val = size;
+
+	if (!align || (align & (align - 1)))
+		return false;
+
+	if (__builtin_add_overflow(val, (size_t)align - 1, &val))
+		return false;
+
+	*aligned = val & ~((size_t)align - 1);
+
+	return true;
+}
+
+/**
+ * efi_image_region_add_checked() - add a region of an image
+ * @regs:	pointer to array of regions
+ * @efi:	pointer to the image
+ * @len:	size of the image
+ * @start:	offset of the region in the image
+ * @size:	size of the region
+ * @nocheck:	flag against overlapped regions
+ *
+ * Add the region [@start, @start + @size[ of the image to @regs if it lies
+ * within the image.
+ *
+ * Return:	status code
+ */
+static efi_status_t efi_image_region_add_checked(struct efi_image_regions *regs,
+						 const void *efi, size_t len,
+						 size_t start, size_t size,
+						 int nocheck)
+{
+	if (!pe_range_ok(len, start, size))
+		return EFI_INVALID_PARAMETER;
+
+	return efi_image_region_add(regs, efi + start, efi + start + size,
+				    nocheck);
+}
+
 /**
  * efi_image_parse() - parse a PE image
  * @efi:	Pointer to image
@@ -401,8 +464,8 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
  * @auth:	Pointer to a pointer to authentication data in PE
  * @auth_len:	Size of @auth
  *
- * Parse image binary in PE32(+) format, assuming that sanity of PE image
- * has been checked by a caller.
+ * Parse image binary in PE32(+) format. Each offset and size taken from the
+ * image is checked against @len before it is used.
  * On success, an address of authentication data in @efi and its size will
  * be returned in @auth and @auth_len, respectively.
  *
@@ -411,97 +474,135 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
 bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		     WIN_CERTIFICATE **auth, size_t *auth_len)
 {
-	struct efi_image_regions *regs;
-	IMAGE_DOS_HEADER *dos;
+	struct efi_image_regions *regs = NULL;
 	IMAGE_NT_HEADERS32 *nt;
-	IMAGE_SECTION_HEADER *sections, **sorted;
+	IMAGE_SECTION_HEADER *sections, **sorted = NULL;
 	int num_regions, num_sections, i;
 	int ctidx = IMAGE_DIRECTORY_ENTRY_SECURITY;
-	u32 align, size, authsz, authoff;
-	size_t bytes_hashed;
+	u32 align, authsz, authoff, header_size, num_dirs;
+	size_t bytes_hashed, opt_off, opt_size, min_opt_size;
+	size_t csum_off, subsys_off, dir_off, dir_end;
+	size_t sections_off, sections_size;
 
-	dos = (void *)efi;
-	nt = (void *)(efi + dos->e_lfanew);
 	authoff = 0;
 	authsz = 0;
 
 	/*
-	 * Count maximum number of regions to be digested.
-	 * We don't have to have an exact number here.
-	 * See efi_image_region_add()'s in parsing below.
+	 * efi_check_pe() checks the DOS and PE signatures and that the image
+	 * is large enough to hold a 64bit NT header, which is longer than a
+	 * 32bit one. Every optional header field and data directory entry
+	 * read below therefore lies within the image.
 	 */
-	num_regions = 3; /* for header */
-	num_regions += nt->FileHeader.NumberOfSections;
-	num_regions++; /* for extra */
-
-	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
-		      1);
-	if (!regs)
-		goto err;
-	regs->max = num_regions;
+	if (efi_check_pe(efi, len, (void **)&nt) != EFI_SUCCESS)
+		return false;
+	opt_off = (u8 *)&nt->OptionalHeader - (u8 *)efi;
+	opt_size = nt->FileHeader.SizeOfOptionalHeader;
 
-	/*
-	 * Collect data regions for hash calculation
-	 * 1. File headers
-	 */
 	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
 		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
 		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
 
-		/* Skip CheckSum */
-		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
-		if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
-			efi_image_region_add(regs,
-					     &opt->Subsystem,
-					     efi + opt->SizeOfHeaders, 0);
-		} else {
-			/* Skip Certificates Table */
-			efi_image_region_add(regs,
-					     &opt->Subsystem,
-					     &opt->DataDirectory[ctidx], 0);
-			efi_image_region_add(regs,
-					     &opt->DataDirectory[ctidx] + 1,
-					     efi + opt->SizeOfHeaders, 0);
-
+		min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER64, DataDirectory);
+		csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+					      CheckSum);
+		subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+						Subsystem);
+		dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+					     DataDirectory[ctidx]);
+		header_size = opt->SizeOfHeaders;
+		align = opt->FileAlignment;
+		num_dirs = opt->NumberOfRvaAndSizes;
+		if (num_dirs > ctidx) {
 			authoff = opt->DataDirectory[ctidx].VirtualAddress;
 			authsz = opt->DataDirectory[ctidx].Size;
 		}
-
-		bytes_hashed = opt->SizeOfHeaders;
-		align = opt->FileAlignment;
 	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
 
-		/* Skip CheckSum */
-		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
-		if (nt->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
-			efi_image_region_add(regs,
-					     &opt->Subsystem,
-					     efi + opt->SizeOfHeaders, 0);
-		} else {
-			/* Skip Certificates Table */
-			efi_image_region_add(regs, &opt->Subsystem,
-					     &opt->DataDirectory[ctidx], 0);
-			efi_image_region_add(regs,
-					     &opt->DataDirectory[ctidx] + 1,
-					     efi + opt->SizeOfHeaders, 0);
-
+		min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory);
+		csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+					      CheckSum);
+		subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+						Subsystem);
+		dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+					     DataDirectory[ctidx]);
+		header_size = opt->SizeOfHeaders;
+		align = opt->FileAlignment;
+		num_dirs = opt->NumberOfRvaAndSizes;
+		if (num_dirs > ctidx) {
 			authoff = opt->DataDirectory[ctidx].VirtualAddress;
 			authsz = opt->DataDirectory[ctidx].Size;
 		}
-
-		bytes_hashed = opt->SizeOfHeaders;
-		align = opt->FileAlignment;
 	} else {
 		log_err("%s: Invalid optional header magic %x\n", __func__,
 			nt->OptionalHeader.Magic);
+		return false;
+	}
+	dir_end = dir_off + sizeof(IMAGE_DATA_DIRECTORY);
+
+	/*
+	 * The optional header must hold the fields used here, and the
+	 * certificate table entry when there is one. The section table
+	 * follows the optional header; it and the whole header area
+	 * (SizeOfHeaders) must lie within the image, as must the certificate
+	 * table.
+	 */
+	num_sections = nt->FileHeader.NumberOfSections;
+	sections_off = opt_off + opt_size;
+	sections_size = (size_t)num_sections * sizeof(IMAGE_SECTION_HEADER);
+	if (opt_size < min_opt_size ||
+	    (num_dirs > ctidx && sections_off < dir_end) ||
+	    !pe_range_ok(len, sections_off, sections_size) ||
+	    !pe_range_ok(len, 0, header_size) ||
+	    header_size < sections_off + sections_size) {
+		log_err("%s: Invalid PE headers\n", __func__);
+		return false;
+	}
+	if (authsz && !pe_range_ok(len, authoff, authsz)) {
+		log_err("%s: Size for auth too large: %u\n", __func__, authsz);
+		return false;
+	}
+
+	/*
+	 * Count maximum number of regions to be digested.
+	 * We don't have to have an exact number here.
+	 * See efi_image_region_add()'s in parsing below.
+	 */
+	num_regions = 3; /* for header */
+	num_regions += num_sections;
+	num_regions++; /* for extra */
+
+	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
+		      1);
+	if (!regs)
 		goto err;
+	regs->max = num_regions;
+
+	/*
+	 * Collect data regions for hash calculation
+	 * 1. File headers
+	 */
+	/* Skip CheckSum */
+	if (efi_image_region_add_checked(regs, efi, len, 0, csum_off, 0))
+		goto err;
+	if (num_dirs <= ctidx) {
+		if (efi_image_region_add_checked(regs, efi, len, subsys_off,
+						 header_size - subsys_off, 0))
+			goto err;
+	} else {
+		/* Skip Certificates Table */
+		if (efi_image_region_add_checked(regs, efi, len, subsys_off,
+						 dir_off - subsys_off, 0))
+			goto err;
+		if (efi_image_region_add_checked(regs, efi, len, dir_end,
+						 header_size - dir_end, 0))
+			goto err;
 	}
 
+	bytes_hashed = header_size;
+
 	/* 2. Sections */
-	num_sections = nt->FileHeader.NumberOfSections;
-	sections = (void *)((uint8_t *)&nt->OptionalHeader +
-			    nt->FileHeader.SizeOfOptionalHeader);
+	sections = (void *)((uint8_t *)efi + sections_off);
 	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
 	if (!sorted) {
 		log_err("%s: Out of memory\n", __func__);
@@ -516,14 +617,26 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 	qsort(sorted, num_sections, sizeof(sorted[0]), cmp_pe_section);
 
 	for (i = 0; i < num_sections; i++) {
+		size_t size;
+
 		if (!sorted[i]->SizeOfRawData)
 			continue;
 
-		size = (sorted[i]->SizeOfRawData + align - 1) & ~(align - 1);
-		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
-				     efi + sorted[i]->PointerToRawData + size,
-				     0);
-		log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
+		if (!pe_range_ok(len, sorted[i]->PointerToRawData,
+				 sorted[i]->SizeOfRawData) ||
+		    !pe_align_size(sorted[i]->SizeOfRawData, align, &size)) {
+			log_err("%s: Invalid section %d\n", __func__, i);
+			goto err;
+		}
+		/* The padding to the file alignment ends with the image */
+		size = min(size, len - sorted[i]->PointerToRawData);
+		if (efi_image_region_add_checked(regs, efi, len,
+						 sorted[i]->PointerToRawData,
+						 size, 0)) {
+			log_err("%s: Invalid section %d\n", __func__, i);
+			goto err;
+		}
+		log_debug("section[%d](%s): raw: 0x%x-0x%zx, virt: %x-%x\n",
 			  i, sorted[i]->Name,
 			  sorted[i]->PointerToRawData,
 			  sorted[i]->PointerToRawData + size,
@@ -534,25 +647,23 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		bytes_hashed += size;
 	}
 	free(sorted);
+	sorted = NULL;
 
 	/* 3. Extra data excluding Certificates Table */
-	if (bytes_hashed + authsz < len) {
+	if (bytes_hashed < len && authsz < len - bytes_hashed) {
 		log_debug("extra data for hash: %zu\n",
 			  len - (bytes_hashed + authsz));
-		efi_image_region_add(regs, efi + bytes_hashed,
-				     efi + len - authsz, 0);
+		if (efi_image_region_add_checked(regs, efi, len, bytes_hashed,
+						 len - bytes_hashed - authsz,
+						 0))
+			goto err;
 	}
 
 	/* Return Certificates Table */
 	if (authsz) {
-		if (len < authoff + authsz) {
-			log_err("%s: Size for auth too large: %u >= %zu\n",
-				__func__, authsz, len - authoff);
-			goto err;
-		}
-		if (authsz < sizeof(*auth)) {
+		if (authsz < sizeof(**auth)) {
 			log_err("%s: Size for auth too small: %u < %zu\n",
-				__func__, authsz, sizeof(*auth));
+				__func__, authsz, sizeof(**auth));
 			goto err;
 		}
 		*auth = efi + authoff;
@@ -569,6 +680,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 	return true;
 
 err:
+	free(sorted);
 	free(regs);
 
 	return false;

Re: [PATCH 02/10] efi_loader: check the PE image ranges in efi_image_parse()

[Heinrich Schuchardt]

On 10/30/26 07:13, Ana Bobillo wrote:
> From: Ana Bobillo <anabobillo3@gmail.com>
> 
> efi_image_parse() collects the regions of an image to be hashed for
> Secure Boot and for TCG2 measurements. It takes the header size, the
> section table, each section's raw data and the certificate table from
> the image without checking them against the image size:
> 
> - SizeOfHeaders and each section's PointerToRawData and SizeOfRawData
>    are added as regions even when they run past the end of the image,
>    which is then read out of bounds when the regions are hashed;
> - the section table is read wherever SizeOfOptionalHeader and
>    NumberOfSections put it;
> - the end of the certificate table is computed in 32 bits, so a large
>    offset wraps around and passes the check;
> - errors of efi_image_region_add() are ignored.
> 
> All of this happens before the signature is verified.
> 
> Call efi_check_pe() first, check each offset and size against the image
> size without wrapping, add regions only through a checked helper and fail
> on its errors, and require the file alignment to be a power of two. The
> padding of a section to the file alignment is hashed only as far as the
> image goes, so an image whose last section is not padded is still
> accepted. Check the certificate table against the size of
> WIN_CERTIFICATE, not of a pointer to it.
> 
> barebox, whose EFI loader is based on this file, added the same checks in
> 4ec1f7166778 ("efi: validate PE parser input ranges"); this is a port to
> U-Boot. Unlike there, an optional header with fewer than 16 data
> directories is still accepted if it holds the certificate table entry it
> declares: U-Boot's own EFI applications have six.

The PECOFF specification does not require any table. Six are needed if 
you have a .reloc section which some non-compliant software assumes. 
Aren't you describing a bug in barebox?

read_header() in 
https://github.com/rhboot/shim/blob/e794a714339b1277abe9f890fc98de6e415681b0/pe-relocate.c#L376 
has rather strict tests for PE files. It might be worthwhile to compare 
their checks with ours.

> 
> Fixes: 4540dabdcaca ("efi_loader: image_loader: support image authentication")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 268 +++++++++++++++++++++---------
>   1 file changed, 190 insertions(+), 78 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index e31493d5135..35cf3cd268f 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -393,6 +393,69 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
>   	}
>   }
>   
> +/**
> + * pe_range_ok() - check that a range lies within the image
> + * @len:	size of the image
> + * @off:	offset of the range from the start of the image
> + * @size:	size of the range
> + *
> + * Return:	true if [@off, @off + @size) lies within [0, @len)
> + */
> +static bool pe_range_ok(size_t len, size_t off, size_t size)
> +{
> +	return off <= len && size <= len - off;
> +}
> +
> +/**
> + * pe_align_size() - round up a section size to the file alignment
> + * @size:	size of the raw data of a section
> + * @align:	file alignment, a power of two
> + * @aligned:	on return @size rounded up to @align
> + *
> + * Return:	true on success, false if @align is not a power of two or
> + *		the rounded size overflows
> + */
> +static bool pe_align_size(u32 size, u32 align, size_t *aligned)
> +{
> +	size_t val = size;
> +
> +	if (!align || (align & (align - 1)))
> +		return false;

FileAlignment and SectionAlignment should be checked once before the 
loop calling this function instead of checking the same value multiple 
times.

> +
> +	if (__builtin_add_overflow(val, (size_t)align - 1, &val))
> +		return false;

We are already checking for overflows in pe_range_ok(). Why use 
__builtin_add_overflow()?

> +
> +	*aligned = val & ~((size_t)align - 1);

This whole function can be replaced by using the ALIGN() macro.

> +
> +	return true;
> +}
> +
> +/**
> + * efi_image_region_add_checked() - add a region of an image
> + * @regs:	pointer to array of regions
> + * @efi:	pointer to the image
> + * @len:	size of the image
> + * @start:	offset of the region in the image
> + * @size:	size of the region
> + * @nocheck:	flag against overlapped regions
> + *
> + * Add the region [@start, @start + @size[ of the image to @regs if it lies
> + * within the image.
> + *
> + * Return:	status code
> + */
> +static efi_status_t efi_image_region_add_checked(struct efi_image_regions *regs,
> +						 const void *efi, size_t len,
> +						 size_t start, size_t size,
> +						 int nocheck)
> +{
> +	if (!pe_range_ok(len, start, size))
> +		return EFI_INVALID_PARAMETER;
> +
> +	return efi_image_region_add(regs, efi + start, efi + start + size,
> +				    nocheck);
> +}
> +
>   /**
>    * efi_image_parse() - parse a PE image
>    * @efi:	Pointer to image
> @@ -401,8 +464,8 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
>    * @auth:	Pointer to a pointer to authentication data in PE
>    * @auth_len:	Size of @auth
>    *
> - * Parse image binary in PE32(+) format, assuming that sanity of PE image
> - * has been checked by a caller.
> + * Parse image binary in PE32(+) format. Each offset and size taken from the
> + * image is checked against @len before it is used.
>    * On success, an address of authentication data in @efi and its size will
>    * be returned in @auth and @auth_len, respectively.
>    *
> @@ -411,97 +474,135 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
>   bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   		     WIN_CERTIFICATE **auth, size_t *auth_len)
>   {
> -	struct efi_image_regions *regs;
> -	IMAGE_DOS_HEADER *dos;
> +	struct efi_image_regions *regs = NULL;
>   	IMAGE_NT_HEADERS32 *nt;
> -	IMAGE_SECTION_HEADER *sections, **sorted;
> +	IMAGE_SECTION_HEADER *sections, **sorted = NULL;
>   	int num_regions, num_sections, i;
>   	int ctidx = IMAGE_DIRECTORY_ENTRY_SECURITY;
> -	u32 align, size, authsz, authoff;
> -	size_t bytes_hashed;
> +	u32 align, authsz, authoff, header_size, num_dirs;
> +	size_t bytes_hashed, opt_off, opt_size, min_opt_size;
> +	size_t csum_off, subsys_off, dir_off, dir_end;
> +	size_t sections_off, sections_size;
>   
> -	dos = (void *)efi;
> -	nt = (void *)(efi + dos->e_lfanew);
>   	authoff = 0;
>   	authsz = 0;
>   
>   	/*
> -	 * Count maximum number of regions to be digested.
> -	 * We don't have to have an exact number here.
> -	 * See efi_image_region_add()'s in parsing below.
> +	 * efi_check_pe() checks the DOS and PE signatures and that the image
> +	 * is large enough to hold a 64bit NT header, which is longer than a
> +	 * 32bit one. Every optional header field and data directory entry
> +	 * read below therefore lies within the image.
>   	 */
> -	num_regions = 3; /* for header */
> -	num_regions += nt->FileHeader.NumberOfSections;
> -	num_regions++; /* for extra */
> -
> -	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
> -		      1);
> -	if (!regs)
> -		goto err;
> -	regs->max = num_regions;
> +	if (efi_check_pe(efi, len, (void **)&nt) != EFI_SUCCESS)
> +		return false;
> +	opt_off = (u8 *)&nt->OptionalHeader - (u8 *)efi;
> +	opt_size = nt->FileHeader.SizeOfOptionalHeader;
>   
> -	/*
> -	 * Collect data regions for hash calculation
> -	 * 1. File headers
> -	 */
>   	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>   		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
>   		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>   
> -		/* Skip CheckSum */
> -		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
> -		if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
> -			efi_image_region_add(regs,
> -					     &opt->Subsystem,
> -					     efi + opt->SizeOfHeaders, 0);
> -		} else {
> -			/* Skip Certificates Table */
> -			efi_image_region_add(regs,
> -					     &opt->Subsystem,
> -					     &opt->DataDirectory[ctidx], 0);
> -			efi_image_region_add(regs,
> -					     &opt->DataDirectory[ctidx] + 1,
> -					     efi + opt->SizeOfHeaders, 0);
> -
> +		min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER64, DataDirectory);
> +		csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
> +					      CheckSum);
> +		subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
> +						Subsystem);
> +		dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
> +					     DataDirectory[ctidx]);
> +		header_size = opt->SizeOfHeaders;
> +		align = opt->FileAlignment;
> +		num_dirs = opt->NumberOfRvaAndSizes;
> +		if (num_dirs > ctidx) {
>   			authoff = opt->DataDirectory[ctidx].VirtualAddress;
>   			authsz = opt->DataDirectory[ctidx].Size;
>   		}
> -
> -		bytes_hashed = opt->SizeOfHeaders;
> -		align = opt->FileAlignment;
>   	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>   		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>   
> -		/* Skip CheckSum */
> -		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
> -		if (nt->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
> -			efi_image_region_add(regs,
> -					     &opt->Subsystem,
> -					     efi + opt->SizeOfHeaders, 0);
> -		} else {
> -			/* Skip Certificates Table */
> -			efi_image_region_add(regs, &opt->Subsystem,
> -					     &opt->DataDirectory[ctidx], 0);
> -			efi_image_region_add(regs,
> -					     &opt->DataDirectory[ctidx] + 1,
> -					     efi + opt->SizeOfHeaders, 0);
> -
> +		min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory);
> +		csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
> +					      CheckSum);
> +		subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
> +						Subsystem);
> +		dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
> +					     DataDirectory[ctidx]);
> +		header_size = opt->SizeOfHeaders;
> +		align = opt->FileAlignment;
> +		num_dirs = opt->NumberOfRvaAndSizes;
> +		if (num_dirs > ctidx) {
>   			authoff = opt->DataDirectory[ctidx].VirtualAddress;
>   			authsz = opt->DataDirectory[ctidx].Size;
>   		}
> -
> -		bytes_hashed = opt->SizeOfHeaders;
> -		align = opt->FileAlignment;
>   	} else {
>   		log_err("%s: Invalid optional header magic %x\n", __func__,
>   			nt->OptionalHeader.Magic);
> +		return false;
> +	}
> +	dir_end = dir_off + sizeof(IMAGE_DATA_DIRECTORY);
> +
> +	/*
> +	 * The optional header must hold the fields used here, and the
> +	 * certificate table entry when there is one. The section table
> +	 * follows the optional header; it and the whole header area
> +	 * (SizeOfHeaders) must lie within the image, as must the certificate
> +	 * table.
> +	 */
> +	num_sections = nt->FileHeader.NumberOfSections;
> +	sections_off = opt_off + opt_size;
> +	sections_size = (size_t)num_sections * sizeof(IMAGE_SECTION_HEADER);
> +	if (opt_size < min_opt_size ||
> +	    (num_dirs > ctidx && sections_off < dir_end) ||
> +	    !pe_range_ok(len, sections_off, sections_size) ||
> +	    !pe_range_ok(len, 0, header_size) ||
> +	    header_size < sections_off + sections_size) {
> +		log_err("%s: Invalid PE headers\n", __func__);
> +		return false;
> +	}
> +	if (authsz && !pe_range_ok(len, authoff, authsz)) {
> +		log_err("%s: Size for auth too large: %u\n", __func__, authsz);
> +		return false;
> +	}
> +
> +	/*
> +	 * Count maximum number of regions to be digested.
> +	 * We don't have to have an exact number here.
> +	 * See efi_image_region_add()'s in parsing below.
> +	 */
> +	num_regions = 3; /* for header */
> +	num_regions += num_sections;
> +	num_regions++; /* for extra */

Please, avoid this increment by starting at 4.

> +
> +	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
> +		      1);
> +	if (!regs)
>   		goto err;
> +	regs->max = num_regions;
> +
> +	/*
> +	 * Collect data regions for hash calculation
> +	 * 1. File headers
> +	 */
> +	/* Skip CheckSum */
> +	if (efi_image_region_add_checked(regs, efi, len, 0, csum_off, 0))
> +		goto err;
> +	if (num_dirs <= ctidx) {
> +		if (efi_image_region_add_checked(regs, efi, len, subsys_off,
> +						 header_size - subsys_off, 0))
> +			goto err;
> +	} else {
> +		/* Skip Certificates Table */
> +		if (efi_image_region_add_checked(regs, efi, len, subsys_off,
> +						 dir_off - subsys_off, 0))
> +			goto err;
> +		if (efi_image_region_add_checked(regs, efi, len, dir_end,
> +						 header_size - dir_end, 0))
> +			goto err;
>   	}
>   
> +	bytes_hashed = header_size;
> +
>   	/* 2. Sections */
> -	num_sections = nt->FileHeader.NumberOfSections;
> -	sections = (void *)((uint8_t *)&nt->OptionalHeader +
> -			    nt->FileHeader.SizeOfOptionalHeader);
> +	sections = (void *)((uint8_t *)efi + sections_off);
>   	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
>   	if (!sorted) {
>   		log_err("%s: Out of memory\n", __func__);
> @@ -516,14 +617,26 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   	qsort(sorted, num_sections, sizeof(sorted[0]), cmp_pe_section);
>   
>   	for (i = 0; i < num_sections; i++) {
> +		size_t size;
> +
>   		if (!sorted[i]->SizeOfRawData)
>   			continue;
>   
> -		size = (sorted[i]->SizeOfRawData + align - 1) & ~(align - 1);
> -		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
> -				     efi + sorted[i]->PointerToRawData + size,
> -				     0);
> -		log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> +		if (!pe_range_ok(len, sorted[i]->PointerToRawData,
> +				 sorted[i]->SizeOfRawData) ||
> +		    !pe_align_size(sorted[i]->SizeOfRawData, align, &size)) {

The PECOFF specification defines a default of 512 for the FileAlignment. 
That is the value you should use if FileAlignment == 0.

> +			log_err("%s: Invalid section %d\n", __func__, i);
> +			goto err;
> +		}
> +		/* The padding to the file alignment ends with the image */
> +		size = min(size, len - sorted[i]->PointerToRawData);
> +		if (efi_image_region_add_checked(regs, efi, len,
> +						 sorted[i]->PointerToRawData,
> +						 size, 0)) {
> +			log_err("%s: Invalid section %d\n", __func__, i);
> +			goto err;
> +		}
> +		log_debug("section[%d](%s): raw: 0x%x-0x%zx, virt: %x-%x\n",
>   			  i, sorted[i]->Name,
>   			  sorted[i]->PointerToRawData,
>   			  sorted[i]->PointerToRawData + size,
> @@ -534,25 +647,23 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   		bytes_hashed += size;
>   	}
>   	free(sorted);
> +	sorted = NULL;
>   
>   	/* 3. Extra data excluding Certificates Table */
> -	if (bytes_hashed + authsz < len) {
> +	if (bytes_hashed < len && authsz < len - bytes_hashed) {
>   		log_debug("extra data for hash: %zu\n",
>   			  len - (bytes_hashed + authsz));
> -		efi_image_region_add(regs, efi + bytes_hashed,
> -				     efi + len - authsz, 0);
> +		if (efi_image_region_add_checked(regs, efi, len, bytes_hashed,
> +						 len - bytes_hashed - authsz,
> +						 0))
> +			goto err;
>   	}
>   
>   	/* Return Certificates Table */
>   	if (authsz) {
> -		if (len < authoff + authsz) {
> -			log_err("%s: Size for auth too large: %u >= %zu\n",
> -				__func__, authsz, len - authoff);
> -			goto err;
> -		}
> -		if (authsz < sizeof(*auth)) {
> +		if (authsz < sizeof(**auth)) {
>   			log_err("%s: Size for auth too small: %u < %zu\n",
> -				__func__, authsz, sizeof(*auth));
> +				__func__, authsz, sizeof(**auth));
>   			goto err;
>   		}
>   		*auth = efi + authoff;
> @@ -569,6 +680,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   	return true;
>   
>   err:
> +	free(sorted);

Here you are closing a memory leak. But this issue is not described in 
the commit message.

Ideally for each issue there would be a separate patch. But at least 
mention it in the commit message.

Best regards

Heinrich

>   	free(regs);
>   
>   	return false;

Re: [PATCH 02/10] efi_loader: check the PE image ranges in efi_image_parse()

[Ana Bobillo]

Hi Heinrich,

On 10/30/26 13:14, Heinrich Schuchardt wrote:
> The PECOFF specification does not require any table. Six are needed if
> you have a .reloc section which some non-compliant software assumes.
> Aren't you describing a bug in barebox?

No, that sentence is wrong, sorry. barebox's optional header struct
does not include the data directories, so its size check accepts fewer
than 16 as well. I'll drop the sentence in v2.

> read_header() in
> https://github.com/rhboot/shim/blob/e794a714.../pe-relocate.c#L376
> has rather strict tests for PE files. It might be worthwhile to compare
> their checks with ours.

I went through it. shim has one check the series lacks, and EDK2's
PeCoffLoaderGetPeHeader() has it too: NumberOfRvaAndSizes must be at
most 16, and SizeOfOptionalHeader must hold exactly that many entries.
v2 checks that instead of only checking that the certificate table
entry fits. The rest is covered by the series, is loader policy
(stripped relocations, NX), concerns SizeOfImage, which U-Boot doesn't
use, or, like requiring e_lfanew to point past the DOS header, rejects
odd images that do no harm here.

> FileAlignment and SectionAlignment should be checked once before the
> loop calling this function instead of checking the same value multiple
> times.

OK. efi_image_parse() only uses FileAlignment. SectionAlignment is only
used in efi_load_pe(), where efi_alloc_aligned_pages() already refuses
one that is not a power of two.

> We are already checking for overflows in pe_range_ok(). Why use
> __builtin_add_overflow()?

pe_range_ok() checks SizeOfRawData before it is rounded up, and the
rounding could still wrap where size_t is 32 bits.

> This whole function can be replaced by using the ALIGN() macro.

Yes. v2 drops pe_align_size() and rounds up with ALIGN() on a u64,
which can't wrap.

> Please, avoid this increment by starting at 4.

Will do.

> The PECOFF specification defines a default of 512 for the FileAlignment.
> That is the value you should use if FileAlignment == 0.

Will do, with a test.

> Here you are closing a memory leak. But this issue is not described in
> the commit message.

There was no leak before: the old code had no error exit while sorted
was allocated. The free is needed for the error exits this patch adds
to the section loop. I'll say so in the commit message.

Thanks,
Ana

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

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@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 <anabobillo3@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.
+		 */
+		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);
 	}
 
 	/* Read 32/64bit specific header bits */

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

[Ilias Apalodimas]

On Fri, 30 Oct 2026 at 08:13, Ana Bobillo <anabobillo3@gmail.com> wrote:
>
> From: Ana Bobillo <anabobillo3@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 <anabobillo3@gmail.com>
> ---

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

>  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.
> +                */
> +               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);
>         }
>
>         /* Read 32/64bit specific header bits */

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 <anabobillo3@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 <anabobillo3@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 */

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

[Ana Bobillo]

Hi Heinrich,

On 10/30/26 13:35, Heinrich Schuchardt wrote:
> 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.

It does wrap on 64bit too: VirtualAddress and section_size() are both
u32, so the sum is computed in 32 bits before max_t() converts it to
unsigned long. But the comment should say so. In v2 it reads:

	/*
	 * VirtualAddress and section_size() are both u32, so their sum
	 * wraps around at 4 GiB, on 64bit systems too. A wrapped end
	 * would make virt_size too small for the memset() and memcpy()
	 * below.
	 */

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

Agreed. I left the name alone here to keep this patch to the fix.

Thanks,
Ana

[PATCH 04/10] efi_loader: check PE section raw data against the image size

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_load_pe() copies each section from PointerToRawData in the image
without checking that the copied bytes lie within the image, so a
section pointing near or past the end of the image is read out of
bounds.

Reject such a section before the image is allocated, as barebox, whose
EFI loader is based on this file, does since 552a54e4d357 ("efi: loader:
validate section raw data bounds against image size").

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 <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index b15a662e5d6..7fd03ac6211 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -1075,6 +1075,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 			return EFI_LOAD_ERROR;
 		}
 
+		/* The data copied for the section must lie within the image */
+		if (!pe_range_ok(efi_size, sec->PointerToRawData,
+				 min(section_size(sec), sec->SizeOfRawData))) {
+			log_err("Section %d exceeds image size\n", i);
+			return EFI_LOAD_ERROR;
+		}
+
 		virt_size = max_t(unsigned long, virt_size, end);
 	}
 

Re: [PATCH 04/10] efi_loader: check PE section raw data against the image size

[Heinrich Schuchardt]

On 10/30/26 07:14, Ana Bobillo wrote:
> From: Ana Bobillo <anabobillo3@gmail.com>
> 
> efi_load_pe() copies each section from PointerToRawData in the image
> without checking that the copied bytes lie within the image, so a
> section pointing near or past the end of the image is read out of
> bounds.
> 
> Reject such a section before the image is allocated, as barebox, whose
> EFI loader is based on this file, does since 552a54e4d357 ("efi: loader:
> validate section raw data bounds against image size").
> 
> 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 <anabobillo3@gmail.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index b15a662e5d6..7fd03ac6211 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -1075,6 +1075,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>   			return EFI_LOAD_ERROR;
>   		}
>   
> +		/* The data copied for the section must lie within the image */
> +		if (!pe_range_ok(efi_size, sec->PointerToRawData,
> +				 min(section_size(sec), sec->SizeOfRawData))) {

All of raw data should be inside the image even if VirtualSize < 
SizeOfRawData.

Best regards

Heinrich

> +			log_err("Section %d exceeds image size\n", i);
> +			return EFI_LOAD_ERROR;
> +		}
> +
>   		virt_size = max_t(unsigned long, virt_size, end);
>   	}
>   

Re: [PATCH 04/10] efi_loader: check PE section raw data against the image size

[Ana Bobillo]

Hi Heinrich,

On 10/30/26 13:45, Heinrich Schuchardt wrote:
>> +		/* The data copied for the section must lie within the image */
>> +		if (!pe_range_ok(efi_size, sec->PointerToRawData,
>> +				 min(section_size(sec), sec->SizeOfRawData))) {
>
> All of raw data should be inside the image even if VirtualSize <
> SizeOfRawData.

Agreed. efi_image_parse() already requires that after 02, so in v2 this
checks all of SizeOfRawData, with a test for a section whose VirtualSize
is smaller.

Thanks,
Ana

[PATCH 05/10] efi_loader: check that the PE headers fit into the loaded image

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_load_pe() copies the PE headers to the start of the allocated image.
The size it copies is not the size of the headers: it adds the size of a
whole 32bit NT header, which already holds an optional header, to
SizeOfOptionalHeader, and assumes that the NT header follows the DOS
header directly. Nor is it checked against the size of the allocation,
which only covers the sections. An image with many empty section
headers but little section data makes the copy write past the
allocation, and the copy reads past the end of a small image.

Copy the headers up to the end of the section table, which is already
checked to lie within the image, and reject an image whose headers do
not fit into its virtual size before allocating it.

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

Fixes: 8458bf648fd7 ("efi_loader: set image_base and image_size to correct values")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 7fd03ac6211..f9cd2cb274c 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -1010,7 +1010,6 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 			 struct efi_loaded_image *loaded_image_info)
 {
 	IMAGE_NT_HEADERS32 *nt;
-	IMAGE_DOS_HEADER *dos;
 	IMAGE_SECTION_HEADER *sections;
 	int num_sections;
 	void *efi_reloc;
@@ -1020,6 +1019,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
 	uint64_t image_base;
 	unsigned long virt_size = 0;
+	size_t hdr_size;
 	int supported = 0;
 	efi_status_t ret;
 
@@ -1085,6 +1085,16 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 		virt_size = max_t(unsigned long, virt_size, end);
 	}
 
+	/*
+	 * The PE headers, up to the end of the section table, are copied to
+	 * the start of the loaded image and must fit into it.
+	 */
+	hdr_size = (void *)(sections + num_sections) - efi;
+	if (hdr_size > virt_size) {
+		log_err("PE headers do not fit into the image\n");
+		return EFI_LOAD_ERROR;
+	}
+
 	/* Read 32/64bit specific header bits */
 	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
 		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
@@ -1141,11 +1151,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 #endif
 
 	/* Copy PE headers */
-	memcpy(efi_reloc, efi,
-	       sizeof(*dos)
-		 + sizeof(*nt)
-		 + nt->FileHeader.SizeOfOptionalHeader
-		 + num_sections * sizeof(IMAGE_SECTION_HEADER));
+	memcpy(efi_reloc, efi, hdr_size);
 
 	/* Load sections into RAM */
 	for (i = num_sections - 1; i >= 0; i--) {

Re: [PATCH 05/10] efi_loader: check that the PE headers fit into the loaded image

[Heinrich Schuchardt]

On 10/30/26 07:14, Ana Bobillo wrote:
> From: Ana Bobillo <anabobillo3@gmail.com>
> 
> efi_load_pe() copies the PE headers to the start of the allocated image.
> The size it copies is not the size of the headers: it adds the size of a
> whole 32bit NT header, which already holds an optional header, to
> SizeOfOptionalHeader, and assumes that the NT header follows the DOS
> header directly. Nor is it checked against the size of the allocation,
> which only covers the sections. An image with many empty section
> headers but little section data makes the copy write past the
> allocation, and the copy reads past the end of a small image.

For secure boot we are looking at all regions. Shouldn't we make

> 
> Copy the headers up to the end of the section table, which is already
> checked to lie within the image, and reject an image whose headers do
> not fit into its virtual size before allocating it.
> 
> This runs before the image is rejected when Secure Boot fails to verify
> it.
> 
> Fixes: 8458bf648fd7 ("efi_loader: set image_base and image_size to correct values")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 7fd03ac6211..f9cd2cb274c 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -1010,7 +1010,6 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>   			 struct efi_loaded_image *loaded_image_info)
>   {
>   	IMAGE_NT_HEADERS32 *nt;
> -	IMAGE_DOS_HEADER *dos;
>   	IMAGE_SECTION_HEADER *sections;
>   	int num_sections;
>   	void *efi_reloc;
> @@ -1020,6 +1019,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>   	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
>   	uint64_t image_base;
>   	unsigned long virt_size = 0;
> +	size_t hdr_size;
>   	int supported = 0;
>   	efi_status_t ret;
>   
> @@ -1085,6 +1085,16 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>   		virt_size = max_t(unsigned long, virt_size, end);
>   	}
>   
> +	/*
> +	 * The PE headers, up to the end of the section table, are copied to
> +	 * the start of the loaded image and must fit into it.
> +	 */
> +	hdr_size = (void *)(sections + num_sections) - efi;
> +	if (hdr_size > virt_size) {
> +		log_err("PE headers do not fit into the image\n");
> +		return EFI_LOAD_ERROR;
> +	}
> +

Shouldn't we ensure that we always allocate this amount of memory 
instead of failing here?

Best regards

Heinrich

>   	/* Read 32/64bit specific header bits */
>   	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>   		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
> @@ -1141,11 +1151,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>   #endif
>   
>   	/* Copy PE headers */
> -	memcpy(efi_reloc, efi,
> -	       sizeof(*dos)
> -		 + sizeof(*nt)
> -		 + nt->FileHeader.SizeOfOptionalHeader
> -		 + num_sections * sizeof(IMAGE_SECTION_HEADER));
> +	memcpy(efi_reloc, efi, hdr_size);
>   
>   	/* Load sections into RAM */
>   	for (i = num_sections - 1; i >= 0; i--) {

Re: [PATCH 05/10] efi_loader: check that the PE headers fit into the loaded image

[Ana Bobillo]

Hi Heinrich,

On 10/30/26 14:09, Heinrich Schuchardt wrote:
> For secure boot we are looking at all regions. Shouldn't we make

This sentence got cut off. Could you finish it? If you meant that the
header copy should cover SizeOfHeaders, as the hashed regions do, I can
change that in v2.

>> +	hdr_size = (void *)(sections + num_sections) - efi;
>> +	if (hdr_size > virt_size) {
>> +		log_err("PE headers do not fit into the image\n");
>> +		return EFI_LOAD_ERROR;
>> +	}
>
> Shouldn't we ensure that we always allocate this amount of memory
> instead of failing here?

Yes, that is better. In v2 virt_size is raised to at least hdr_size, so
the headers always fit and such an image loads instead of being
refused. The test now checks that the loaded image holds the whole
section table.

Thanks,
Ana

[PATCH 06/10] efi_loader: check the relocation table against the image size

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_loader_relocate() checks each relocation block and entry against the
end of the relocation table and each target against the image size, but
the table itself is taken from the base relocation data directory as it
is: its address and size are not checked against the image, so the
blocks are read from wherever they point, up to 4 GiB past the image.
The directory is also read when NumberOfRvaAndSizes says the image has
none, in which case its bytes belong to the section table.

Use the base relocation directory only if the image has one, and reject
a relocation table that does not lie within the image, as EDK2's
PeCoffLoaderRelocateImage() does.

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 <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index f9cd2cb274c..c0677b440e8 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -117,9 +117,17 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
 					unsigned long virt_size)
 {
 	unsigned long delta = (unsigned long)efi_reloc - pref_address;
+	unsigned long rel_off = (const char *)rel - (const char *)efi_reloc;
 	const IMAGE_BASE_RELOCATION *end;
 	int i;
 
+	/* The relocation table must lie within the image */
+	if (rel_size &&
+	    (rel_off > virt_size || rel_size > virt_size - rel_off)) {
+		log_debug("Relocation table exceeds image size\n");
+		return EFI_LOAD_ERROR;
+	}
+
 	if (delta == 0)
 		return EFI_SUCCESS;
 
@@ -1111,7 +1119,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 			goto err;
 		}
 		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
-		rel_size = opt->DataDirectory[rel_idx].Size;
+		rel_size = opt->NumberOfRvaAndSizes > rel_idx ?
+			   opt->DataDirectory[rel_idx].Size : 0;
 		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
@@ -1127,7 +1136,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 			goto err;
 		}
 		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
-		rel_size = opt->DataDirectory[rel_idx].Size;
+		rel_size = opt->NumberOfRvaAndSizes > rel_idx ?
+			   opt->DataDirectory[rel_idx].Size : 0;
 		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else {
 		log_err("Invalid optional header magic %x\n",

[PATCH 07/10] efi_loader: check certificate lengths in efi_image_authenticate()

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_image_authenticate() walks the certificate table of an image by
dwLength, rounded up to a multiple of 8, without checking it:

- a dwLength of 0, or one that wraps around to 0 when rounded up,
  does not advance the walk, so U-Boot hangs on such an image;
- a dwLength larger than the rest of the table makes
  pkcs7_parse_message() read past the end of the table and of the
  image.

As EDK2's DxeImageVerificationLib does, reject an image whose
certificate table holds a certificate shorter than its header or
longer than the rest of the table.

Fixes: 4540dabdcaca ("efi_loader: image_loader: support image authentication")
Fixes: 1a44b7059c18 ("efi_loader: image_loader: add a check against certificate type of authenticode")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index c0677b440e8..c4f64f102e8 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -822,10 +822,27 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	for (wincert = wincerts, wincerts_end = (u8 *)wincerts + wincerts_len;
 	     (u8 *)wincert < wincerts_end;
 	     wincert = (WIN_CERTIFICATE *)
-			((u8 *)wincert + ALIGN(wincert->dwLength, 8))) {
-		if ((u8 *)wincert + sizeof(*wincert) >= wincerts_end)
+			((u8 *)wincert + ALIGN((size_t)wincert->dwLength, 8))) {
+		size_t remaining = wincerts_end - (u8 *)wincert;
+
+		if (sizeof(*wincert) >= remaining)
 			break;
 
+		/*
+		 * dwLength includes the WIN_CERTIFICATE header. A shorter one
+		 * would not advance the walk, and one longer than the rest of
+		 * the certificate table would make the signature run past its
+		 * end. Like EDK2, reject an image with such a corrupted
+		 * certificate table.
+		 */
+		if (wincert->dwLength < sizeof(*wincert) ||
+		    wincert->dwLength > remaining) {
+			log_err("Invalid certificate length: %u\n",
+				wincert->dwLength);
+			ret = false;
+			goto out;
+		}
+
 		if (wincert->dwLength <= sizeof(*wincert)) {
 			log_debug("dwLength too small: %u < %zu\n",
 				  wincert->dwLength, sizeof(*wincert));

[PATCH 08/10] efi_loader: do not load an image that cannot be measured

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

With TCG2 measured boot, efi_load_pe() measures each image into PCR 4
or 2, but it gives up only when the measurement reports that no TPM was
found. Any other error is ignored, so an image whose hash cannot be
computed, because efi_image_parse() rejects it, or whose PCR could not
be extended is loaded and can be started without being measured. With
the range checks added to efi_image_parse() earlier in this series,
more malformed images take this path.

As EDK2's DxeTpm2MeasureBootLib does, refuse to load an image whose
measurement failed: free the memory allocated for it, which the error
path leaked so far, and return EFI_ACCESS_DENIED, so that LoadImage()
returns no image handle. A full event log is no error, as there: the
PCR is extended all the same.

Fixes: 163a0d7e2cbd ("efi_loader: add PE/COFF image measurement")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 lib/efi_loader/efi_image_loader.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index c4f64f102e8..c33b693c4ff 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -1166,12 +1166,22 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 #if IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)
 	/* Measure an PE/COFF image */
 	ret = tcg2_measure_pe_image(efi, efi_size, handle, loaded_image_info);
-	if (ret == EFI_SECURITY_VIOLATION) {
-		/*
-		 * TCG2 Protocol is installed but no TPM device found,
-		 * this is not expected.
-		 */
-		log_err("PE image measurement failed, no tpm device found\n");
+	/*
+	 * Do not load an image that could not be measured. A full event log
+	 * is no error: the PCR has been extended all the same.
+	 */
+	if (ret != EFI_SUCCESS && ret != EFI_VOLUME_FULL) {
+		if (ret == EFI_SECURITY_VIOLATION)
+			/*
+			 * TCG2 Protocol is installed but no TPM device found,
+			 * this is not expected.
+			 */
+			log_err("PE image measurement failed, no tpm device found\n");
+		else
+			log_err("PE image measurement failed\n");
+		efi_free_pages((uintptr_t)efi_reloc,
+			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
+		ret = EFI_ACCESS_DENIED;
 		goto err;
 	}
 

Re: [PATCH 08/10] efi_loader: do not load an image that cannot be measured

[Ilias Apalodimas]

Hi Ana

On Fri, 30 Oct 2026 at 08:14, Ana Bobillo <anabobillo3@gmail.com> wrote:
>
> From: Ana Bobillo <anabobillo3@gmail.com>
>
> With TCG2 measured boot, efi_load_pe() measures each image into PCR 4
> or 2, but it gives up only when the measurement reports that no TPM was
> found. Any other error is ignored, so an image whose hash cannot be
> computed, because efi_image_parse() rejects it, or whose PCR could not
> be extended is loaded and can be started without being measured. With
> the range checks added to efi_image_parse() earlier in this series,
> more malformed images take this path.
>

This is a policy decision. The logic behind this is that the OS is
responsible for catching wrong measurements, not the firmware.
E.g sealed keys etc would fail. If someone cares so much about
security, wouldn't EFI secure boot be enough to verify the image? IOW
do have use cases where people rely *only* on a TPM for security?

> As EDK2's DxeTpm2MeasureBootLib does, refuse to load an image whose
> measurement failed: free the memory allocated for it, which the error
> path leaked so far, and return EFI_ACCESS_DENIED, so that LoadImage()
> returns no image handle. A full event log is no error, as there: the
> PCR is extended all the same.

Doesn't EDKII also disable the TPM if Tcg2HashLogExtendEvent fails? So
does it allow to load further images without measuring if the
measurements fail?

Thanks
/Ilias

>
> Fixes: 163a0d7e2cbd ("efi_loader: add PE/COFF image measurement")
> Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
> ---
>  lib/efi_loader/efi_image_loader.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index c4f64f102e8..c33b693c4ff 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -1166,12 +1166,22 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>  #if IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)
>         /* Measure an PE/COFF image */
>         ret = tcg2_measure_pe_image(efi, efi_size, handle, loaded_image_info);
> -       if (ret == EFI_SECURITY_VIOLATION) {
> -               /*
> -                * TCG2 Protocol is installed but no TPM device found,
> -                * this is not expected.
> -                */
> -               log_err("PE image measurement failed, no tpm device found\n");
> +       /*
> +        * Do not load an image that could not be measured. A full event log
> +        * is no error: the PCR has been extended all the same.
> +        */
> +       if (ret != EFI_SUCCESS && ret != EFI_VOLUME_FULL) {
> +               if (ret == EFI_SECURITY_VIOLATION)
> +                       /*
> +                        * TCG2 Protocol is installed but no TPM device found,
> +                        * this is not expected.
> +                        */
> +                       log_err("PE image measurement failed, no tpm device found\n");
> +               else
> +                       log_err("PE image measurement failed\n");
> +               efi_free_pages((uintptr_t)efi_reloc,
> +                              (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
> +               ret = EFI_ACCESS_DENIED;
>                 goto err;
>         }
>

Re: [PATCH 08/10] efi_loader: do not load an image that cannot be measured

[Ana Bobillo]

Hi Ilias,

On Fri, 30 Oct 2026 at 09:22, Ilias Apalodimas wrote:
> This is a policy decision. The logic behind this is that the OS is
> responsible for catching wrong measurements, not the firmware.
> E.g sealed keys etc would fail. If someone cares so much about
> security, wouldn't EFI secure boot be enough to verify the image? IOW
> do have use cases where people rely *only* on a TPM for security?

The OS can catch a wrong measurement, but not a missing one. An image
that U-Boot starts without measuring it runs with boot services, so it
can load and start the expected boot loader itself. That loader is
measured as usual, and PCR 4 and the event log come out as for a good
boot: keys sealed to them unseal, and attestation passes.

No broken image is needed for that. An application whose Subsystem
field holds a value U-Boot does not know, say 3, is loaded all the
same (efi_set_code_and_data_type() logs "invalid image type" and gives
it an application's memory types), tcg2_measure_pe_image() returns
EFI_UNSUPPORTED before hashing it, and efi_load_pe() goes on. On
sandbox, LoadImage() of such an image returns EFI_SUCCESS and leaves
PCR 4 as it was, while the same image with subsystem 10 extends it.
efi_start_image() then skips efi_tcg2_measure_efi_app_invocation() as
well, since its image_type is not IMAGE_SUBSYSTEM_EFI_APPLICATION, so
the "Calling EFI Application from Boot Option" event and the separators
are measured only once it starts the real loader, as when U-Boot starts
that loader directly.

With Secure Boot on, the patch changes nothing in practice: such an
image fails efi_image_authenticate(), since the Subsystem field is
covered by the signature and an image efi_image_parse() rejects fails
it too, so it is never started. It matters with measured boot and no
Secure Boot, which is common on boards where nobody provisions Secure
Boot keys: disk keys sealed to PCRs (systemd-cryptenroll --tpm2-pcrs,
Clevis) and remote attestation (Keylime) rely on the measurements, not
on Secure Boot. There an attacker who can write to the ESP can run what
they like, but it is measured and the seal breaks; an image that
escapes the measurement leaves the seal intact.

> Doesn't EDKII also disable the TPM if Tcg2HashLogExtendEvent fails? So
> does it allow to load further images without measuring if the
> measurements fail?

Only when the TPM itself fails. Tcg2HashLogExtendEvent() in Tcg2Dxe
clears TPMPresentFlag when MeasurePeImageAndExtend() returns
EFI_DEVICE_ERROR, and reports a TPM interface error. Even then the
image being measured is not loaded: DxeTpm2MeasureBootHandler() returns
the error, and CoreLoadImage() fails on any error of a security handler
but EFI_SECURITY_VIOLATION. Later images do load unmeasured, as on a
platform without a TPM, but nothing is extended any more, so the PCRs
stop short of a good boot's values and sealed keys fail. That is the
case the OS catches.

An image EDK2 cannot measure because of what it is, one that
PeCoffLoaderGetImageInfo() rejects or whose subsystem
Tcg2MeasurePeImage() does not know, gets an error that leaves the TPM
on, and is refused the same way. So EDK2 does not start an image it
failed to measure while later measurements still work, and that is
what this patch stops U-Boot from doing. On a TPM error, the patch
refuses the image as EDK2 does.

In v2 I will name the subsystem case in the commit message and add it
to the unit test, and carry your Reviewed-by on 01 and 03.

Thanks,
Ana

Re: [PATCH 08/10] efi_loader: do not load an image that cannot be measured

[Ilias Apalodimas]

Hi Ana

On Fri, 30 Oct 2026 at 09:54, Ana Bobillo <anabobillo3@gmail.com> wrote:
>
> Hi Ilias,
>
> On Fri, 30 Oct 2026 at 09:22, Ilias Apalodimas wrote:
> > This is a policy decision. The logic behind this is that the OS is
> > responsible for catching wrong measurements, not the firmware.
> > E.g sealed keys etc would fail. If someone cares so much about
> > security, wouldn't EFI secure boot be enough to verify the image? IOW
> > do have use cases where people rely *only* on a TPM for security?
>
> The OS can catch a wrong measurement, but not a missing one. An image
> that U-Boot starts without measuring it runs with boot services, so it
> can load and start the expected boot loader itself. That loader is
> measured as usual, and PCR 4 and the event log come out as for a good
> boot: keys sealed to them unseal, and attestation passes.
>
> No broken image is needed for that. An application whose Subsystem
> field holds a value U-Boot does not know, say 3, is loaded all the
> same (efi_set_code_and_data_type() logs "invalid image type" and gives
> it an application's memory types), tcg2_measure_pe_image() returns
> EFI_UNSUPPORTED before hashing it, and efi_load_pe() goes on.

This looks completely AI generated and unreviewed. What kind of Image
are you expecting to boot and measure via EFI if it's not one of the
defined types (10,11,12)?
If you are worried about such an image shoudn't we check that in
loadimage or startimage? The UEFI spec itself only lists these [0]

> On
> sandbox, LoadImage() of such an image returns EFI_SUCCESS and leaves
> PCR 4 as it was, while the same image with subsystem 10 extends it.
> efi_start_image() then skips efi_tcg2_measure_efi_app_invocation() as
> well, since its image_type is not IMAGE_SUBSYSTEM_EFI_APPLICATION, so
> the "Calling EFI Application from Boot Option" event and the separators
> are measured only once it starts the real loader, as when U-Boot starts
> that loader directly.
>
> With Secure Boot on, the patch changes nothing in practice: such an
> image fails efi_image_authenticate(), since the Subsystem field is
> covered by the signature and an image efi_image_parse() rejects fails
> it too, so it is never started. It matters with measured boot and no
> Secure Boot, which is common on boards where nobody provisions Secure
> Boot keys: disk keys sealed to PCRs (systemd-cryptenroll --tpm2-pcrs,
> Clevis) and remote attestation (Keylime) rely on the measurements, not
> on Secure Boot. There an attacker who can write to the ESP can run what
> they like, but it is measured and the seal breaks; an image that
> escapes the measurement leaves the seal intact.
>
> > Doesn't EDKII also disable the TPM if Tcg2HashLogExtendEvent fails? So
> > does it allow to load further images without measuring if the
> > measurements fail?
>
> Only when the TPM itself fails. Tcg2HashLogExtendEvent() in Tcg2Dxe
> clears TPMPresentFlag when MeasurePeImageAndExtend() returns
> EFI_DEVICE_ERROR, and reports a TPM interface error. Even then the
> image being measured is not loaded: DxeTpm2MeasureBootHandler() returns
> the error, and CoreLoadImage() fails on any error of a security handler
> but EFI_SECURITY_VIOLATION. Later images do load unmeasured, as on a
> platform without a TPM, but nothing is extended any more, so the PCRs
> stop short of a good boot's values and sealed keys fail. That is the
> case the OS catches.
>
> An image EDK2 cannot measure because of what it is, one that
> PeCoffLoaderGetImageInfo() rejects or whose subsystem
> Tcg2MeasurePeImage() does not know, gets an error that leaves the TPM
> on, and is refused the same way. So EDK2 does not start an image it
> failed to measure while later measurements still work, and that is
> what this patch stops U-Boot from doing. On a TPM error, the patch
> refuses the image as EDK2 does.
>
> In v2 I will name the subsystem case in the commit message and add it
> to the unit test, and carry your Reviewed-by on 01 and 03.
>

[0] https://uefi.org/specs/UEFI/2.11/02_Overview.html#uefi-images

P.S: Again this is AI generated an I am personally ok to review them
if they fix problems. Please do not copy paste what your LLM thought
without reading it.

Thanks
/Ilias
> Thanks,
> Ana

Re: [PATCH 08/10] efi_loader: do not load an image that cannot be measured

[Ana Bobillo]

Hi Ilias,

On Fri, 30 Oct 2026 at 11:23, Ilias Apalodimas wrote:
> What kind of Image are you expecting to boot and measure via EFI if
> it's not one of the defined types (10,11,12)?
> If you are worried about such an image shoudn't we check that in
> loadimage or startimage? The UEFI spec itself only lists these [0]

Sorry, my last mail was much longer than it needed to be.

None that is legitimate. Such an image is only interesting to someone
who wants code to run unmeasured, so I agree it should be refused in
LoadImage(). That is what EDK2 does as well: CoreLoadPeImage() returns
EFI_UNSUPPORTED for any subsystem other than 10, 11 and 12.

In v2 I'll add a patch that makes efi_load_pe() return EFI_UNSUPPORTED
for any other subsystem, before the image is measured.

That leaves 08 with images LoadImage() takes but efi_image_parse()
can't hash, e.g. a FileAlignment that is not a power of two once 02 is
in (the case the test in 09 uses), and with a failed PCR extend. I'd
keep it for those.

Thanks,
Ana

[PATCH 09/10] test: lib: add tests for the PE image checks

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Add unit tests that pass malformed PE images to efi_check_pe(),
LoadImage() and efi_image_parse() and check that they are rejected:

- an image ending after a 32bit NT header;
- a section whose end wraps around 4 GiB;
- a section whose raw data runs past the end of the image;
- headers that do not fit into the loaded image;
- a relocation table past the end of the image;
- an image that cannot be measured, when TCG2 is installed;
- headers, an optional header, a certificate table entry, a section or
  a certificate table that do not fit, a file alignment that is not a
  power of two, and sections sharing their raw data.

Check that PE32 and PE32+ images with 16, 6 or 5 data directories, an
empty section, a last section that is not padded and trailing data of an
odd size still load and parse.

Each image is built at the start of a larger zeroed buffer, so that
without the checks most tests read the bytes after it and get a wrong
status instead of faulting. Without the checks the tests of the wrapped
section and of the relocation table far past the image crash, and the
test of the headers writes past the loaded image.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 test/lib/Makefile           |   2 +-
 test/lib/efi_image_loader.c | 442 ++++++++++++++++++++++++++++++++++++
 2 files changed, 443 insertions(+), 1 deletion(-)
 create mode 100644 test/lib/efi_image_loader.c

diff --git a/test/lib/Makefile b/test/lib/Makefile
index 9fc4a1a9b5d..a67e53ca9c5 100644
--- a/test/lib/Makefile
+++ b/test/lib/Makefile
@@ -9,7 +9,7 @@ ifeq ($(CONFIG_XPL_BUILD),)
 obj-y += abuf.o
 obj-y += alist.o
 obj-$(CONFIG_ECDSA_VERIFY_MBEDTLS) += ecdsa.o
-obj-$(CONFIG_EFI_LOADER) += efi_device_path.o efi_memory.o
+obj-$(CONFIG_EFI_LOADER) += efi_device_path.o efi_image_loader.o efi_memory.o
 obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
 ifdef CONFIG_RISCV
 obj-$(CONFIG_USE_PRIVATE_LIBGCC) += test_clz.o
diff --git a/test/lib/efi_image_loader.c b/test/lib/efi_image_loader.c
new file mode 100644
index 00000000000..2cd4520a4e3
--- /dev/null
+++ b/test/lib/efi_image_loader.c
@@ -0,0 +1,442 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test the checks of the PE image loader
+ *
+ * Copyright (c) 2026 Ana Bobillo <anabobillo3@gmail.com>
+ */
+
+#include <efi_loader.h>
+#include <pe.h>
+#include <test/lib.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+#if defined(__aarch64__)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_ARM64
+#elif defined(__arm__)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_THUMB
+#elif defined(__x86_64__)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_AMD64
+#elif defined(__i386__)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_I386
+#elif defined(__riscv) && (__riscv_xlen == 64)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_RISCV64
+#elif defined(__riscv) && (__riscv_xlen == 32)
+#define TEST_MACHINE	IMAGE_FILE_MACHINE_RISCV32
+#else
+/* The loader supports no other machine: skip the tests that load images */
+#define TEST_MACHINE	0
+#endif
+
+#define TEST_FILE_ALIGN		0x200
+#define TEST_SECTION_ALIGN	0x1000
+/* Size of a valid image: headers and one section, each TEST_FILE_ALIGN */
+#define TEST_IMAGE_SIZE		(2 * TEST_FILE_ALIGN)
+
+/**
+ * struct test_image - a PE image at the start of buf
+ *
+ * @dos:	DOS header
+ * @nt:		NT headers, of a PE32 or a PE32+ image
+ * @opt32:	optional header of a PE32 image, else NULL
+ * @opt64:	optional header of a PE32+ image, else NULL
+ * @dirs:	data directories
+ * @sections:	section table
+ */
+struct test_image {
+	IMAGE_DOS_HEADER *dos;
+	IMAGE_NT_HEADERS32 *nt;
+	IMAGE_OPTIONAL_HEADER32 *opt32;
+	IMAGE_OPTIONAL_HEADER64 *opt64;
+	IMAGE_DATA_DIRECTORY *dirs;
+	IMAGE_SECTION_HEADER *sections;
+};
+
+/* Set a field of the optional header of a PE32 or a PE32+ image */
+#define opt_set(img, field, val)			\
+	do {						\
+		if ((img)->opt64)			\
+			(img)->opt64->field = (val);	\
+		else					\
+			(img)->opt32->field = (val);	\
+	} while (0)
+
+/*
+ * Images are built at the start of this buffer, which is larger than any of
+ * them. The bytes after an image are zero, so that they can be read without
+ * a fault if a size check is missing.
+ */
+static u8 buf[0x4000] __aligned(8);
+
+/**
+ * test_image_init() - set up a valid image at the start of buf
+ *
+ * The image is TEST_IMAGE_SIZE bytes long: the headers, padded to the file
+ * alignment, and one section of TEST_FILE_ALIGN bytes. Further sections are
+ * empty.
+ *
+ * @img:		image to set up
+ * @pe32plus:		true for a PE32+ image, false for a PE32 image
+ * @num_dirs:		number of data directories
+ * @num_sections:	number of section headers
+ */
+static void test_image_init(struct test_image *img, bool pe32plus,
+			    u32 num_dirs, int num_sections)
+{
+	size_t opt_size;
+
+	memset(buf, 0, sizeof(buf));
+	memset(img, 0, sizeof(*img));
+
+	img->dos = (void *)buf;
+	img->dos->e_magic = IMAGE_DOS_SIGNATURE;
+	img->dos->e_lfanew = sizeof(*img->dos);
+
+	img->nt = (void *)buf + img->dos->e_lfanew;
+	img->nt->Signature = IMAGE_NT_SIGNATURE;
+	img->nt->FileHeader.Machine = TEST_MACHINE;
+	img->nt->FileHeader.NumberOfSections = num_sections;
+
+	if (pe32plus) {
+		img->opt64 = (void *)&img->nt->OptionalHeader;
+		img->opt64->Magic = IMAGE_NT_OPTIONAL_HDR64_MAGIC;
+		img->dirs = img->opt64->DataDirectory;
+		opt_size = offsetof(IMAGE_OPTIONAL_HEADER64, DataDirectory);
+	} else {
+		img->opt32 = &img->nt->OptionalHeader;
+		img->opt32->Magic = IMAGE_NT_OPTIONAL_HDR32_MAGIC;
+		img->dirs = img->opt32->DataDirectory;
+		opt_size = offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory);
+	}
+	opt_size += num_dirs * sizeof(IMAGE_DATA_DIRECTORY);
+	img->nt->FileHeader.SizeOfOptionalHeader = opt_size;
+	img->sections = (void *)&img->nt->OptionalHeader + opt_size;
+
+	opt_set(img, AddressOfEntryPoint, TEST_SECTION_ALIGN);
+	opt_set(img, SectionAlignment, TEST_SECTION_ALIGN);
+	opt_set(img, FileAlignment, TEST_FILE_ALIGN);
+	opt_set(img, SizeOfImage, 2 * TEST_SECTION_ALIGN);
+	opt_set(img, SizeOfHeaders, TEST_FILE_ALIGN);
+	opt_set(img, Subsystem, IMAGE_SUBSYSTEM_EFI_APPLICATION);
+	opt_set(img, NumberOfRvaAndSizes, num_dirs);
+
+	memcpy(img->sections[0].Name, ".text", 5);
+	img->sections[0].Misc.VirtualSize = TEST_FILE_ALIGN;
+	img->sections[0].VirtualAddress = TEST_SECTION_ALIGN;
+	img->sections[0].SizeOfRawData = TEST_FILE_ALIGN;
+	img->sections[0].PointerToRawData = TEST_FILE_ALIGN;
+}
+
+/**
+ * load_image() - pass an image at the start of buf to LoadImage()
+ *
+ * If the image is loaded, unload it again.
+ *
+ * @size:	size of the image
+ * Return:	status code of LoadImage()
+ */
+static efi_status_t load_image(size_t size)
+{
+	efi_handle_t handle = NULL;
+	efi_status_t ret;
+
+	ret = EFI_CALL(efi_load_image(false, efi_root, NULL, buf, size,
+				      &handle));
+	if (handle)
+		EFI_CALL(efi_unload_image(handle));
+
+	return ret;
+}
+
+/**
+ * parse_image() - pass an image at the start of buf to efi_image_parse()
+ *
+ * @size:	size of the image
+ * Return:	result of efi_image_parse()
+ */
+static bool parse_image(size_t size)
+{
+	struct efi_image_regions *regs = NULL;
+	WIN_CERTIFICATE *auth = NULL;
+	size_t auth_len = 0;
+	bool ret;
+
+	ret = efi_image_parse(buf, size, &regs, &auth, &auth_len);
+	free(regs);
+
+	return ret;
+}
+
+/* The checks must let valid images through */
+static int lib_test_efi_image_valid(struct unit_test_state *uts)
+{
+	static const u32 num_dirs[] = { IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 6 };
+	struct test_image img;
+	int i, j;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+
+	for (i = 0; i < 2; i++) {
+		for (j = 0; j < ARRAY_SIZE(num_dirs); j++) {
+			test_image_init(&img, i, num_dirs[j], 1);
+			ut_asserteq(true, parse_image(TEST_IMAGE_SIZE));
+			ut_asserteq_64(EFI_SUCCESS, load_image(TEST_IMAGE_SIZE));
+		}
+	}
+
+	/* An empty section, and trailing data of an odd size */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 2);
+	memcpy(img.sections[1].Name, ".bss", 4);
+	img.sections[1].Misc.VirtualSize = TEST_FILE_ALIGN;
+	img.sections[1].VirtualAddress = 2 * TEST_SECTION_ALIGN;
+	opt_set(&img, SizeOfImage, 3 * TEST_SECTION_ALIGN);
+	ut_asserteq(true, parse_image(TEST_IMAGE_SIZE + 3));
+	ut_asserteq_64(EFI_SUCCESS, load_image(TEST_IMAGE_SIZE + 3));
+
+	/* An image with five data directories has no relocation table */
+	test_image_init(&img, true, 5, 1);
+	ut_asserteq(true, parse_image(TEST_IMAGE_SIZE));
+	ut_asserteq_64(EFI_SUCCESS, load_image(TEST_IMAGE_SIZE));
+
+	/* The last section is not padded to the file alignment */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	img.sections[0].SizeOfRawData = TEST_FILE_ALIGN / 2;
+	ut_asserteq(true, parse_image(TEST_IMAGE_SIZE - TEST_FILE_ALIGN / 2));
+	ut_asserteq_64(EFI_SUCCESS,
+		       load_image(TEST_IMAGE_SIZE - TEST_FILE_ALIGN / 2));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_image_valid, 0);
+
+/* efi_check_pe() must check that the image holds a 64bit NT header */
+static int lib_test_efi_check_pe_nt_header(struct unit_test_state *uts)
+{
+	struct test_image img;
+	size_t size;
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	ut_asserteq_64(EFI_SUCCESS, efi_check_pe(buf, TEST_IMAGE_SIZE, NULL));
+
+	/* The image ends after a 32bit NT header */
+	size = img.dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32);
+	ut_asserteq_64(EFI_INVALID_PARAMETER, efi_check_pe(buf, size, NULL));
+
+	/* The NT header offset lies past the end (wraps on 32bit hosts) */
+	img.dos->e_lfanew = 0xfffffff0;
+	ut_asserteq_64(EFI_INVALID_PARAMETER,
+		       efi_check_pe(buf, TEST_IMAGE_SIZE, NULL));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_check_pe_nt_header, 0);
+
+/* efi_load_pe() must reject a section ending past 4 GiB */
+static int lib_test_efi_load_pe_section_wrap(struct unit_test_state *uts)
+{
+	struct test_image img;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	/* VirtualAddress + VirtualSize wraps around to TEST_SECTION_ALIGN */
+	img.sections[0].VirtualAddress = 0xfffff000;
+	img.sections[0].Misc.VirtualSize = 0x2000;
+	ut_asserteq_64(EFI_LOAD_ERROR, load_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_load_pe_section_wrap, 0);
+
+/* efi_load_pe() must check the raw data of a section against the image */
+static int lib_test_efi_load_pe_section_raw(struct unit_test_state *uts)
+{
+	struct test_image img;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	/* The raw data of the section runs past the end of the image */
+	img.sections[0].SizeOfRawData = 2 * TEST_FILE_ALIGN;
+	img.sections[0].Misc.VirtualSize = 2 * TEST_FILE_ALIGN;
+	ut_asserteq_64(EFI_LOAD_ERROR, load_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_load_pe_section_raw, 0);
+
+/* efi_load_pe() must check that the headers fit into the loaded image */
+static int lib_test_efi_load_pe_headers(struct unit_test_state *uts)
+{
+	struct test_image img;
+	int num_sections, i;
+	size_t hdr_size, size;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+
+	/*
+	 * Enough empty sections for the section table to end past the first
+	 * page, while the only section with data is TEST_FILE_ALIGN bytes
+	 * long at virtual address 0: the loaded image is a single page.
+	 */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	num_sections = (EFI_PAGE_SIZE - ((void *)img.sections - (void *)buf)) /
+		       sizeof(IMAGE_SECTION_HEADER) + 2;
+	hdr_size = ALIGN((void *)&img.sections[num_sections] - (void *)buf,
+			 TEST_FILE_ALIGN);
+	size = hdr_size + TEST_FILE_ALIGN;
+	ut_assert(size <= sizeof(buf));
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES,
+			num_sections);
+	opt_set(&img, SizeOfHeaders, hdr_size);
+	opt_set(&img, AddressOfEntryPoint, 0);
+	img.sections[0].VirtualAddress = 0;
+	img.sections[0].PointerToRawData = hdr_size;
+	for (i = 1; i < num_sections; i++)
+		memcpy(img.sections[i].Name, ".bss", 4);
+	ut_asserteq_64(EFI_LOAD_ERROR, load_image(size));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_load_pe_headers, 0);
+
+/* efi_loader_relocate() must check the relocation table against the image */
+static int lib_test_efi_load_pe_reloc_table(struct unit_test_state *uts)
+{
+	struct test_image img;
+	IMAGE_DATA_DIRECTORY *dir;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	dir = &img.dirs[IMAGE_DIRECTORY_ENTRY_BASERELOC];
+	/* The relocation table starts in the section and ends past it */
+	dir->VirtualAddress = TEST_SECTION_ALIGN;
+	dir->Size = TEST_SECTION_ALIGN;
+	ut_asserteq_64(EFI_LOAD_ERROR, load_image(TEST_IMAGE_SIZE));
+
+	/* The relocation table lies far past the end of the image */
+	dir->VirtualAddress = 0xfffff000;
+	dir->Size = sizeof(IMAGE_BASE_RELOCATION);
+	ut_asserteq_64(EFI_LOAD_ERROR, load_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_load_pe_reloc_table, 0);
+
+/* efi_load_pe() must not load an image that cannot be measured */
+static int lib_test_efi_load_pe_measure(struct unit_test_state *uts)
+{
+	const efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
+	struct efi_handler *handler;
+	struct test_image img;
+
+	if (!TEST_MACHINE)
+		return -EAGAIN;
+
+	ut_asserteq_64(EFI_SUCCESS, efi_init_obj_list());
+	if (efi_search_protocol(efi_root, &tcg2_guid, &handler) != EFI_SUCCESS)
+		return -EAGAIN;
+
+	/*
+	 * LoadImage() takes a file alignment that is not a power of two, but
+	 * the image cannot be hashed for its measurement.
+	 */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	opt_set(&img, FileAlignment, 3 * TEST_FILE_ALIGN / 2);
+	ut_asserteq_64(EFI_ACCESS_DENIED, load_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_load_pe_measure, 0);
+
+/* efi_image_parse() must check the PE headers against the image */
+static int lib_test_efi_image_parse_headers(struct unit_test_state *uts)
+{
+	struct test_image img;
+
+	/* The headers run past the end of the image */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	opt_set(&img, SizeOfHeaders, 2 * TEST_IMAGE_SIZE);
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	/* The optional header is shorter than its fixed fields */
+	test_image_init(&img, true, 0, 1);
+	img.nt->FileHeader.SizeOfOptionalHeader = 0x40;
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	/*
+	 * The certificate table entry lies past the optional header, in the
+	 * section table: clear the section name read as the entry.
+	 */
+	test_image_init(&img, false, IMAGE_DIRECTORY_ENTRY_SECURITY, 1);
+	opt_set(&img, NumberOfRvaAndSizes, IMAGE_NUMBEROF_DIRECTORY_ENTRIES);
+	memset(img.sections[0].Name, 0, sizeof(img.sections[0].Name));
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_image_parse_headers, 0);
+
+/* efi_image_parse() must check each section against the image */
+static int lib_test_efi_image_parse_section(struct unit_test_state *uts)
+{
+	struct test_image img;
+
+	/* The raw data of the section runs past the end of the image */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	img.sections[0].SizeOfRawData = 2 * TEST_FILE_ALIGN;
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	/* The raw data of the section starts past the end of the image */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	img.sections[0].PointerToRawData = 0xfffffe00;
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	/* The file alignment is not a power of two */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	opt_set(&img, FileAlignment, 3 * TEST_FILE_ALIGN / 2);
+	ut_asserteq(false, parse_image(2 * TEST_IMAGE_SIZE));
+
+	/* Two sections share their raw data */
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 2);
+	img.sections[1] = img.sections[0];
+	img.sections[1].VirtualAddress = 2 * TEST_SECTION_ALIGN;
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_image_parse_section, 0);
+
+/* efi_image_parse() must check the certificate table against the image */
+static int lib_test_efi_image_parse_cert_table(struct unit_test_state *uts)
+{
+	struct test_image img;
+	IMAGE_DATA_DIRECTORY *dir;
+
+	test_image_init(&img, true, IMAGE_NUMBEROF_DIRECTORY_ENTRIES, 1);
+	dir = &img.dirs[IMAGE_DIRECTORY_ENTRY_SECURITY];
+	/* The end of the certificate table wraps around to 0x10 */
+	dir->VirtualAddress = 0xfffffff0;
+	dir->Size = 0x20;
+	ut_asserteq(false, parse_image(TEST_IMAGE_SIZE));
+
+	return 0;
+}
+LIB_TEST(lib_test_efi_image_parse_cert_table, 0);

[PATCH 10/10] test: py: test invalid certificate lengths in signed images

[Ana Bobillo]

From: Ana Bobillo <anabobillo3@gmail.com>

Add a Secure Boot test with two signed images whose first certificate has
an invalid length: zero, which made efi_image_authenticate() loop
forever, and one larger than the certificate table. Both images must be
rejected.

Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
 test/py/tests/test_efi_secboot/conftest.py    | 31 ++++++++++++++++
 test/py/tests/test_efi_secboot/test_signed.py | 36 +++++++++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 0755497d46d..9858dd36f17 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -4,11 +4,36 @@
 
 """Fixture for UEFI secure boot test."""
 
+import struct
 from subprocess import call, check_call, CalledProcessError
 from tests import fs_helper
 import pytest
 from defs import *
 
+
+def set_cert_length(src, dst, length):
+    """Copy a signed image, setting the length of its first certificate.
+
+    Args:
+        src: Path of the signed image.
+        dst: Path of the image to write.
+        length: dwLength to set, or None for one past the certificate table.
+    """
+    with open(src, 'rb') as inf:
+        data = bytearray(inf.read())
+    pe_off = struct.unpack_from('<I', data, 0x3c)[0]
+    opt_off = pe_off + 24
+    magic = struct.unpack_from('<H', data, opt_off)[0]
+    # The data directories follow the PE32+ or the PE32 optional header
+    dirs_off = opt_off + (112 if magic == 0x20b else 96)
+    # The certificate table is data directory 4
+    cert_off, cert_size = struct.unpack_from('<II', data, dirs_off + 4 * 8)
+    if length is None:
+        length = cert_size + 8
+    struct.pack_into('<I', data, cert_off, length)
+    with open(dst, 'wb') as outf:
+        outf.write(data)
+
 @pytest.fixture(scope='function')
 def efi_boot_env(request, ubman):
     """Set up a file system to be used in UEFI secure boot test.
@@ -118,6 +143,12 @@ def efi_boot_env(request, ubman):
         # Create a corrupted signed image
         check_call('cd %s; sh %s/test/py/tests/test_efi_secboot/forge_image.sh helloworld.efi.signed helloworld_forged.efi.signed'
                    % (mnt_point, ubman.config.source_dir), shell=True)
+        # Create signed images with an invalid certificate length
+        set_cert_length('%s/helloworld.efi.signed' % mnt_point,
+                        '%s/helloworld_certlen0.efi.signed' % mnt_point, 0)
+        set_cert_length('%s/helloworld.efi.signed' % mnt_point,
+                        '%s/helloworld_certlen_long.efi.signed' % mnt_point,
+                        None)
         # Digest image
         check_call('cd %s; %shash-to-efi-sig-list helloworld.efi db_hello.hash; %ssign-efi-sig-list -t "2020-04-07" -c KEK.crt -k KEK.key db db_hello.hash db_hello.auth'
                    % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 9e6d3fdf8d8..d14f9b795d5 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -378,3 +378,39 @@ class TestEfiSignedImage(object):
                 'printenv -e -n PK'])
             assert 'Failed to set EFI variable' not in ''.join(output)
             assert '\"PK\" not defined' in ''.join(output)
+
+    def test_efi_signed_image_auth9(self, ubman, efi_boot_env):
+        """
+        Test Case 9 - Secure boot is in force,
+                      the length of the certificate in the signed image
+                      is invalid.
+                      Must be rejected.
+        """
+        ubman.restart_uboot()
+        disk_img = efi_boot_env
+        with ubman.log.section('Test Case 9a'):
+            # Test Case 9a, certificate length of zero
+            output = ubman.run_command_list([
+                'host bind 0 %s' % disk_img,
+                'fatload host 0:1 4000000 db.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
+                'fatload host 0:1 4000000 KEK.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK',
+                'fatload host 0:1 4000000 PK.auth',
+                'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK'])
+            assert 'Failed to set EFI variable' not in ''.join(output)
+            output = ubman.run_command_list([
+                'efidebug boot add -b 1 HELLO1 host 0:1 /helloworld_certlen0.efi.signed -s ""',
+                'efidebug boot order 1',
+                'efidebug test bootmgr'])
+            assert '\'HELLO1\' failed' in ''.join(output)
+            assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
+
+        with ubman.log.section('Test Case 9b'):
+            # Test Case 9b, certificate longer than the certificate table
+            output = ubman.run_command_list([
+                'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld_certlen_long.efi.signed -s ""',
+                'efidebug boot order 2',
+                'efidebug test bootmgr'])
+            assert '\'HELLO2\' failed' in ''.join(output)
+            assert 'efi_bootmgr_load() returned: 26' in ''.join(output)