All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] efi_loader: capsule: remove authentication data
@ 2021-05-10  8:20 AKASHI Takahiro
  2021-05-20  2:37 ` Heinrich Schuchardt
  0 siblings, 1 reply; 5+ messages in thread
From: AKASHI Takahiro @ 2021-05-10  8:20 UTC (permalink / raw)
  To: u-boot

If capsule authentication is disabled and yet a capsule file is signed,
its signature must be removed from image data to flush.
Otherwise, the firmware will be corrupted after update.

Fixes: 04be98bd6bcf ("efi: capsule: Add support for uefi capsule
	authentication")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_capsule.c | 70 +++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index b0dffd3ac9ce..5d156c730faa 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -206,6 +206,39 @@ skip:
 	return NULL;
 }
 
+/**
+ * efi_remove_auth_hdr - remove authentication data from image
+ * @image:	Pointer to pointer to Image
+ * @image_size:	Pointer to Image size
+ *
+ * Remove the authentication data from image if possible.
+ * Update @image and @image_size.
+ *
+ * Return:		status code
+ */
+static efi_status_t efi_remove_auth_hdr(void **image, efi_uintn_t *image_size)
+{
+	struct efi_firmware_image_authentication *auth_hdr;
+	efi_status_t ret = EFI_INVALID_PARAMETER;
+
+	auth_hdr = (struct efi_firmware_image_authentication *)*image;
+	if (*image_size < sizeof(*auth_hdr))
+		goto out;
+
+	if (auth_hdr->auth_info.hdr.dwLength <=
+	    offsetof(struct win_certificate_uefi_guid, cert_data))
+		goto out;
+
+	*image = (uint8_t *)*image + sizeof(auth_hdr->monotonic_count) +
+		auth_hdr->auth_info.hdr.dwLength;
+	*image_size = *image_size - auth_hdr->auth_info.hdr.dwLength -
+		sizeof(auth_hdr->monotonic_count);
+
+	ret = EFI_SUCCESS;
+out:
+	return ret;
+}
+
 #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 
 #if defined(CONFIG_EFI_PKEY_DTB_EMBED)
@@ -271,21 +304,15 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 	if (capsule == NULL || capsule_size == 0)
 		goto out;
 
-	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
-	if (capsule_size < sizeof(*auth_hdr))
-		goto out;
-
-	if (auth_hdr->auth_info.hdr.dwLength <=
-	    offsetof(struct win_certificate_uefi_guid, cert_data))
+	*image = (uint8_t *)capsule;
+	*image_size = capsule_size;
+	if (efi_remove_auth_hdr(image, image_size) != EFI_SUCCESS)
 		goto out;
 
+	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
 	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
 		goto out;
 
-	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
-		auth_hdr->auth_info.hdr.dwLength;
-	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
-		sizeof(auth_hdr->monotonic_count);
 	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
 	       sizeof(monotonic_count));
 
@@ -367,7 +394,7 @@ static efi_status_t efi_capsule_update_firmware(
 {
 	struct efi_firmware_management_capsule_header *capsule;
 	struct efi_firmware_management_capsule_image_header *image;
-	size_t capsule_size;
+	size_t capsule_size, image_binary_size;
 	void *image_binary, *vendor_code;
 	efi_handle_t *handles;
 	efi_uintn_t no_handles;
@@ -429,13 +456,30 @@ static efi_status_t efi_capsule_update_firmware(
 		}
 
 		/* do update */
+		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
+		    !(image->image_capsule_support &
+				CAPSULE_SUPPORT_AUTHENTICATION)) {
+			/* no signature */
+			ret = EFI_SECURITY_VIOLATION;
+			goto out;
+		}
+
 		image_binary = (void *)image + sizeof(*image);
-		vendor_code = image_binary + image->update_image_size;
+		image_binary_size = image->update_image_size;
+		vendor_code = image_binary + image_binary_size;
+		if (!IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
+		    (image->image_capsule_support &
+				CAPSULE_SUPPORT_AUTHENTICATION)) {
+			ret = efi_remove_auth_hdr(&image_binary,
+						  &image_binary_size);
+			if (ret != EFI_SUCCESS)
+				goto out;
+		}
 
 		abort_reason = NULL;
 		ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index,
 					      image_binary,
-					      image->update_image_size,
+					      image_binary_size,
 					      vendor_code, NULL,
 					      &abort_reason));
 		if (ret != EFI_SUCCESS) {
-- 
2.31.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH] efi_loader: capsule: remove authentication data
  2021-05-10  8:20 [PATCH] efi_loader: capsule: remove authentication data AKASHI Takahiro
@ 2021-05-20  2:37 ` Heinrich Schuchardt
  2021-07-20  2:17   ` AKASHI Takahiro
  0 siblings, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2021-05-20  2:37 UTC (permalink / raw)
  To: u-boot

On 5/10/21 10:20 AM, AKASHI Takahiro wrote:
> If capsule authentication is disabled and yet a capsule file is signed,
> its signature must be removed from image data to flush.
> Otherwise, the firmware will be corrupted after update.
>
> Fixes: 04be98bd6bcf ("efi: capsule: Add support for uefi capsule
> 	authentication")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   lib/efi_loader/efi_capsule.c | 70 +++++++++++++++++++++++++++++-------
>   1 file changed, 57 insertions(+), 13 deletions(-)
>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index b0dffd3ac9ce..5d156c730faa 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -206,6 +206,39 @@ skip:
>   	return NULL;
>   }
>
> +/**
> + * efi_remove_auth_hdr - remove authentication data from image
> + * @image:	Pointer to pointer to Image
> + * @image_size:	Pointer to Image size
> + *
> + * Remove the authentication data from image if possible.
> + * Update @image and @image_size.
> + *
> + * Return:		status code
> + */
> +static efi_status_t efi_remove_auth_hdr(void **image, efi_uintn_t *image_size)
> +{
> +	struct efi_firmware_image_authentication *auth_hdr;
> +	efi_status_t ret = EFI_INVALID_PARAMETER;
> +
> +	auth_hdr = (struct efi_firmware_image_authentication *)*image;
> +	if (*image_size < sizeof(*auth_hdr))
> +		goto out;
> +
> +	if (auth_hdr->auth_info.hdr.dwLength <=
> +	    offsetof(struct win_certificate_uefi_guid, cert_data))
> +		goto out;
> +
> +	*image = (uint8_t *)*image + sizeof(auth_hdr->monotonic_count) +
> +		auth_hdr->auth_info.hdr.dwLength;
> +	*image_size = *image_size - auth_hdr->auth_info.hdr.dwLength -
> +		sizeof(auth_hdr->monotonic_count);
> +
> +	ret = EFI_SUCCESS;
> +out:
> +	return ret;
> +}
> +
>   #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
>
>   #if defined(CONFIG_EFI_PKEY_DTB_EMBED)

The string EFI_PKEY_DTB_EMBED does not yet exist in U-boot. The patch
seems to depend on Sughosh series "Add support for embedding public key
in platform's dtb". Please, state dependencies in future patches.

In the discussion of Sughosh's patch series the conclusion was that
embedding the public key in the DTB is not preferable.

I will mark this patch as not applicable.

Best regards

Heinrich

> @@ -271,21 +304,15 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>   	if (capsule == NULL || capsule_size == 0)
>   		goto out;
>
> -	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> -	if (capsule_size < sizeof(*auth_hdr))
> -		goto out;
> -
> -	if (auth_hdr->auth_info.hdr.dwLength <=
> -	    offsetof(struct win_certificate_uefi_guid, cert_data))
> +	*image = (uint8_t *)capsule;
> +	*image_size = capsule_size;
> +	if (efi_remove_auth_hdr(image, image_size) != EFI_SUCCESS)
>   		goto out;
>
> +	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
>   	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
>   		goto out;
>
> -	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> -		auth_hdr->auth_info.hdr.dwLength;
> -	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> -		sizeof(auth_hdr->monotonic_count);
>   	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
>   	       sizeof(monotonic_count));
>
> @@ -367,7 +394,7 @@ static efi_status_t efi_capsule_update_firmware(
>   {
>   	struct efi_firmware_management_capsule_header *capsule;
>   	struct efi_firmware_management_capsule_image_header *image;
> -	size_t capsule_size;
> +	size_t capsule_size, image_binary_size;
>   	void *image_binary, *vendor_code;
>   	efi_handle_t *handles;
>   	efi_uintn_t no_handles;
> @@ -429,13 +456,30 @@ static efi_status_t efi_capsule_update_firmware(
>   		}
>
>   		/* do update */
> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> +		    !(image->image_capsule_support &
> +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> +			/* no signature */
> +			ret = EFI_SECURITY_VIOLATION;
> +			goto out;
> +		}
> +
>   		image_binary = (void *)image + sizeof(*image);
> -		vendor_code = image_binary + image->update_image_size;
> +		image_binary_size = image->update_image_size;
> +		vendor_code = image_binary + image_binary_size;
> +		if (!IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> +		    (image->image_capsule_support &
> +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> +			ret = efi_remove_auth_hdr(&image_binary,
> +						  &image_binary_size);
> +			if (ret != EFI_SUCCESS)
> +				goto out;
> +		}
>
>   		abort_reason = NULL;
>   		ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index,
>   					      image_binary,
> -					      image->update_image_size,
> +					      image_binary_size,
>   					      vendor_code, NULL,
>   					      &abort_reason));
>   		if (ret != EFI_SUCCESS) {
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] efi_loader: capsule: remove authentication data
  2021-05-20  2:37 ` Heinrich Schuchardt
