On 09.08.23 16:31, 'Jan Kiszka' via EFI Boot Guard wrote:
> On 09.08.23 15:58, 'Earl Chew' via EFI Boot Guard wrote:
>> 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 <[email protected]>
>> ---
>> 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);
>> +
>
> 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.
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.
Jan
--
Siemens AG, Technology
Linux Expert Center
--
You received this message because you are subscribed to the Google Groups "EFI
Boot Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/8c33bca5-15bc-421f-9e2e-611cfbb0fce0%40siemens.com.
