u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [PATCH] efi_loader: check update capsule parameters
@ 2021-06-11 16:15 Vincent Stehlé
  2021-06-11 17:06 ` Heinrich Schuchardt
  0 siblings, 1 reply; 4+ messages in thread
From: Vincent Stehlé @ 2021-06-11 16:15 UTC (permalink / raw)
  To: u-boot
  Cc: Vincent Stehlé, Grant Likely, Heinrich Schuchardt, Alexander Graf

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.

This fixes SCT UpdateCapsule_Conf failures.

Reviewed-by: Grant Likely <grant.likely@arm.com>
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Alexander Graf <agraf@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;
+
+	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;
+
+	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);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
 	return EFI_UNSUPPORTED;
 }
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: check update capsule parameters
  2021-06-11 16:15 [PATCH] efi_loader: check update capsule parameters Vincent Stehlé
@ 2021-06-11 17:06 ` Heinrich Schuchardt
  2021-06-12  0:13   ` AKASHI Takahiro
  0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2021-06-11 17:06 UTC (permalink / raw)
  To: Vincent Stehlé
  Cc: Grant Likely, Alexander Graf, AKASHI Takahiro, Sughosh Ganu,
	Ilias Apalodimas, u-boot

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.
>
> This fixes SCT UpdateCapsule_Conf failures.
>
> Reviewed-by: Grant Likely <grant.likely@arm.com>
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Alexander Graf <agraf@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);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
>   	return EFI_UNSUPPORTED;
>   }
>
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: check update capsule parameters
  2021-06-11 17:06 ` Heinrich Schuchardt
@ 2021-06-12  0:13   ` AKASHI Takahiro
  2021-06-21  6:04     ` Heinrich Schuchardt
  0 siblings, 1 reply; 4+ messages in thread
From: AKASHI Takahiro @ 2021-06-12  0:13 UTC (permalink / raw)
  To: Heinrich Schuchardt
  Cc: Vincent Stehl??,
	Grant Likely, Alexander Graf, Sughosh Ganu, Ilias Apalodimas,
	u-boot

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@arm.com>
> > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Alexander Graf <agraf@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

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] efi_loader: check update capsule parameters
  2021-06-12  0:13   ` AKASHI Takahiro
@ 2021-06-21  6:04     ` Heinrich Schuchardt
  0 siblings, 0 replies; 4+ messages in thread
From: Heinrich Schuchardt @ 2021-06-21  6:04 UTC (permalink / raw)
  To: Vincent Stehle
  Cc: AKASHI Takahiro, u-boot, Grant Likely, Alexander Graf,
	Ilias Apalodimas, Sughosh Ganu

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@arm.com>
>>> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> Cc: Alexander Graf <agraf@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;
>>>    }
>>>
>>>
>>


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-21  6:04 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-11 16:15 [PATCH] efi_loader: check update capsule parameters Vincent Stehlé
2021-06-11 17:06 ` Heinrich Schuchardt
2021-06-12  0:13   ` AKASHI Takahiro
2021-06-21  6:04     ` Heinrich Schuchardt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).