[PATCH] 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 --- 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.
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@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.
Thanks for the replay,
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.
That's not what I see in the case that I try to explain, because the sec->Misc.VirtualSize virtual size is 16 bytes for this section .
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.
Correct, I see in the efi_load_pe code that section will be zeroed If sec->Misc.VirtualSize is bigger then sec->SizeOfRawData, but this is not a case for our image it's oposite. I can also confirm that in the real image the section is zeroed up to sec->SizeOfRawData, but the problem is that only the sec->Misc.VirtualSize (that is 16 bytes for my image) is copied to the "efi_reloc" location.
I'm far from really understanding the PE format, actually this is first time to deal with this and efi boot on Linux, however our EFI image is generated with a "standard" tools like bg_gen_unified_kernel by our yocto based distro generator, so in any case I see a nasty bug. Even if the image is wrong it should be rejected earlier and not overflowing even overwriting the u-boot code. It's a vulnerability.
Please, provide an example image for download to give the full picture.
I must think how to do that, where I could upload?
With "rel + 1 < end" you are not preserving the change but reverting it.
Well, this is the tricky part, in my understanding not because rel is moving by 2 bytes down in the loop.
I'm trying to describe the problem again, let's say in my image the problematic section is generated with sec->Misc.VirtualSize = 16 and sec->SizeOfRawData = 512, what will be provided to the efi_loader_relocate in this case is rel_size of 16 and only 16 bytes will be copied to efi_reloc location (see the " Load sections into RAM" logic in efi_load_pe), so the content of the RAM after efi_reloc + 16 stays unknown: bce26000: 3000 0000 000c 0000 0000 0000 0000 0000 -> reloc section bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20 -> random content
So efi_loader_relocate gets rel_size of 16 bytes for reloc section of 12 bytes, and this will overflow.
When I'm writing this, I'm thinking isn't then rel_size problem as well, that is 16 bytes in this case?
From: Heinrich Schuchardt xypron.glpk@gmx.de Sent: Mittwoch, 4. Dezember 2024 11:11 To: Aleksandar Gerasimovski Aleksandar.Gerasimovski@belden.com Cc: u-boot@lists.denx.de; Ilias Apalodimas ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
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 mailto:aleksandar.gerasimovski@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.
********************************************************************** 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.
On 05.12.24 09:13, Aleksandar Gerasimovski wrote:
Thanks for the replay,
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.
That's not what I see in the case that I try to explain, because the sec->Misc.VirtualSize virtual size is 16 bytes for this section .
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.
Correct, I see in the efi_load_pe code that section will be zeroed If sec->Misc.VirtualSize is bigger then sec->SizeOfRawData, but this is not a case for our image it's oposite. I can also confirm that in the real image the section is zeroed up to sec->SizeOfRawData, but the problem is that only the sec->Misc.VirtualSize (that is 16 bytes for my image) is copied to the "efi_reloc" location.
I'm far from really understanding the PE format, actually this is first time to deal with this and efi boot on Linux, however our EFI image is generated with a "standard" tools like bg_gen_unified_kernel by our yocto based distro generator, so in any case I see a nasty bug. Even if the image is wrong it should be rejected earlier and not overflowing even overwriting the u-boot code. It's a vulnerability.
Please, provide an example image for download to give the full picture.
I must think how to do that, where I could upload?
With "rel + 1 < end" you are not preserving the change but reverting it.
Well, this is the tricky part, in my understanding not because rel is moving by 2 bytes down in the loop.
I'm trying to describe the problem again, let's say in my image the problematic section is generated with sec->Misc.VirtualSize = 16 and sec->SizeOfRawData = 512, what will be provided to the efi_loader_relocate in this case is rel_size of 16 and only 16 bytes will be copied to efi_reloc location (see the " Load sections into RAM" logic in efi_load_pe), so the content of the RAM after efi_reloc + 16 stays unknown: bce26000: 3000 0000 000c 0000 0000 0000 0000 0000 -> reloc section bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20 -> random content
So efi_loader_relocate gets rel_size of 16 bytes for reloc section of 12 bytes, and this will overflow.
If VirtualSize = 16, then the reloc section has 16 bytes and not 12.
A section size of less than 512 is invalid anyway.
Something is wrong with the tooling that you use to create the EFI binary.
When I'm writing this, I'm thinking isn't then rel_size problem as well, that is 16 bytes in this case?
From: Heinrich Schuchardt xypron.glpk@gmx.de Sent: Mittwoch, 4. Dezember 2024 11:11 To: Aleksandar Gerasimovski Aleksandar.Gerasimovski@belden.com Cc: u-boot@lists.denx.de; Ilias Apalodimas ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
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,
Starting at bce26008 you have eight zero bytes with this structure:
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
"two relocations at offset bce26008 and bce2600a" cannot be true.
Best regards
Heinrich
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 mailto:aleksandar.gerasimovski@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.
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.
If VirtualSize = 16, then the reloc section has 16 bytes and not 12. A section size of less than 512 is invalid anyway. Something is wrong with the tooling that you use to create the EFI binary.
This is valid point, we will follow that track as well, but at the end is a boot-guard efi binary.
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; > /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
Correct, but look how the rel pointer is moving for 2 bytes: const uint16_t *relocs = (const uint16_t *)(rel + 1); ... rel = (const IMAGE_BASE_RELOCATION *)relocs;
And it ends at bce2600c, and then takes 7c00 9340 for size ; )
"two relocations at offset bce26008 and bce2600a" cannot be true.
This I really don't know, yes we need to follow with the tolling. In any case this image however is generated is let to run to the point when it break the u-boot, the image must be at least rejected earlier.
Regards, Aleksandar
From: Heinrich Schuchardt xypron.glpk@gmx.de Sent: Donnerstag, 5. Dezember 2024 09:35 To: Aleksandar Gerasimovski Aleksandar.Gerasimovski@belden.com Cc: u-boot@lists.denx.de; Ilias Apalodimas ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
On 05.12.24 09:13, Aleksandar Gerasimovski wrote:
Thanks for the replay,
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.
That's not what I see in the case that I try to explain, because the sec->Misc.VirtualSize virtual size is 16 bytes for this section .
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.
Correct, I see in the efi_load_pe code that section will be zeroed If sec->Misc.VirtualSize is bigger then sec->SizeOfRawData, but this is not a case for our image it's oposite. I can also confirm that in the real image the section is zeroed up to sec->SizeOfRawData, but the problem is that only the sec->Misc.VirtualSize (that is 16 bytes for my image) is copied to the "efi_reloc" location.
I'm far from really understanding the PE format, actually this is first time to deal with this and efi boot on Linux, however our EFI image is generated with a "standard" tools like bg_gen_unified_kernel by our yocto based distro generator, so in any case I see a nasty bug. Even if the image is wrong it should be rejected earlier and not overflowing even overwriting the u-boot code. It's a vulnerability.
Please, provide an example image for download to give the full picture.
I must think how to do that, where I could upload?
With "rel + 1 < end" you are not preserving the change but reverting it.
Well, this is the tricky part, in my understanding not because rel is moving by 2 bytes down in the loop.
I'm trying to describe the problem again, let's say in my image the problematic section is generated with sec->Misc.VirtualSize = 16 and sec->SizeOfRawData = 512, what will be provided to the efi_loader_relocate in this case is rel_size of 16 and only 16 bytes will be copied to efi_reloc location (see the " Load sections into RAM" logic in efi_load_pe), so the content of the RAM after efi_reloc + 16 stays unknown: bce26000: 3000 0000 000c 0000 0000 0000 0000 0000 -> reloc section bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20 -> random content
So efi_loader_relocate gets rel_size of 16 bytes for reloc section of 12 bytes, and this will overflow.
If VirtualSize = 16, then the reloc section has 16 bytes and not 12.
A section size of less than 512 is invalid anyway.
Something is wrong with the tooling that you use to create the EFI binary.
When I'm writing this, I'm thinking isn't then rel_size problem as well, that is 16 bytes in this case?
From: Heinrich Schuchardt mailto:xypron.glpk@gmx.de Sent: Mittwoch, 4. Dezember 2024 11:11 To: Aleksandar Gerasimovski mailto:Aleksandar.Gerasimovski@belden.com Cc: mailto:u-boot@lists.denx.de; Ilias Apalodimas mailto:ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
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,
Starting at bce26008 you have eight zero bytes with this structure:
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
"two relocations at offset bce26008 and bce2600a" cannot be true.
Best regards
Heinrich
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 mailto:aleksandar.gerasimovski@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.
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.
********************************************************************** 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.
On 05.12.24 09:58, Aleksandar Gerasimovski wrote:
If VirtualSize = 16, then the reloc section has 16 bytes and not 12. A section size of less than 512 is invalid anyway. Something is wrong with the tooling that you use to create the EFI binary.
This is valid point, we will follow that track as well, but at the end is a boot-guard efi binary.
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; > /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
Correct, but look how the rel pointer is moving for 2 bytes: const uint16_t *relocs = (const uint16_t *)(rel + 1); ... rel = (const IMAGE_BASE_RELOCATION *)relocs;
And it ends at bce2600c, and then takes 7c00 9340 for size ; )
"two relocations at offset bce26008 and bce2600a" cannot be true.
This I really don't know, yes we need to follow with the tolling. In any case this image however is generated is let to run to the point when it break the u-boot, the image must be at least rejected earlier.
Regards, Aleksandar
From: Heinrich Schuchardt xypron.glpk@gmx.de Sent: Donnerstag, 5. Dezember 2024 09:35 To: Aleksandar Gerasimovski Aleksandar.Gerasimovski@belden.com Cc: u-boot@lists.denx.de; Ilias Apalodimas ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
On 05.12.24 09:13, Aleksandar Gerasimovski wrote:
Thanks for the replay,
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.
That's not what I see in the case that I try to explain, because the sec->Misc.VirtualSize virtual size is 16 bytes for this section .
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.
Correct, I see in the efi_load_pe code that section will be zeroed If sec->Misc.VirtualSize is bigger then sec->SizeOfRawData, but this is not a case for our image it's oposite. I can also confirm that in the real image the section is zeroed up to sec->SizeOfRawData, but the problem is that only the sec->Misc.VirtualSize (that is 16 bytes for my image) is copied to the "efi_reloc" location.
I'm far from really understanding the PE format, actually this is first time to deal with this and efi boot on Linux, however our EFI image is generated with a "standard" tools like bg_gen_unified_kernel by our yocto based distro generator, so in any case I see a nasty bug. Even if the image is wrong it should be rejected earlier and not overflowing even overwriting the u-boot code. It's a vulnerability.
Please, provide an example image for download to give the full picture.
I must think how to do that, where I could upload?
With "rel + 1 < end" you are not preserving the change but reverting it.
Well, this is the tricky part, in my understanding not because rel is moving by 2 bytes down in the loop.
I'm trying to describe the problem again, let's say in my image the problematic section is generated with sec->Misc.VirtualSize = 16 and sec->SizeOfRawData = 512, what will be provided to the efi_loader_relocate in this case is rel_size of 16 and only 16 bytes will be copied to efi_reloc location (see the " Load sections into RAM" logic in efi_load_pe), so the content of the RAM after efi_reloc + 16 stays unknown: bce26000: 3000 0000 000c 0000 0000 0000 0000 0000 -> reloc section bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20 -> random content
So efi_loader_relocate gets rel_size of 16 bytes for reloc section of 12 bytes, and this will overflow.
If VirtualSize = 16, then the reloc section has 16 bytes and not 12.
A section size of less than 512 is invalid anyway.
Something is wrong with the tooling that you use to create the EFI binary.
When I'm writing this, I'm thinking isn't then rel_size problem as well, that is 16 bytes in this case?
From: Heinrich Schuchardt mailto:xypron.glpk@gmx.de Sent: Mittwoch, 4. Dezember 2024 11:11 To: Aleksandar Gerasimovski mailto:Aleksandar.Gerasimovski@belden.com Cc: mailto:u-boot@lists.denx.de; Ilias Apalodimas mailto:ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
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,
Starting at bce26008 you have eight zero bytes with this structure:
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
"two relocations at offset bce26008 and bce2600a" cannot be true.
Best regards
Heinrich
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.
const uint16_t *relocs = (const uint16_t *)(rel + 1); points to the first relocation of the relocation block.
rel + 1 must be inside the relocation section or it cannot point to a valid relocation.
In the line
i = (rel->SizeOfBlock - sizeof(*rel)) / sizeof(uint16_t);
we assume but do not check that
(u8 *)rel + rel->SizeOfBlock <= end
and
rel->SizeOfBlock >= sizeof(*rel)
A malformed relocation section could still lead to a buffer overrun.
Fixes: 997fc12ec91e ("efi_loader: do not miss last relocation block") Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
Signed-off-by: Aleksandar Gerasimovski mailto:aleksandar.gerasimovski@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.
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.
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.
Hello Folks,
What do we do with this patch, can we agree is mergeable?
Regards, Aleksandar
On 05.12.24 09:58, Aleksandar Gerasimovski wrote:
If VirtualSize = 16, then the reloc section has 16 bytes and not 12. A section size of less than 512 is invalid anyway. Something is wrong with the tooling that you use to create the EFI binary.
This is valid point, we will follow that track as well, but at the end is a boot-guard efi binary.
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; > /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
Correct, but look how the rel pointer is moving for 2 bytes: const uint16_t *relocs = (const uint16_t *)(rel + 1); ... rel = (const IMAGE_BASE_RELOCATION *)relocs;
And it ends at bce2600c, and then takes 7c00 9340 for size ; )
"two relocations at offset bce26008 and bce2600a" cannot be true.
This I really don't know, yes we need to follow with the tolling. In any case this image however is generated is let to run to the point when it break the u-boot, the image must be at least rejected earlier.
Regards, Aleksandar
From: Heinrich Schuchardt mailto:xypron.glpk@gmx.de Sent: Donnerstag, 5. Dezember 2024 09:35 To: Aleksandar Gerasimovski mailto:Aleksandar.Gerasimovski@belden.com Cc: mailto:u-boot@lists.denx.de; Ilias Apalodimas mailto:ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
On 05.12.24 09:13, Aleksandar Gerasimovski wrote:
Thanks for the replay,
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.
That's not what I see in the case that I try to explain, because the sec->Misc.VirtualSize virtual size is 16 bytes for this section .
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.
Correct, I see in the efi_load_pe code that section will be zeroed If sec->Misc.VirtualSize is bigger then sec->SizeOfRawData, but this is not a case for our image it's oposite. I can also confirm that in the real image the section is zeroed up to sec->SizeOfRawData, but the problem is that only the sec->Misc.VirtualSize (that is 16 bytes for my image) is copied to the "efi_reloc" location.
I'm far from really understanding the PE format, actually this is first time to deal with this and efi boot on Linux, however our EFI image is generated with a "standard" tools like bg_gen_unified_kernel by our yocto based distro generator, so in any case I see a nasty bug. Even if the image is wrong it should be rejected earlier and not overflowing even overwriting the u-boot code. It's a vulnerability.
Please, provide an example image for download to give the full picture.
I must think how to do that, where I could upload?
With "rel + 1 < end" you are not preserving the change but reverting it.
Well, this is the tricky part, in my understanding not because rel is moving by 2 bytes down in the loop.
I'm trying to describe the problem again, let's say in my image the problematic section is generated with sec->Misc.VirtualSize = 16 and sec->SizeOfRawData = 512, what will be provided to the efi_loader_relocate in this case is rel_size of 16 and only 16 bytes will be copied to efi_reloc location (see the " Load sections into RAM" logic in efi_load_pe), so the content of the RAM after efi_reloc + 16 stays unknown: bce26000: 3000 0000 000c 0000 0000 0000 0000 0000 -> reloc section bce26010: 7c00 9340 67e0 f900 1c00 0ea1 a400 0f20 -> random content
So efi_loader_relocate gets rel_size of 16 bytes for reloc section of 12 bytes, and this will overflow.
If VirtualSize = 16, then the reloc section has 16 bytes and not 12.
A section size of less than 512 is invalid anyway.
Something is wrong with the tooling that you use to create the EFI binary.
When I'm writing this, I'm thinking isn't then rel_size problem as well, that is 16 bytes in this case?
From: Heinrich Schuchardt mailto:xypron.glpk@gmx.de Sent: Mittwoch, 4. Dezember 2024 11:11 To: Aleksandar Gerasimovski mailto:Aleksandar.Gerasimovski@belden.com Cc: mailto:u-boot@lists.denx.de; Ilias Apalodimas mailto:ilias.apalodimas@linaro.org Subject: Re: [PATCH] efi_loader: fix pe reloc pointer overrun
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,
Starting at bce26008 you have eight zero bytes with this structure:
typedef struct _IMAGE_BASE_RELOCATION { uint32_t VirtualAddress; uint32_t SizeOfBlock; /* WORD TypeOffset[1]; */ } IMAGE_BASE_RELOCATION,*PIMAGE_BASE_RELOCATION;
The loop stops if rel->SizeOfBlock == 0.
"two relocations at offset bce26008 and bce2600a" cannot be true.
Best regards
Heinrich
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.
const uint16_t *relocs = (const uint16_t *)(rel + 1); points to the first relocation of the relocation block.
rel + 1 must be inside the relocation section or it cannot point to a valid relocation.
In the line
i = (rel->SizeOfBlock - sizeof(*rel)) / sizeof(uint16_t);
we assume but do not check that
(u8 *)rel + rel->SizeOfBlock <= end
and
rel->SizeOfBlock >= sizeof(*rel)
A malformed relocation section could still lead to a buffer overrun.
Fixes: 997fc12ec91e ("efi_loader: do not miss last relocation block") Reviewed-by: Heinrich Schuchardt mailto:xypron.glpk@gmx.de
Signed-off-by: Aleksandar Gerasimovski mailto:aleksandar.gerasimovski@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.
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.
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.
********************************************************************** 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.
participants (2)
-
Aleksandar Gerasimovski -
Heinrich Schuchardt