diff mbox series

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

Message ID 20210329135955.5804-1-ilias.apalodimas@linaro.org
State New
Headers show
Series efi_loader: convert void* to u8* on the tcg eventlog buffer | expand

Commit Message

Ilias Apalodimas March 29, 2021, 1:59 p.m. UTC 
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 *'

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

---
 lib/efi_loader/efi_tcg2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.31.0

Comments

Alexandru Gagniuc March 29, 2021, 6:41 p.m. UTC | #1 
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@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);

>
Ilias Apalodimas March 29, 2021, 8:34 p.m. UTC | #2 
On Mon, Mar 29, 2021 at 01:41:37PM -0500, Alex G. wrote:
> 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.


That's the general use case of alignment and casting, but in EFI mode,
unaligned accesses are required by the spec (if the architecture supports
them).  More over the buffer here is treated like a huge u8 array, never
accessed directly. All the changes are with get/put unaligned for that reason.
Events are densely packed for that reason, since the event size
can have an arbitrary size.
Anyway I did indeed miss 2 lines I had to change with put_unaligned. So I'll
end up doing a uintptr_t cast instead on v2.

> 

> 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.


Again you don't have to keep any alignment constraints in this case.
Regarding arithmetics and uintptr_t the standard says:
"The following type designates an unsigned integer type with the property 
that any valid pointer to void can be converted to this type, then converted 
back to pointer to void, and the result will compare equal to the original 
pointer". Is it preferable? Maybe, but it's not the only way to do
arithmetics in our case.

> 

> 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@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) ?


I'd use void ** const. I think void * const * is also correct?

> 

> >   	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?

> 


Because the whole point of that buffer is to hold arbitrary length events,
which can be unaligned by design.

Thanks
/Ilias


> >   	final_event->number_of_events = 0;

> >   	final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION;

> >   	event_log.final_pos = sizeof(*final_event);

> >
diff mbox series

Patch

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);
 	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;
 	final_event->number_of_events = 0;
 	final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION;
 	event_log.final_pos = sizeof(*final_event);