Re: [PATCH] efi_loader: capsule: enforce guid check in api and capsule_on_disk

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Hi Michal,

On Thu, Jul 27, 2023 at 10:53:44AM +0200, Michal Simek wrote:
> 
> 
> On 7/27/23 02:38, AKASHI Takahiro wrote:
> > While UPDATE_CAPSULE api is not fully implemented, this interface and
> > capsule-on-disk feature should behave in the same way, especially in
> > handling an empty capsule for fwu multibank, for future enhancement.
> > 
> > So move the guid check into efi_capsule_update_firmware().
> > 
> > Fixed: commit a6aafce494ab ("efi_loader: use efi_update_capsule_firmware()
> > 	for capsule on disk")
> 
> just fyi: b4 mess this.
> You should likely put it on the same line and ignore line limit.
> 
> This is how this ends up.
> 
> handling an empty capsule for fwu multibank, for future enhancement.
> 
> So move the guid check into efi_capsule_update_firmware().
> 
>         for capsule on disk")
> 
> Fixed: commit a6aafce494ab ("efi_loader: use efi_update_capsule_firmware()
> Reported-by: Michal Simek <michal.simek@amd.com>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Link: https://lore.kernel.org/r/20230727003800.25105-1-takahiro.akashi@linaro.org
> 
> 
> 
> 
> > Reported-by: Michal Simek <michal.simek@amd.com>
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   lib/efi_loader/efi_capsule.c | 17 ++++++++---------
> >   1 file changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 7a6f195cbc02..ddf8153e0982 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -581,6 +581,13 @@ static efi_status_t efi_capsule_update_firmware(
> >   		fw_accept_os = capsule_data->flags & FW_ACCEPT_OS ? 0x1 : 0x0;
> >   	}
> > +	if (guidcmp(&capsule_data->capsule_guid,
> > +		    &efi_guid_firmware_management_capsule_id)) {
> > +		log_err("Unsupported capsule type: %pUs\n",
> > +			&capsule_data->capsule_guid);
> > +		return EFI_UNSUPPORTED;
> > +	}
> > +
> >   	/* sanity check */
> >   	if (capsule_data->header_size < sizeof(*capsule) ||
> >   	    capsule_data->header_size >= capsule_data->capsule_image_size)
> > @@ -751,15 +758,7 @@ efi_status_t EFIAPI efi_update_capsule(
> >   		log_debug("Capsule[%d] (guid:%pUs)\n",
> >   			  i, &capsule->capsule_guid);
> > -		if (!guidcmp(&capsule->capsule_guid,
> > -			     &efi_guid_firmware_management_capsule_id)) {
> > -			ret  = efi_capsule_update_firmware(capsule);
> > -		} else {
> > -			log_err("Unsupported capsule type: %pUs\n",
> > -				&capsule->capsule_guid);
> > -			ret = EFI_UNSUPPORTED;
> > -		}
> > -
> > +		ret  = efi_capsule_update_firmware(capsule);
> >   		if (ret != EFI_SUCCESS)
> >   			goto out;
> >   	}
> 
> I have no problem with this patch because it works as the previous one. When
> commit message is fixed feel free to add
> Tested-by: Michal Simek <michal.simek@amd.com>

I hope that the maintainer would take care of it when he tries to merge
the patch.

> And regarding empty capsule functionality with A/B.
> I boot from A. Download capsules and run disk-update to get to Image B and
> trial state and I can download and apply acceptance capsule by hand via
> efidebug capsule update <addr>. That works fine for acceptance capsule is
> reflected via fwu in mdata.
> When I apply revert capsule there is nothing visible in mdata and I think it
> should. The only visibility is that it resets to A system. Is this the only
> intention of revert capsules?
> (keep in mind that I use two images per bank).
> 
> Empty capsules are just accepted only in trial state which is understandable.
> 
> And I also see that with latest master branch capsule on disk feature is not
> working properly. Capsule are not processed at all. Can you please double
> check it?

I locally ran the pytest with v2023.10-rc1, and
- test_capsule_firmware_raw passed
- test_capsule_firmware_signed_raw failed

but it seems to me that 'signed_raw' failure is not directly
related to efi implementation (I didn't dig into details, though).

Can you describe more about your environment?

-Takahiro Akashi

> Thanks,
> Michal
> 
>