[PATCH] Check for overflow when casting to VOID*

[Earl Chew]

When building for 32 bit architectures, casting from
64 bit EFI_PHYSICAL_ADDRESS to 32 bit VOID * elicits a
compiler warning (-Wint-to-pointer-cast).

Check that the cast will not drop high order bits by using
uintptr_t as an intermediate type to transform the address
quietly.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---
kernel-stub/main.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index c0be1f6..c2e9482 100644
--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -104,6 +104,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
BOOLEAN has_dtbs = FALSE;
const VOID *kernel_source;
EFI_PHYSICAL_ADDRESS kernel_buffer;
+ EFI_PHYSICAL_ADDRESS aligned_kernel_buffer;
const CHAR8 *fdt_compatible;
VOID *fdt, *alt_fdt = NULL;
EFI_IMAGE_ENTRY_POINT kernel_entry;
@@ -193,10 +194,16 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
goto cleanup_initrd;
}

- kernel_image.ImageBase = (VOID *)
- align_addr(kernel_buffer, pe_header->Opt.SectionAlignment);
+ aligned_kernel_buffer = align_addr(kernel_buffer, pe_header->Opt.SectionAlignment);
+
+ kernel_image.ImageBase = (VOID *) (uintptr_t) aligned_kernel_buffer;
kernel_image.ImageSize = kernel_section->VirtualSize;

+ if ((uintptr_t) kernel_image.ImageBase != aligned_kernel_buffer) {
+ error(L"Error aligning memory for kernel image", EFI_LOAD_ERROR);
+ goto cleanup_buffer;
+ }
+
CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
/* Clear the rest so that .bss is definitely zero. */
SetMem((UINT8 *) kernel_image.ImageBase + kernel_image.ImageSize,
--
2.39.1

[Jan Kiszka]

Let's not change the wrapping here.

> + kernel_image.ImageBase = (VOID *) (uintptr_t) aligned_kernel_buffer;
> kernel_image.ImageSize = kernel_section->VirtualSize;
>
> + if ((uintptr_t) kernel_image.ImageBase != aligned_kernel_buffer) {

How about testing this right after obtaining aligned_kernel_buffer,
checking weather for "(uintptr_t) aligned_kernel_buffer !=
aligned_kernel_buffer"?

> + error(L"Error aligning memory for kernel image", EFI_LOAD_ERROR);

"Overflow while aligning kernel image"?

> + goto cleanup_buffer;
> + }
> +
> CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
> /* Clear the rest so that .bss is definitely zero. */
> SetMem((UINT8 *) kernel_image.ImageBase + kernel_image.ImageSize,

Good catch. We are missing -Werror, and no one has actually reading this:

https://github.com/siemens/efibootguard/actions/runs/5809411513/job/15748132394

Means also that there are more issues.

Jan

--
Siemens AG, Technology
Linux Expert Center

[Jan Kiszka]

We should add

diff --git a/Makefile.am b/Makefile.am
index ef18ffd..c326c6f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -214,6 +214,7 @@ efi_cflags = \
-fno-stack-protector \
-Wsign-compare \
-DGNU_EFI_USE_MS_ABI \
+ -Werror
$(CFLAGS)

if ARCH_X86_64

Looking at the other cases in fdt.c, all are fine to be casted to
uintptr_t first as the address are coming 1:1 from an allocator, thus
are 32-bit.

[Earl Chew]

When building for 32 bit architectures, casting from
64 bit EFI_PHYSICAL_ADDRESS to 32 bit VOID * elicits a
compiler warning (-Wint-to-pointer-cast).

Check that the cast will not drop high order bits by using
uintptr_t as an intermediate type to transform the address
quietly.

Signed-off-by: Earl Chew <earl...@yahoo.com>
---

kernel-stub/main.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel-stub/main.c b/kernel-stub/main.c
index c0be1f6..1695eec 100644

--- a/kernel-stub/main.c
+++ b/kernel-stub/main.c
@@ -104,6 +104,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
BOOLEAN has_dtbs = FALSE;
const VOID *kernel_source;
EFI_PHYSICAL_ADDRESS kernel_buffer;
+ EFI_PHYSICAL_ADDRESS aligned_kernel_buffer;
const CHAR8 *fdt_compatible;
VOID *fdt, *alt_fdt = NULL;
EFI_IMAGE_ENTRY_POINT kernel_entry;

@@ -193,8 +194,14 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)

goto cleanup_initrd;
}

- kernel_image.ImageBase = (VOID *)

+ aligned_kernel_buffer =
align_addr(kernel_buffer, pe_header->Opt.SectionAlignment);

+ if ((uintptr_t) kernel_image.ImageBase != aligned_kernel_buffer) {

+ error(L"Alignment overflow for kernel image", EFI_LOAD_ERROR);

+ goto cleanup_buffer;
+ }
+

+ kernel_image.ImageBase = (VOID *) (uintptr_t) aligned_kernel_buffer;
kernel_image.ImageSize = kernel_section->VirtualSize;

CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);
--
2.39.1

[Jan Kiszka]

Thanks, applied.

[Jan Kiszka]

On 10.08.23 02:48, 'Earl Chew' via EFI Boot Guard wrote:

This was not adjusted as needed after moving things around:
kernel_image.ImageBase->aligned_kernel_buffer. Found while testing on a
real device. I'm fixing it in next (not yet master, force-pushing possible).

Jan

> + error(L"Alignment overflow for kernel image", EFI_LOAD_ERROR);
> + goto cleanup_buffer;
> + }
> +
> + kernel_image.ImageBase = (VOID *) (uintptr_t) aligned_kernel_buffer;
> kernel_image.ImageSize = kernel_section->VirtualSize;
>
> CopyMem(kernel_image.ImageBase, kernel_source, kernel_image.ImageSize);

--