@ 2021-07-20  2:17   ` AKASHI Takahiro
  0 siblings, 0 replies; 5+ messages in thread
From: AKASHI Takahiro @ 2021-07-20  2:17 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Ilias Apalodimas, Alexander Graf, Sughosh Ganu, U-Boot Mailing List

On Thu, May 20, 2021 at 04:37:58AM +0200, Heinrich Schuchardt wrote:
> On 5/10/21 10:20 AM, AKASHI Takahiro wrote:
> > If capsule authentication is disabled and yet a capsule file is signed,
> > its signature must be removed from image data to flush.
> > Otherwise, the firmware will be corrupted after update.
> > 
> > Fixes: 04be98bd6bcf ("efi: capsule: Add support for uefi capsule
> > 	authentication")
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   lib/efi_loader/efi_capsule.c | 70 +++++++++++++++++++++++++++++-------
> >   1 file changed, 57 insertions(+), 13 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index b0dffd3ac9ce..5d156c730faa 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -206,6 +206,39 @@ skip:
> >   	return NULL;
> >   }
> > 
> > +/**
> > + * efi_remove_auth_hdr - remove authentication data from image
> > + * @image:	Pointer to pointer to Image
> > + * @image_size:	Pointer to Image size
> > + *
> > + * Remove the authentication data from image if possible.
> > + * Update @image and @image_size.
> > + *
> > + * Return:		status code
> > + */
> > +static efi_status_t efi_remove_auth_hdr(void **image, efi_uintn_t *image_size)
> > +{
> > +	struct efi_firmware_image_authentication *auth_hdr;
> > +	efi_status_t ret = EFI_INVALID_PARAMETER;
> > +
> > +	auth_hdr = (struct efi_firmware_image_authentication *)*image;
> > +	if (*image_size < sizeof(*auth_hdr))
> > +		goto out;
> > +
> > +	if (auth_hdr->auth_info.hdr.dwLength <=
> > +	    offsetof(struct win_certificate_uefi_guid, cert_data))
> > +		goto out;
> > +
> > +	*image = (uint8_t *)*image + sizeof(auth_hdr->monotonic_count) +
> > +		auth_hdr->auth_info.hdr.dwLength;
> > +	*image_size = *image_size - auth_hdr->auth_info.hdr.dwLength -
> > +		sizeof(auth_hdr->monotonic_count);
> > +
> > +	ret = EFI_SUCCESS;
> > +out:
> > +	return ret;
> > +}
> > +
> >   #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > 
> >   #if defined(CONFIG_EFI_PKEY_DTB_EMBED)
> 
> The string EFI_PKEY_DTB_EMBED does not yet exist in U-boot. The patch
> seems to depend on Sughosh series "Add support for embedding public key
> in platform's dtb". Please, state dependencies in future patches.
> 
> In the discussion of Sughosh's patch series the conclusion was that
> embedding the public key in the DTB is not preferable.
> 
> I will mark this patch as not applicable.

Now that Ilias posted a patch, I will respin my patch.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > @@ -271,21 +304,15 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
> >   	if (capsule == NULL || capsule_size == 0)
> >   		goto out;
> > 
> > -	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > -	if (capsule_size < sizeof(*auth_hdr))
> > -		goto out;
> > -
> > -	if (auth_hdr->auth_info.hdr.dwLength <=
> > -	    offsetof(struct win_certificate_uefi_guid, cert_data))
> > +	*image = (uint8_t *)capsule;
> > +	*image_size = capsule_size;
> > +	if (efi_remove_auth_hdr(image, image_size) != EFI_SUCCESS)
> >   		goto out;
> > 
> > +	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> >   	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
> >   		goto out;
> > 
> > -	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> > -		auth_hdr->auth_info.hdr.dwLength;
> > -	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > -		sizeof(auth_hdr->monotonic_count);
> >   	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> >   	       sizeof(monotonic_count));
> > 
> > @@ -367,7 +394,7 @@ static efi_status_t efi_capsule_update_firmware(
> >   {
> >   	struct efi_firmware_management_capsule_header *capsule;
> >   	struct efi_firmware_management_capsule_image_header *image;
> > -	size_t capsule_size;
> > +	size_t capsule_size, image_binary_size;
> >   	void *image_binary, *vendor_code;
> >   	efi_handle_t *handles;
> >   	efi_uintn_t no_handles;
> > @@ -429,13 +456,30 @@ static efi_status_t efi_capsule_update_firmware(
> >   		}
> > 
> >   		/* do update */
> > +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> > +		    !(image->image_capsule_support &
> > +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> > +			/* no signature */
> > +			ret = EFI_SECURITY_VIOLATION;
> > +			goto out;
> > +		}
> > +
> >   		image_binary = (void *)image + sizeof(*image);
> > -		vendor_code = image_binary + image->update_image_size;
> > +		image_binary_size = image->update_image_size;
> > +		vendor_code = image_binary + image_binary_size;
> > +		if (!IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> > +		    (image->image_capsule_support &
> > +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> > +			ret = efi_remove_auth_hdr(&image_binary,
> > +						  &image_binary_size);
> > +			if (ret != EFI_SUCCESS)
> > +				goto out;
> > +		}
> > 
> >   		abort_reason = NULL;
> >   		ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index,
> >   					      image_binary,
> > -					      image->update_image_size,
> > +					      image_binary_size,
> >   					      vendor_code, NULL,
> >   					      &abort_reason));
> >   		if (ret != EFI_SUCCESS) {
> > 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] efi_loader: capsule: remove authentication data
  2021-07-20  5:44 AKASHI Takahiro
@ 2021-07-20  5:51 ` AKASHI Takahiro
  0 siblings, 0 replies; 5+ messages in thread
