[PATCH] efi_loader: capsule: remove authentication data

[AKASHI Takahiro]

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