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

[Heinrich Schuchardt]

On 5/25/26 17:13, Simon Glass wrote:
> 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.

Hello Simon,

Thanks for reviewing.

ok

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

This neither makes the code easier to read nor the code faster nor the 
resulting binary image smaller.

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

We originally introduced log_err() here when we were not sure if U-Boot 
covered all relocation types.

efi_loader_relocate() is invoked by EFI applications like GRUB. It 
probably would be better to convert the log_err() to log_debug() as we 
should not output anything inside an EFI application.

But this is beyond the scope of this patch.

Best regards

Heinrich