From: AKASHI Takahiro @ 2021-07-20  5:51 UTC (permalink / raw)
  To: xypron.glpk, agraf; +Cc: u-boot

Please ignore this patch as I mistakenly sent out v1.

-Takahiro Akashi

On Tue, Jul 20, 2021 at 02:44:42PM +0900, AKASHI Takahiro wrote:
> If capsule authentication is disabled and yet a capsule file is signed,
> its signature must be removed from image data to flush.
> Otherwise, the firmware will be corrupted after update.
> 
> Fixes: 04be98bd6bcf ("efi: capsule: Add support for uefi capsule
> 	authentication")
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_capsule.c | 70 +++++++++++++++++++++++++++++-------
>  1 file changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index b0dffd3ac9ce..5d156c730faa 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -206,6 +206,39 @@ skip:
>  	return NULL;
>  }
>  
> +/**
> + * efi_remove_auth_hdr - remove authentication data from image
> + * @image:	Pointer to pointer to Image
> + * @image_size:	Pointer to Image size
> + *
> + * Remove the authentication data from image if possible.
> + * Update @image and @image_size.
> + *
> + * Return:		status code
> + */
> +static efi_status_t efi_remove_auth_hdr(void **image, efi_uintn_t *image_size)
> +{
> +	struct efi_firmware_image_authentication *auth_hdr;
> +	efi_status_t ret = EFI_INVALID_PARAMETER;
> +
> +	auth_hdr = (struct efi_firmware_image_authentication *)*image;
> +	if (*image_size < sizeof(*auth_hdr))
> +		goto out;
> +
> +	if (auth_hdr->auth_info.hdr.dwLength <=
> +	    offsetof(struct win_certificate_uefi_guid, cert_data))
> +		goto out;
> +
> +	*image = (uint8_t *)*image + sizeof(auth_hdr->monotonic_count) +
> +		auth_hdr->auth_info.hdr.dwLength;
> +	*image_size = *image_size - auth_hdr->auth_info.hdr.dwLength -
> +		sizeof(auth_hdr->monotonic_count);
> +
> +	ret = EFI_SUCCESS;
> +out:
> +	return ret;
> +}
> +
>  #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
>  
>  #if defined(CONFIG_EFI_PKEY_DTB_EMBED)
> @@ -271,21 +304,15 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
>  	if (capsule == NULL || capsule_size == 0)
>  		goto out;
>  
> -	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> -	if (capsule_size < sizeof(*auth_hdr))
> -		goto out;
> -
> -	if (auth_hdr->auth_info.hdr.dwLength <=
> -	    offsetof(struct win_certificate_uefi_guid, cert_data))
> +	*image = (uint8_t *)capsule;
> +	*image_size = capsule_size;
> +	if (efi_remove_auth_hdr(image, image_size) != EFI_SUCCESS)
>  		goto out;
>  
> +	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
>  	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
>  		goto out;
>  
> -	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> -		auth_hdr->auth_info.hdr.dwLength;
> -	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> -		sizeof(auth_hdr->monotonic_count);
>  	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
>  	       sizeof(monotonic_count));
>  
> @@ -367,7 +394,7 @@ static efi_status_t efi_capsule_update_firmware(
>  {
>  	struct efi_firmware_management_capsule_header *capsule;
>  	struct efi_firmware_management_capsule_image_header *image;
> -	size_t capsule_size;
> +	size_t capsule_size, image_binary_size;
>  	void *image_binary, *vendor_code;
>  	efi_handle_t *handles;
>  	efi_uintn_t no_handles;
> @@ -429,13 +456,30 @@ static efi_status_t efi_capsule_update_firmware(
>  		}
>  
>  		/* do update */
> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> +		    !(image->image_capsule_support &
> +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> +			/* no signature */
> +			ret = EFI_SECURITY_VIOLATION;
> +			goto out;
> +		}
> +
>  		image_binary = (void *)image + sizeof(*image);
> -		vendor_code = image_binary + image->update_image_size;
> +		image_binary_size = image->update_image_size;
> +		vendor_code = image_binary + image_binary_size;
> +		if (!IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
> +		    (image->image_capsule_support &
> +				CAPSULE_SUPPORT_AUTHENTICATION)) {
> +			ret = efi_remove_auth_hdr(&image_binary,
> +						  &image_binary_size);
> +			if (ret != EFI_SUCCESS)
> +				goto out;
> +		}
>  
>  		abort_reason = NULL;
>  		ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index,
>  					      image_binary,
> -					      image->update_image_size,
> +					      image_binary_size,
>  					      vendor_code, NULL,
>  					      &abort_reason));
>  		if (ret != EFI_SUCCESS) {
> -- 
> 2.31.0
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] efi_loader: capsule: remove authentication data
@ 2021-07-20  5:44 AKASHI Takahiro
  2021-07-20  5:51 ` AKASHI Takahiro
  0 siblings, 1 reply; 5+ messages in thread
From: AKASHI Takahiro @ 2021-07-20  5:44 UTC (permalink / raw)
  To: xypron.glpk, agraf; +Cc: u-boot, AKASHI Takahiro

If capsule authentication is disabled and yet a capsule file is signed,
its signature must be removed from image data to flush.
Otherwise, the firmware will be corrupted after update.

Fixes: 04be98bd6bcf ("efi: capsule: Add support for uefi capsule
	authentication")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_capsule.c | 70 +++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index b0dffd3ac9ce..5d156c730faa 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -206,6 +206,39 @@ skip:
 	return NULL;
 }
 
+/**
+ * efi_remove_auth_hdr - remove authentication data from image
+ * @image:	Pointer to pointer to Image
+ * @image_size:	Pointer to Image size
+ *
+ * Remove the authentication data from image if possible.
+ * Update @image and @image_size.
+ *
+ * Return:		status code
+ */
+static efi_status_t efi_remove_auth_hdr(void **image, efi_uintn_t *image_size)
+{
+	struct efi_firmware_image_authentication *auth_hdr;
+	efi_status_t ret = EFI_INVALID_PARAMETER;
+
+	auth_hdr = (struct efi_firmware_image_authentication *)*image;
+	if (*image_size < sizeof(*auth_hdr))
+		goto out;
+
+	if (auth_hdr->auth_info.hdr.dwLength <=
+	    offsetof(struct win_certificate_uefi_guid, cert_data))
+		goto out;
+
+	*image = (uint8_t *)*image + sizeof(auth_hdr->monotonic_count) +
+		auth_hdr->auth_info.hdr.dwLength;
+	*image_size = *image_size - auth_hdr->auth_info.hdr.dwLength -
+		sizeof(auth_hdr->monotonic_count);
+
+	ret = EFI_SUCCESS;
+out:
+	return ret;
+}
+
 #if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 
 #if defined(CONFIG_EFI_PKEY_DTB_EMBED)
@@ -271,21 +304,15 @@ efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_s
 	if (capsule == NULL || capsule_size == 0)
 		goto out;
 
-	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
-	if (capsule_size < sizeof(*auth_hdr))
-		goto out;
-
-	if (auth_hdr->auth_info.hdr.dwLength <=
-	    offsetof(struct win_certificate_uefi_guid, cert_data))
+	*image = (uint8_t *)capsule;
+	*image_size = capsule_size;
+	if (efi_remove_auth_hdr(image, image_size) != EFI_SUCCESS)
 		goto out;
 
+	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
 	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
 		goto out;
 
-	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
-		auth_hdr->auth_info.hdr.dwLength;
-	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
-		sizeof(auth_hdr->monotonic_count);
 	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
 	       sizeof(monotonic_count));
 
@@ -367,7 +394,7 @@ static efi_status_t efi_capsule_update_firmware(
 {
 	struct efi_firmware_management_capsule_header *capsule;
 	struct efi_firmware_management_capsule_image_header *image;
-	size_t capsule_size;
+	size_t capsule_size, image_binary_size;
 	void *image_binary, *vendor_code;
 	efi_handle_t *handles;
 	efi_uintn_t no_handles;
@@ -429,13 +456,30 @@ static efi_status_t efi_capsule_update_firmware(
 		}
 
 		/* do update */
+		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
+		    !(image->image_capsule_support &
+				CAPSULE_SUPPORT_AUTHENTICATION)) {
+			/* no signature */
+			ret = EFI_SECURITY_VIOLATION;
+			goto out;
+		}
+
 		image_binary = (void *)image + sizeof(*image);
-		vendor_code = image_binary + image->update_image_size;
+		image_binary_size = image->update_image_size;
+		vendor_code = image_binary + image_binary_size;
+		if (!IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
+		    (image->image_capsule_support &
+				CAPSULE_SUPPORT_AUTHENTICATION)) {
+			ret = efi_remove_auth_hdr(&image_binary,
+						  &image_binary_size);
+			if (ret != EFI_SUCCESS)
+				goto out;
+		}
 
 		abort_reason = NULL;
 		ret = EFI_CALL(fmp->set_image(fmp, image->update_image_index,
 					      image_binary,
-					      image->update_image_size,
+					      image_binary_size,
 					      vendor_code, NULL,
 					      &abort_reason));
 		if (ret != EFI_SUCCESS) {
-- 
2.31.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-20  5:51 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10  8:20 [PATCH] efi_loader: capsule: remove authentication data AKASHI Takahiro
2021-05-20  2:37 ` Heinrich Schuchardt
2021-07-20  2:17   ` AKASHI Takahiro
2021-07-20  5:44 AKASHI Takahiro
2021-07-20  5:51 ` AKASHI Takahiro

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.