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

[Michal Simek <michal.simek@amd.com>]

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>

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?

Thanks,
Michal