[PATCH] efi_loader: Remove incorrect calls of EFI_CALL in TCG2

[PATCH] efi_loader: Remove incorrect calls of EFI_CALL in TCG2

[Ilias Apalodimas]

There is two unneeded EFI_CALL references in tcg2_measure_pe_image().
The first one in efi_search_protocol() and the second on in the device path
calculation.  The second isn't even a function we should be calling, but a
pointer assignment, which happens to work with the existing macro.

While at it switch the malloc call to a calloc, remove the unnecessary cast
and get rid of an unneeded if statement before copying the device path

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_tcg2.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 1319a8b37868..d026af2b2350 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-	ret = EFI_CALL(efi_search_protocol(&handle->header,
-					   &efi_guid_loaded_image_device_path,
-					   &handler));
+	ret = efi_search_protocol(&handle->header,
+				  &efi_guid_loaded_image_device_path, &handler);
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-	device_path = EFI_CALL(handler->protocol_interface);
+	device_path = handler->protocol_interface;
 	device_path_length = efi_dp_size(device_path);
 	if (device_path_length > 0) {
 		/* add end node size */
 		device_path_length += sizeof(struct efi_device_path);
 	}
 	event_size = sizeof(struct uefi_image_load_event) + device_path_length;
-	image_load_event = (struct uefi_image_load_event *)malloc(event_size);
+	image_load_event = calloc(1, event_size);
 	if (!image_load_event)
 		return EFI_OUT_OF_RESOURCES;
 
@@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
 		goto out;
 	}
 
-	if (device_path_length > 0) {
-		memcpy(image_load_event->device_path, device_path,
-		       device_path_length);
-	}
+	/* device_path_length might be zero */
+	memcpy(image_load_event->device_path, device_path, device_path_length);
 
 	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
 				    event_size, (u8 *)image_load_event);
-- 
2.33.0

Re: [PATCH] efi_loader: Remove incorrect calls of EFI_CALL in TCG2

[Heinrich Schuchardt]

On 9/8/21 11:30 PM, Ilias Apalodimas wrote:
> There is two unneeded EFI_CALL references in tcg2_measure_pe_image().
> The first one in efi_search_protocol() and the second on in the device path
> calculation.  The second isn't even a function we should be calling, but a
> pointer assignment, which happens to work with the existing macro.
>
> While at it switch the malloc call to a calloc, remove the unnecessary cast
> and get rid of an unneeded if statement before copying the device path
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   lib/efi_loader/efi_tcg2.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 1319a8b37868..d026af2b2350 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> -	ret = EFI_CALL(efi_search_protocol(&handle->header,
> -					   &efi_guid_loaded_image_device_path,
> -					   &handler));
> +	ret = efi_search_protocol(&handle->header,
> +				  &efi_guid_loaded_image_device_path, &handler);
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> -	device_path = EFI_CALL(handler->protocol_interface);
> +	device_path = handler->protocol_interface;
>   	device_path_length = efi_dp_size(device_path);
>   	if (device_path_length > 0) {
>   		/* add end node size */
>   		device_path_length += sizeof(struct efi_device_path);
>   	}
>   	event_size = sizeof(struct uefi_image_load_event) + device_path_length;
> -	image_load_event = (struct uefi_image_load_event *)malloc(event_size);
> +	image_load_event = calloc(1, event_size);
>   	if (!image_load_event)
>   		return EFI_OUT_OF_RESOURCES;
>
> @@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>   		goto out;
>   	}
>
> -	if (device_path_length > 0) {
> -		memcpy(image_load_event->device_path, device_path,
> -		       device_path_length);
> -	}
> +	/* device_path_length might be zero */
> +	memcpy(image_load_event->device_path, device_path, device_path_length);
>
>   	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
>   				    event_size, (u8 *)image_load_event);
>

Re: [PATCH] efi_loader: Remove incorrect calls of EFI_CALL in TCG2

[AKASHI Takahiro]

Heinrich,

On Thu, Sep 09, 2021 at 12:30:49AM +0300, Ilias Apalodimas wrote:
> There is two unneeded EFI_CALL references in tcg2_measure_pe_image().
> The first one in efi_search_protocol() and the second on in the device path
> calculation.  The second isn't even a function we should be calling, but a
> pointer assignment, which happens to work with the existing macro.

It is a quite common mistake.
For most people, it is not very trivial when we should use EFI_CALL
and when should not.
Do you think that we should leave a note somewhere?

It would be much better if we can check any occurrence of mismatch,
say using EFI_CALL for a non-EFIAPI function, at compile time.

-Takahiro Akashi

> While at it switch the malloc call to a calloc, remove the unnecessary cast
> and get rid of an unneeded if statement before copying the device path
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  lib/efi_loader/efi_tcg2.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 1319a8b37868..d026af2b2350 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -839,20 +839,19 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> -	ret = EFI_CALL(efi_search_protocol(&handle->header,
> -					   &efi_guid_loaded_image_device_path,
> -					   &handler));
> +	ret = efi_search_protocol(&handle->header,
> +				  &efi_guid_loaded_image_device_path, &handler);
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> -	device_path = EFI_CALL(handler->protocol_interface);
> +	device_path = handler->protocol_interface;
>  	device_path_length = efi_dp_size(device_path);
>  	if (device_path_length > 0) {
>  		/* add end node size */
>  		device_path_length += sizeof(struct efi_device_path);
>  	}
>  	event_size = sizeof(struct uefi_image_load_event) + device_path_length;
> -	image_load_event = (struct uefi_image_load_event *)malloc(event_size);
> +	image_load_event = calloc(1, event_size);
>  	if (!image_load_event)
>  		return EFI_OUT_OF_RESOURCES;
>  
> @@ -875,10 +874,8 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
>  		goto out;
>  	}
>  
> -	if (device_path_length > 0) {
> -		memcpy(image_load_event->device_path, device_path,
> -		       device_path_length);
> -	}
> +	/* device_path_length might be zero */
> +	memcpy(image_load_event->device_path, device_path, device_path_length);
>  
>  	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
>  				    event_size, (u8 *)image_load_event);
> -- 
> 2.33.0
>