[PATCH] efi_loader: convert void* to u8* on the tcg eventlog buffer

[Alex G.]

Hi Ilias,

On 3/29/21 8:59 AM, Ilias Apalodimas wrote:
> Although ptr arithmetics are allowed with extensions in gcc, they
> are not allowed by the C spec.  So switch the 'void *' containing our
> eventlog buffer into 'u8 *'

NAK. This patch is in my opinion wrong.

In C, void * can point to anything. The same is not true of a u8*. You 
can take a generic pointer, assign it to a void pointer, then assign 
back to the original type. u8* has an alignment of 1. While the compiler 
can now do math on u8*, it only has to worry about an alignment of 1. As 
you can imagine, this can have unintended consequences when the data 
behind the pointer is not actually an array of u8.

The correct way in C to do pointer arithmetic is to go via uintptr_t. 
You still have to manually make sure you keep alignment and other 
constraints, but this and only this is portable.

You can use either void * or the a pointer to the actual type. It's up 
to you whether you want to use a uintptr_t in struct event_log_buffer, 
or cast to uintptr_t when you do the actual pointer math, then cast back 
to void *.

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
>   lib/efi_loader/efi_tcg2.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 09046844c723..d5213586cb9c 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -23,8 +23,8 @@
>   #include <hexdump.h>
>   
>   struct event_log_buffer {
> -	void *buffer;
> -	void *final_buffer;
> +	u8 *buffer;
> +	u8 *final_buffer;
>   	size_t pos; /* eventlog position */
>   	size_t final_pos; /* final events config table position */
>   	size_t last_event_size;
> @@ -990,12 +990,12 @@ static efi_status_t create_final_event(void)
>   	 * EFI_CONFIGURATION_TABLE
>   	 */
>   	ret = efi_allocate_pool(EFI_ACPI_MEMORY_NVS, TPM2_EVENT_LOG_SIZE,
> -				&event_log.final_buffer);
> +				(void **)&event_log.final_buffer);

Now this sort of cast I would rather avoid. Double pointer casts rarely 
do what you want them to do. For once, if someone ever changes 
final_buffer to be a const pointer, you've lost that in this cast. The 
compiler likely won't warn you. So you've just exposed this code to 
possible silent failures.

BTW, without looking at stack overflow would that be a (const void **), 
(void * const *), or (void ** const) ?

>   	if (ret != EFI_SUCCESS)
>   		goto out;
>   
>   	memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE);
> -	final_event = event_log.final_buffer;
> +	final_event = (struct efi_tcg2_final_events_table *)event_log.final_buffer;

This sort of cast is also undefined. Why not make event_log.final_buffer 
a struct efi_tcg2_final_events_table * in the first place?

>   	final_event->number_of_events = 0;
>   	final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION;
>   	event_log.final_pos = sizeof(*final_event);
>