[PATCH] efi_loader: Bump the number of shared pages with StandAloneMM

[PATCH] efi_loader: Bump the number of shared pages with StandAloneMM

[Ilias Apalodimas]

Currently we allow (and explicitly check) a single shared page with
StandAloneMM.  This is dictated by OP-TEE which runs the application.
However there's no way for us dynamically discover the number of pages we
are allowed to use.  Since writing big EFI signature list variables
requires more than a page, OP-TEE has bumped the number of shared pages to
four.  Bump our page checks to four as well.

Note here that checking some kind of version and reason with the
compatibility doesn't make too much sense.  We sanitize the number of pages
internally in our U-Boot code but eventually OP-TEE will fail if we try to
write more than it's allowing. The error will just happen later on when we
access StandAloneMM.  So in order to avoid compatibility checks change the
number to four unconditionally.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
---
 lib/efi_loader/efi_variable_tee.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index 281f886124af..95eaeaa5fd9d 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -261,8 +261,8 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
 	 * with StMM. Since OP-TEE will reject to map anything bigger than that,
 	 * make sure we are in bounds.
 	 */
-	if (*size > OPTEE_PAGE_SIZE)
-		*size = OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
+	if (*size > 4 * OPTEE_PAGE_SIZE)
+		*size = 4 * OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
 			MM_VARIABLE_COMMUNICATE_SIZE;
 	/*
 	 * There seems to be a bug in EDK2 miscalculating the boundaries and
-- 
2.30.2

Re: [PATCH] efi_loader: Bump the number of shared pages with StandAloneMM

[Heinrich Schuchardt]

On 12/15/21 08:50, Ilias Apalodimas wrote:
> Currently we allow (and explicitly check) a single shared page with
> StandAloneMM.  This is dictated by OP-TEE which runs the application.
> However there's no way for us dynamically discover the number of pages we
> are allowed to use.  Since writing big EFI signature list variables
> requires more than a page, OP-TEE has bumped the number of shared pages to
> four.  Bump our page checks to four as well.
>
> Note here that checking some kind of version and reason with the
> compatibility doesn't make too much sense.  We sanitize the number of pages
> internally in our U-Boot code but eventually OP-TEE will fail if we try to
> write more than it's allowing. The error will just happen later on when we
> access StandAloneMM.  So in order to avoid compatibility checks change the
> number to four unconditionally.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> ---
>   lib/efi_loader/efi_variable_tee.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index 281f886124af..95eaeaa5fd9d 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -261,8 +261,8 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
>   	 * with StMM. Since OP-TEE will reject to map anything bigger than that,
>   	 * make sure we are in bounds.
>   	 */
> -	if (*size > OPTEE_PAGE_SIZE)
> -		*size = OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
> +	if (*size > 4 * OPTEE_PAGE_SIZE)
> +		*size = 4 * OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
>   			MM_VARIABLE_COMMUNICATE_SIZE;

Why do we need this check at all if OPTEE checks again?

Best regards

Heinrich

>   	/*
>   	 * There seems to be a bug in EDK2 miscalculating the boundaries and

Re: [PATCH] efi_loader: Bump the number of shared pages with StandAloneMM

[Ilias Apalodimas]

Hi Heinrich,

On Sat, Dec 18, 2021 at 12:03:34PM +0100, Heinrich Schuchardt wrote:
> 
> 
> On 12/15/21 08:50, Ilias Apalodimas wrote:
> > Currently we allow (and explicitly check) a single shared page with
> > StandAloneMM.  This is dictated by OP-TEE which runs the application.
> > However there's no way for us dynamically discover the number of pages we
> > are allowed to use.  Since writing big EFI signature list variables
> > requires more than a page, OP-TEE has bumped the number of shared pages to
> > four.  Bump our page checks to four as well.
> > 
> > Note here that checking some kind of version and reason with the
> > compatibility doesn't make too much sense.  We sanitize the number of pages
> > internally in our U-Boot code but eventually OP-TEE will fail if we try to
> > write more than it's allowing. The error will just happen later on when we
> > access StandAloneMM.  So in order to avoid compatibility checks change the
> > number to four unconditionally.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > ---
> >   lib/efi_loader/efi_variable_tee.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index 281f886124af..95eaeaa5fd9d 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -261,8 +261,8 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> >   	 * with StMM. Since OP-TEE will reject to map anything bigger than that,
> >   	 * make sure we are in bounds.
> >   	 */
> > -	if (*size > OPTEE_PAGE_SIZE)
> > -		*size = OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
> > +	if (*size > 4 * OPTEE_PAGE_SIZE)
> > +		*size = 4 * OPTEE_PAGE_SIZE - MM_COMMUNICATE_HEADER_SIZE  -
> >   			MM_VARIABLE_COMMUNICATE_SIZE;
> 
> Why do we need this check at all if OPTEE checks again?
> 

OP-TEE will have to try and register the memory in tee_shm_register() to
fail. So since we know if only allows 4 pages we have an internal sanity checking
to bail out earlier.


Regards
/Ilias
> Best regards
> 
> Heinrich
> 
> >   	/*
> >   	 * There seems to be a bug in EDK2 miscalculating the boundaries and