[PATCH] efi_loader: fix pe reloc pointer overrun

[Heinrich Schuchardt]

On 29.11.24 22:09, Aleksandar Gerasimovski wrote:
> The fix provided by 997fc12ec91 is actually introducing
> a buffer overrun, and the overrun is effective if the
> memory after the reloc section is not zeroed.
> Probably that's why this bug is not always noticeable.
>
> The problem is that 8-bytes 'rel' pointer can be 4-bytes aligned
> according to the PE Format, so the actual relocate function can
> take values after the reloc section.

Sections have to be at least 512 bytes aligned. The file alignment
cannot be smaller the 512 bytes either.

Hence for a valid EFI binary the value of "rel" when entering
efi_loader_relocate() will always be at least 512 aligned.

>
> One example is the following dump from the reloc section:
>    bce26000: 3000 0000 000c 0000 0000 0000 0000 0000
>    bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20

Please, provide an example image for download to give the full picture.

>
> This section has two relocations at offset bce26008 and bce2600a,
> however the given size (rel_size) for this relocation is 16-bytes

The value of "rel_size" used to calculate "end" is not the size of a
relocation but the size of the relocation table.

The PE-COFF specification describes:

"SizeOfRawData

The size of the section (for object files) or the size of the
initialized data on disk (for image files). For executable images, this
must be a multiple of FileAlignment from the optional header. If this is
less than VirtualSize, the remainder of the section is zero-filled."

The situation above cannot occur if you follow the specification and
zero fill the raw data up to SizeOfRawData which is a multiple of 512.

> and this is coming form the efi image Misc.VirtualSize, so in this
> case the 'reloc' pointer ends at affset bce2600c and is taken as
> valid and this is where the overflow is.
>
> In our system we see this problem when we are starting the
> Boot Guard efi image.
>
> This patch is fixing the overrun while preserving the fix done
> by 997fc12ec91.

Before 997fc12ec91 we had "rel < end - 1".

With "rel + 1 < end" you are not preserving the change but reverting it.

Best regards

Heinrich

>
> Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski at belden.com>
> ---
>   lib/efi_loader/efi_image_loader.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 0ddf69a091..04075d83cb 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -121,7 +121,7 @@ static efi_status_t efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
>                  return EFI_SUCCESS;
>
>          end = (const IMAGE_BASE_RELOCATION *)((const char *)rel + rel_size);
> -       while (rel < end && rel->SizeOfBlock) {
> +       while (rel + 1 < end && rel->SizeOfBlock) {
>                  const uint16_t *relocs = (const uint16_t *)(rel + 1);
>                  i = (rel->SizeOfBlock - sizeof(*rel)) / sizeof(uint16_t);
>                  while (i--) {
> --
> 2.34.1
>
> **********************************************************************
> DISCLAIMER:
> Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.