[PATCH] efi_loader: check update capsule parameters

[Heinrich Schuchardt]

On 6/12/21 2:13 AM, AKASHI Takahiro wrote:
> Hi Vincent,
>
> Thank you for the update.
>
> On Fri, Jun 11, 2021 at 07:06:01PM +0200, Heinrich Schuchardt wrote:
>> Cc: Takahiro, Sughosh, Ilias
>>
>> On 6/11/21 6:15 PM, Vincent Stehlé wrote:
>>> UpdateCapsule() must return EFI_INVALID_PARAMETER in a number of cases,
>>> listed by the UEFI specification and tested by the SCT. Add a common
>>> function to do that.
>
> First of all, I would like to make clear one thing:
> My initial implementation of capsule support does *not*
> intend to support "UpdateCapsule" as runtime API.
> Instead, the sole objective is to provide "delivery of
> capsules via file" (or capsule on disk) method.
>
> This is because the initially-expected use case was
> that we would adopt capsules as an alternative for
> updating UEFI variables without using SetVariable runtime API.
> (This idea is no longer upheld, though.)
>
> This is why the API does not handle nor check parameters
> in the current code. I even regret to have added
> EFI_RUNTIME_UPDATE_CAPSULE option, which makes people confused.
>
> Nevertheless, I'm more than happy if other folks implement
> full semantics of UpdateCapsule.
>
>>> This fixes SCT UpdateCapsule_Conf failures.
>>>
>>> Reviewed-by: Grant Likely <grant.likely at arm.com>
>>> Signed-off-by: Vincent Stehlé <vincent.stehle at arm.com>
>>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> Cc: Alexander Graf <agraf at csgraf.de>
>>> ---
>>>    include/efi_loader.h         | 24 ++++++++++++++++++++++++
>>>    lib/efi_loader/efi_capsule.c |  8 ++++----
>>>    lib/efi_loader/efi_runtime.c |  8 ++++++++
>>>    3 files changed, 36 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 0a9c82a257e..426d1c72d7d 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -910,6 +910,30 @@ extern const struct efi_firmware_management_protocol efi_fmp_fit;
>>>    extern const struct efi_firmware_management_protocol efi_fmp_raw;
>>>
>>>    /* Capsule update */
>>> +static inline efi_status_t
>>> +efi_valid_update_capsule_params(struct efi_capsule_header
>>> +						**capsule_header_array,
>>> +				efi_uintn_t capsule_count,
>>> +				u64 scatter_gather_list)
>>> +{
>>> +	u32 flags;
>>> +
>>> +	if (!capsule_count)
>>> +		return EFI_INVALID_PARAMETER;
>>
>> If capsule count > 1, don't you have to check all capsules headers?
>>
>>> +
>>> +	flags = capsule_header_array[0]->flags;
>>> +
>>> +	if (((flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET) &&
>>> +	     !scatter_gather_list) ||
>>> +	    ((flags & CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE) &&
>>> +	     !(flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) ||
>>> +	    ((flags & CAPSULE_FLAGS_INITIATE_RESET) &&
>>> +	     !(flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)))
>>> +		return EFI_INVALID_PARAMETER;
>>
>> What happens if capsule(0) has CAPSULE_FLAGS_INITIATE_RESET and
>> capsule(4) has !CAPSULE_FLAGS_PERSIST_ACROSS_RESET?
>>
>> Best regards
>>
>> Heinrich
>>
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>>    efi_status_t EFIAPI efi_update_capsule(
>>>    		struct efi_capsule_header **capsule_header_array,
>>>    		efi_uintn_t capsule_count,
>>> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
>>> index 60309d4a07d..380cfd70290 100644
>>> --- a/lib/efi_loader/efi_capsule.c
>>> +++ b/lib/efi_loader/efi_capsule.c
>>> @@ -442,12 +442,12 @@ efi_status_t EFIAPI efi_update_capsule(
>>>    	EFI_ENTRY("%p, %zu, %llu\n", capsule_header_array, capsule_count,
>>>    		  scatter_gather_list);
>>>
>>> -	if (!capsule_count) {
>>> -		ret = EFI_INVALID_PARAMETER;
>>> +	ret = efi_valid_update_capsule_params(capsule_header_array,
>>> +					      capsule_count,
>>> +					      scatter_gather_list);
>>> +	if (ret != EFI_SUCCESS)
>>>    		goto out;
>>> -	}
>>>
>>> -	ret = EFI_SUCCESS;
>>>    	for (i = 0, capsule = *capsule_header_array; i < capsule_count;
>>>    	     i++, capsule = *(++capsule_header_array)) {
>>>    		/* sanity check */
>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>> index 93a695fc27e..449ad8b9f36 100644
>>> --- a/lib/efi_loader/efi_runtime.c
>>> +++ b/lib/efi_loader/efi_runtime.c
>>> @@ -467,6 +467,14 @@ efi_status_t __efi_runtime EFIAPI efi_update_capsule_unsupported(
>>>    			efi_uintn_t capsule_count,
>>>    			u64 scatter_gather_list)
>>>    {
>>> +	efi_status_t ret;
>>> +
>>> +	ret = efi_valid_update_capsule_params(capsule_header_array,
>>> +					      capsule_count,
>>> +					      scatter_gather_list);
>
> This is "efi_update_capsule_unsupported" function.
> We don't have to check parameters.
>
> -Takahiro Akashi

Hello Vincent,

I will mark this patch as "changes requested". Please, send an updated
version.

Best regards

Heinrich

>
>>> +	if (ret != EFI_SUCCESS)
>>> +		return ret;
>>> +
>>>    	return EFI_UNSUPPORTED;
>>>    }
>>>
>>>
>>