[PATCH] common: pe: fix pe reloc pointer overrun

[PATCH] common: pe: fix pe reloc pointer overrun

[Ahmad Fatoum]

Import the fix from U-Boot commit 463e4e6476299b32452a8a:

| Author:     Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
| AuthorDate: Fri Nov 29 21:09:44 2024 +0000
|
| efi_loader: fix pe reloc pointer overrun
|
| 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.
|
| 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
|
| This section has two relocations at offset bce26008 and bce2600a,
| however the given size (rel_size) for this relocation is 16-bytes
| 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.
| Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>
| Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/pe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/pe.c b/common/pe.c
index 6164dd68b7ab..65d456a2c332 100644
--- a/common/pe.c
+++ b/common/pe.c
@@ -61,7 +61,7 @@ static int pe_loader_relocate(const IMAGE_BASE_RELOCATION *rel,
 		return 0;
 
 	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.39.5

Re: [PATCH] common: pe: fix pe reloc pointer overrun

[Sascha Hauer]

On Thu, 27 Mar 2025 08:50:36 +0100, Ahmad Fatoum wrote:
> Import the fix from U-Boot commit 463e4e6476299b32452a8a:
> 
> | Author:     Aleksandar Gerasimovski <Aleksandar.Gerasimovski@belden.com>
> | AuthorDate: Fri Nov 29 21:09:44 2024 +0000
> |
> | efi_loader: fix pe reloc pointer overrun
> |
> | 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.
> |
> | 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
> |
> | This section has two relocations at offset bce26008 and bce2600a,
> | however the given size (rel_size) for this relocation is 16-bytes
> | 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.
> | Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@belden.com>
> | Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> [...]

Applied, thanks!

[1/1] common: pe: fix pe reloc pointer overrun
      https://git.pengutronix.de/cgit/barebox/commit/?id=d4b5f3d3d191 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>