[PATCH 1/1] efi_loader: validate PE-COFF relocation data

[Simon Glass]

Hi Heinrich,

On 2026-05-18T06:30:20, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> efi_loader: validate PE-COFF relocation data
>
> When applying base relocations from a PE-COFF binary all data must
> be treated as untrusted. Add the following checks to
> efi_loader_relocate():
>
> * Reject relocation blocks that don't start on a 32-bit aligned
>   address.
> * Reject relocation blocks whose SizeOfBlock is smaller than the
>   block header, which would cause an unsigned underflow when computing
>   the entry count.
> * A block with SizeOfBlock == 0 is invalid and does not mark the end of
>   the relocation table.
> * Reject relocation blocks that extend beyond the end of the
>   relocation section.
> * Reject individual relocation entries whose target offset, together
>   with the access width, exceeds the mapped image size, preventing
>   out-of-bounds writes.
>
> Pass virt_size to efi_loader_relocate() from efi_load_pe() to enable
> [...]
>
> lib/efi_loader/efi_image_loader.c | 76 +++++++++++++++++++++++++++++++++------
>  1 file changed, 65 insertions(+), 11 deletions(-)

> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> @@ -122,34 +123,86 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
> +                     /*
> +                      * This first check covers VirtualAddress + Offset
> +                      * resulting in an overflow.
> +                      */
> +                     if (rel->VirtualAddress > virt_size ||
> +                         entry_offset > virt_size - rel->VirtualAddress) {
> +                             log_debug("relocation address out of bounds\n");
> +                             return EFI_LOAD_ERROR;
> +                     }

The rel->VirtualAddress > virt_size half is invariant for the whole
block - so it should be out of the while (i--) loop so it runs once
per block, not per entry. The per-entry overflow check on entry_offset
stays inside.

> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> @@ -122,34 +123,86 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
>                       case IMAGE_REL_BASED_HIGH:
> +                             if (sizeof(uint16_t) > virt_size - offset) {
> +                                     log_debug("relocation address out of bounds\n");
> +                                     return EFI_LOAD_ERROR;
> +                             }
>                               *x16 += ((uint32_t)delta) >> 16;
>                               break;
>                       case IMAGE_REL_BASED_LOW:
> +                             if (sizeof(uint16_t) > virt_size - offset) {
> +                                     log_debug("relocation address out of bounds\n");
> +                                     return EFI_LOAD_ERROR;
> +                             }
>                               *x16 += (uint16_t)delta;
>                               break;
>                       case IMAGE_REL_BASED_HIGHLOW:
> +                             if (sizeof(uint32_t) > virt_size - offset) {
> +                                     log_debug("relocation address out of bounds\n");
> +                                     return EFI_LOAD_ERROR;
> +                             }
>                               *x32 += (uint32_t)delta;
>                               break;
>                       case IMAGE_REL_BASED_DIR64:
> +                             if (sizeof(uint64_t) > virt_size - offset) {
> +                                     log_debug("relocation address out of bounds\n");
> +                                     return EFI_LOAD_ERROR;
> +                             }
>                               *x64 += (uint64_t)delta;
>                               break;
>  #ifdef __riscv
>                       case IMAGE_REL_BASED_RISCV_HI20:
> +                             if (sizeof(uint32_t) > virt_size - offset) {
> +                                     log_debug("relocation address out of bounds\n");
> +                                     return EFI_LOAD_ERROR;
> +                             }

Five copies of the same check make the switch hard to read. Please set
a width per case and do the bounds check once, e.g.

   size_t width;

   switch (type) {
   case IMAGE_REL_BASED_ABSOLUTE:
       continue;
   case IMAGE_REL_BASED_HIGH:
   case IMAGE_REL_BASED_LOW:
       width = sizeof(uint16_t);
       break;
   case IMAGE_REL_BASED_HIGHLOW:
   case IMAGE_REL_BASED_RISCV_HI20:
       width = sizeof(uint32_t);
       break;
   case IMAGE_REL_BASED_DIR64:
       width = sizeof(uint64_t);
       break;
   ...
   }
   if (width > virt_size - offset) {
       log_debug("relocation address out of bounds\n");
       return EFI_LOAD_ERROR;
   }

Then a second switch (or if-ladder) does the write. That keeps the
duplication out of the hot path and stops anyone adding a new case
without the bounds check.

> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> @@ -122,34 +123,86 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
> -     while (rel + 1 < end && rel->SizeOfBlock) {
> +     while (rel + 1 < end) {

Since this plugs a security hole driven by malformed input, please add
a py (or an lib/efi_selftest case) that feeds at least one rejected
shape - short SizeOfBlock, unaligned block, out-of-bounds
VirtualAddress - through efi_load_pe() and confirms EFI_LOAD_ERROR.
Without coverage there's nothing to stop a regression.

> @@ -163,7 +216,7 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
>  #endif
>                       default:
> -                             log_err("Unknown Relocation off %x type %x\n",
> +                             log_err("Unknown Relocation off %lx type %x\n",
>                                       offset, type);

Just to check - every other new diagnostic here uses log_debug(), but
the default arm keeps log_err(). I'm fine with that split, but the
other new "out of bounds" messages probably want log_err() too: a
refused image is something the user will want to see without enabling
debug logs, otherwise they just see the load fail with no clue why.

Regards,
Simon