diff mbox series

efi_loader: capsule: remove authentication data

Message ID 20210510082034.44102-1-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: capsule: remove authentication data | expand

Commit Message

AKASHI Takahiro May 10, 2021, 8:20 a.m. UTC 
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(-)

-- 
2.31.0

Comments

Heinrich Schuchardt May 20, 2021, 2:37 a.m. UTC | #1 
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) {

>
AKASHI Takahiro July 20, 2021, 2:17 a.m. UTC | #2 
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) {

> > 

>
diff mbox series

Patch

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) {