diff mbox series

[v2] efi_loader: Don't limit the StMM buffer size explicitly

Message ID 20211224080841.98906-1-ilias.apalodimas@linaro.org
State New
Headers show
Series [v2] efi_loader: Don't limit the StMM buffer size explicitly | expand

Commit Message

Ilias Apalodimas Dec. 24, 2021, 8:08 a.m. UTC 
From: Ilias Apalodimas <apalos@gmail.com>

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 variable
requires more than a page, OP-TEE has bumped the number of shared pages to
four.

Let's remove our explicit check and allow the request to reach OP-TEE even
if it's bigger than what it supports.  There's no need to sanitize the
number of pages internally.  OP-TEE will fail if we try to write more
than it's allowed. The error will just trigger later on,  during the
StMM access.

While at it add an error message to help users figure out what failed.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>

Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
---
Changes since v1: (was "Bump the number of shared pages with StandAloneMM")
- Remove the check entirely and rely on tee trigeering the error

 include/tee.h                     |  1 +
 lib/efi_loader/efi_variable_tee.c | 21 ++++++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Heinrich Schuchardt Dec. 25, 2021, 10:03 a.m. UTC | #1 
On 12/24/21 09:08, Ilias Apalodimas wrote:
> From: Ilias Apalodimas <apalos@gmail.com>
>
> 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 variable
> requires more than a page, OP-TEE has bumped the number of shared pages to
> four.
>
> Let's remove our explicit check and allow the request to reach OP-TEE even
> if it's bigger than what it supports.  There's no need to sanitize the
> number of pages internally.  OP-TEE will fail if we try to write more
> than it's allowed. The error will just trigger later on,  during the
> StMM access.
>
> While at it add an error message to help users figure out what failed.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
>
> Signed-off-by: Ilias Apalodimas <apalos@gmail.com>
> ---
> Changes since v1: (was "Bump the number of shared pages with StandAloneMM")
> - Remove the check entirely and rely on tee trigeering the error
>
>   include/tee.h                     |  1 +
>   lib/efi_loader/efi_variable_tee.c | 21 ++++++++++-----------
>   2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/tee.h b/include/tee.h
> index 44e9cd4321bc..087810bd12e4 100644
> --- a/include/tee.h
> +++ b/include/tee.h
> @@ -39,6 +39,7 @@
>   #define TEE_SUCCESS			0x00000000
>   #define TEE_ERROR_STORAGE_NOT_AVAILABLE	0xf0100003
>   #define TEE_ERROR_GENERIC		0xffff0000
> +#define TEE_ERROR_EXCESS_DATA		0xffff0004
>   #define TEE_ERROR_BAD_PARAMETERS	0xffff0006
>   #define TEE_ERROR_ITEM_NOT_FOUND	0xffff0008
>   #define TEE_ERROR_NOT_IMPLEMENTED	0xffff0009
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index 281f886124af..b2d1513bea5d 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -15,7 +15,6 @@
>   #include <malloc.h>
>   #include <mm_communication.h>
>
> -#define OPTEE_PAGE_SIZE BIT(12)
>   extern struct efi_var_file __efi_runtime_data *efi_var_buf;
>   static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
>   static efi_uintn_t max_payload_size;	/* func + data */
> @@ -113,9 +112,18 @@ static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
>
>   	rc = tee_invoke_func(conn.tee, &arg, 2, param);
>   	tee_shm_free(shm);
> +	/*
> +	 * Although the max payload is configurable on StMM, we only share
> +	 * four pages from OP-TEE for the non-secure buffer used to communicate
> +	 * with StMM. OP-TEE will reject anything bigger than that and will
> +	 * return.  So le'ts at least warn users
> +	 */
>   	tee_close_session(conn.tee, conn.session);
> -	if (rc || arg.ret != TEE_SUCCESS)
> +	if (rc || arg.ret != TEE_SUCCESS) {

tee_close_session(): Will arg.ret be valid if rc != 0?

Best regards

Heinrich

> +		if (arg.ret == TEE_ERROR_EXCESS_DATA)
> +			log_err("Variable payload too large\n");
>   		return EFI_DEVICE_ERROR;
> +	}
>
>   	switch (param[1].u.value.a) {
>   	case ARM_SVC_SPM_RET_SUCCESS:
> @@ -255,15 +263,6 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
>   		goto out;
>   	}
>   	*size = var_payload->size;
> -	/*
> -	 * Although the max payload is configurable on StMM, we only share a
> -	 * single page from OP-TEE for the non-secure buffer used to communicate
> -	 * 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  -
> -			MM_VARIABLE_COMMUNICATE_SIZE;
>   	/*
>   	 * There seems to be a bug in EDK2 miscalculating the boundaries and
>   	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
Ilias Apalodimas Dec. 25, 2021, 11:16 a.m. UTC | #2 
> > 
[...]
> >   	rc = tee_invoke_func(conn.tee, &arg, 2, param);
> >   	tee_shm_free(shm);
> > +	/*
> > +	 * Although the max payload is configurable on StMM, we only share
> > +	 * four pages from OP-TEE for the non-secure buffer used to communicate
> > +	 * with StMM. OP-TEE will reject anything bigger than that and will
> > +	 * return.  So le'ts at least warn users
> > +	 */
> >   	tee_close_session(conn.tee, conn.session);
> > -	if (rc || arg.ret != TEE_SUCCESS)
> > +	if (rc || arg.ret != TEE_SUCCESS) {
> 
> tee_close_session(): Will arg.ret be valid if rc != 0?

Depends when tee_invoke_func() failed.  But why do we care?
The connection needs to close regardless and we then have to reason with
the error.

Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > +		if (arg.ret == TEE_ERROR_EXCESS_DATA)
> > +			log_err("Variable payload too large\n");
> >   		return EFI_DEVICE_ERROR;
> > +	}
> > 
> >   	switch (param[1].u.value.a) {
> >   	case ARM_SVC_SPM_RET_SUCCESS:
> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> >   		goto out;
> >   	}
> >   	*size = var_payload->size;
> > -	/*
> > -	 * Although the max payload is configurable on StMM, we only share a
> > -	 * single page from OP-TEE for the non-secure buffer used to communicate
> > -	 * 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  -
> > -			MM_VARIABLE_COMMUNICATE_SIZE;
> >   	/*
> >   	 * There seems to be a bug in EDK2 miscalculating the boundaries and
> >   	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
>
Heinrich Schuchardt Dec. 25, 2021, 2:28 p.m. UTC | #3 
Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>> > 
>[...]
>> >   	rc = tee_invoke_func(conn.tee, &arg, 2, param);
>> >   	tee_shm_free(shm);
>> > +	/*
>> > +	 * Although the max payload is configurable on StMM, we only share
>> > +	 * four pages from OP-TEE for the non-secure buffer used to communicate
>> > +	 * with StMM. OP-TEE will reject anything bigger than that and will
>> > +	 * return.  So le'ts at least warn users
>> > +	 */
>> >   	tee_close_session(conn.tee, conn.session);
>> > -	if (rc || arg.ret != TEE_SUCCESS)
>> > +	if (rc || arg.ret != TEE_SUCCESS) {
>> 
>> tee_close_session(): Will arg.ret be valid if rc != 0?
>
>Depends when tee_invoke_func() failed.  But why do we care?

Should we write a message if rc !=0 && arg.ret == TEE_ERROR_EXCESS_DATA?

>The connection needs to close regardless and we then have to reason with
>the error.
>
>Regards
>/Ilias
>> 
>> Best regards
>> 
>> Heinrich
>> 
>> > +		if (arg.ret == TEE_ERROR_EXCESS_DATA)
>> > +			log_err("Variable payload too large\n");
>> >   		return EFI_DEVICE_ERROR;
>> > +	}
>> > 
>> >   	switch (param[1].u.value.a) {
>> >   	case ARM_SVC_SPM_RET_SUCCESS:
>> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
>> >   		goto out;
>> >   	}
>> >   	*size = var_payload->size;
>> > -	/*
>> > -	 * Although the max payload is configurable on StMM, we only share a
>> > -	 * single page from OP-TEE for the non-secure buffer used to communicate
>> > -	 * 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  -
>> > -			MM_VARIABLE_COMMUNICATE_SIZE;
>> >   	/*
>> >   	 * There seems to be a bug in EDK2 miscalculating the boundaries and
>> >   	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
>>
Ilias Apalodimas Dec. 25, 2021, 3:04 p.m. UTC | #4 
On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de> wrote:

>
>
> Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas <
> ilias.apalodimas@linaro.org>:
> >> >
> >[...]
> >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
> >> >    tee_shm_free(shm);
> >> > +  /*
> >> > +   * Although the max payload is configurable on StMM, we only share
> >> > +   * four pages from OP-TEE for the non-secure buffer used to
> communicate
> >> > +   * with StMM. OP-TEE will reject anything bigger than that and will
> >> > +   * return.  So le'ts at least warn users
> >> > +   */
> >> >    tee_close_session(conn.tee, conn.session);
> >> > -  if (rc || arg.ret != TEE_SUCCESS)
> >> > +  if (rc || arg.ret != TEE_SUCCESS) {
> >>
> >> tee_close_session(): Will arg.ret be valid if rc != 0?
> >
> >Depends when tee_invoke_func() failed.  But why do we care?
>
> Should we write a message if rc !=0 && arg.ret == TEE_ERROR_EXCESS_DATA?
>

I don't think its needed. OPTEE will set that only if RC == 0

Cheers
Ilias

>
> >The connection needs to close regardless and we then have to reason with
> >the error.
> >
> >Regards
> >/Ilias
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >> > +          if (arg.ret == TEE_ERROR_EXCESS_DATA)
> >> > +                  log_err("Variable payload too large\n");
> >> >            return EFI_DEVICE_ERROR;
> >> > +  }
> >> >
> >> >    switch (param[1].u.value.a) {
> >> >    case ARM_SVC_SPM_RET_SUCCESS:
> >> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI get_max_payload(efi_uintn_t
> *size)
> >> >            goto out;
> >> >    }
> >> >    *size = var_payload->size;
> >> > -  /*
> >> > -   * Although the max payload is configurable on StMM, we only share
> a
> >> > -   * single page from OP-TEE for the non-secure buffer used to
> communicate
> >> > -   * 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  -
> >> > -                  MM_VARIABLE_COMMUNICATE_SIZE;
> >> >    /*
> >> >     * There seems to be a bug in EDK2 miscalculating the boundaries
> and
> >> >     * size checks, so deduct 2 more bytes to fulfill this
> requirement. Fix
> >>
>
Heinrich Schuchardt Dec. 25, 2021, 4:13 p.m. UTC | #5 
On 12/25/21 16:04, Ilias Apalodimas wrote:
>
>
> On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>
>
>     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
>     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
>      >> >
>      >[...]
>      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
>      >> >    tee_shm_free(shm);
>      >> > +  /*
>      >> > +   * Although the max payload is configurable on StMM, we
>     only share
>      >> > +   * four pages from OP-TEE for the non-secure buffer used to
>     communicate
>      >> > +   * with StMM. OP-TEE will reject anything bigger than that
>     and will
>      >> > +   * return.  So le'ts at least warn users
>      >> > +   */

The comment mentioning four pages does not make too much sense to me as
both OP-TEE as well as U-Boot can be configured to other sizes.

>      >> >    tee_close_session(conn.tee, conn.session);
>      >> > -  if (rc || arg.ret != TEE_SUCCESS)
>      >> > +  if (rc || arg.ret != TEE_SUCCESS) {
>      >>
>      >> tee_close_session(): Will arg.ret be valid if rc != 0?
>      >
>      >Depends when tee_invoke_func() failed.  But why do we care?
>
>     Should we write a message if rc !=0 && arg.ret == TEE_ERROR_EXCESS_DATA?
>
>
> I don't think its needed. OPTEE will set that only if RC == 0
>
> Cheers
> Ilias
>
>
>      >The connection needs to close regardless and we then have to
>     reason with
>      >the error.
>      >
>      >Regards
>      >/Ilias

So how about:

@@ -114,7 +113,11 @@ static efi_status_t optee_mm_communicate(void
*comm_buf, ulong dsize)
         rc = tee_invoke_func(conn.tee, &arg, 2, param);
         tee_shm_free(shm);
         tee_close_session(conn.tee, conn.session);
-       if (rc || arg.ret != TEE_SUCCESS)
+       if (rc)
+               return EFI_DEVICE_ERROR;
+       if (arg.ret == TEE_ERROR_EXCESS_DATA)
+               log_err("Variable payload too large\n");
+       if (arg.ret != TEE_SUCCESS)
                 return EFI_DEVICE_ERROR;

Best regards

Heinrich

>      >>
>      >> > +          if (arg.ret == TEE_ERROR_EXCESS_DATA)
>      >> > +                  log_err("Variable payload too large\n");
>      >> >            return EFI_DEVICE_ERROR;
>      >> > +  }
>      >> >
>      >> >    switch (param[1].u.value.a) {
>      >> >    case ARM_SVC_SPM_RET_SUCCESS:
>      >> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI
>     get_max_payload(efi_uintn_t *size)
>      >> >            goto out;
>      >> >    }
>      >> >    *size = var_payload->size;
>      >> > -  /*
>      >> > -   * Although the max payload is configurable on StMM, we
>     only share a
>      >> > -   * single page from OP-TEE for the non-secure buffer used
>     to communicate
>      >> > -   * 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  -
>      >> > -                  MM_VARIABLE_COMMUNICATE_SIZE;
>      >> >    /*
>      >> >     * There seems to be a bug in EDK2 miscalculating the
>     boundaries and
>      >> >     * size checks, so deduct 2 more bytes to fulfill this
>     requirement. Fix
>      >>
>
Ilias Apalodimas Dec. 25, 2021, 7:35 p.m. UTC | #6 
On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote:
> On 12/25/21 16:04, Ilias Apalodimas wrote:
> > 
> > 
> > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> > 
> > 
> > 
> >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
> >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
> >      >> >
> >      >[...]
> >      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
> >      >> >    tee_shm_free(shm);
> >      >> > +  /*
> >      >> > +   * Although the max payload is configurable on StMM, we
> >     only share
> >      >> > +   * four pages from OP-TEE for the non-secure buffer used to
> >     communicate
> >      >> > +   * with StMM. OP-TEE will reject anything bigger than that
> >     and will
> >      >> > +   * return.  So le'ts at least warn users
> >      >> > +   */
> 
> The comment mentioning four pages does not make too much sense to me as
> both OP-TEE as well as U-Boot can be configured to other sizes.
> 
> >      >> >    tee_close_session(conn.tee, conn.session);
> >      >> > -  if (rc || arg.ret != TEE_SUCCESS)
> >      >> > +  if (rc || arg.ret != TEE_SUCCESS) {
> >      >>
> >      >> tee_close_session(): Will arg.ret be valid if rc != 0?
> >      >
> >      >Depends when tee_invoke_func() failed.  But why do we care?
> > 
> >     Should we write a message if rc !=0 && arg.ret == TEE_ERROR_EXCESS_DATA?
> > 
> > 
> > I don't think its needed. OPTEE will set that only if RC == 0
> > 
> > Cheers
> > Ilias
> > 
> > 
> >      >The connection needs to close regardless and we then have to
> >     reason with
> >      >the error.
> >      >
> >      >Regards
> >      >/Ilias
> 
> So how about:
> 
> @@ -114,7 +113,11 @@ static efi_status_t optee_mm_communicate(void
> *comm_buf, ulong dsize)
>         rc = tee_invoke_func(conn.tee, &arg, 2, param);
>         tee_shm_free(shm);
>         tee_close_session(conn.tee, conn.session);
> -       if (rc || arg.ret != TEE_SUCCESS)
> +       if (rc)
> +               return EFI_DEVICE_ERROR;
> +       if (arg.ret == TEE_ERROR_EXCESS_DATA)
> +               log_err("Variable payload too large\n");
> +       if (arg.ret != TEE_SUCCESS)
>                 return EFI_DEVICE_ERROR;

We just move the error reporting out of the inner if.
i.e 
if (arg.ret == TEE_ERROR_EXCESS_DATA)
	log_err("Variable payload too large\n");

if (rc || arg.ret != TEE_SUCCESS)
	return EFI_DEVICE_ERROR;

Which looks better to me

Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >      >>
> >      >> > +          if (arg.ret == TEE_ERROR_EXCESS_DATA)
> >      >> > +                  log_err("Variable payload too large\n");
> >      >> >            return EFI_DEVICE_ERROR;
> >      >> > +  }
> >      >> >
> >      >> >    switch (param[1].u.value.a) {
> >      >> >    case ARM_SVC_SPM_RET_SUCCESS:
> >      >> > @@ -255,15 +263,6 @@ efi_status_t EFIAPI
> >     get_max_payload(efi_uintn_t *size)
> >      >> >            goto out;
> >      >> >    }
> >      >> >    *size = var_payload->size;
> >      >> > -  /*
> >      >> > -   * Although the max payload is configurable on StMM, we
> >     only share a
> >      >> > -   * single page from OP-TEE for the non-secure buffer used
> >     to communicate
> >      >> > -   * 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  -
> >      >> > -                  MM_VARIABLE_COMMUNICATE_SIZE;
> >      >> >    /*
> >      >> >     * There seems to be a bug in EDK2 miscalculating the
> >     boundaries and
> >      >> >     * size checks, so deduct 2 more bytes to fulfill this
> >     requirement. Fix
> >      >>
> > 
>
Ilias Apalodimas Dec. 25, 2021, 7:39 p.m. UTC | #7 
On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote:
> On 12/25/21 16:04, Ilias Apalodimas wrote:
> > 
> > 
> > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> > 
> > 
> > 
> >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
> >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
> >      >> >
> >      >[...]
> >      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
> >      >> >    tee_shm_free(shm);
> >      >> > +  /*
> >      >> > +   * Although the max payload is configurable on StMM, we
> >     only share
> >      >> > +   * four pages from OP-TEE for the non-secure buffer used to
> >     communicate
> >      >> > +   * with StMM. OP-TEE will reject anything bigger than that
> >     and will
> >      >> > +   * return.  So le'ts at least warn users
> >      >> > +   */
> 
> The comment mentioning four pages does not make too much sense to me as
> both OP-TEE as well as U-Boot can be configured to other sizes.
> 

The pages that op-tee uses are not configurable.  What is configurable is
the number of pages you can request op-tee to map from the non-secure
world for u-boot sharing.  However the four page restriction I refer to 
is an internal op-tee one and refers to the non-secure world buffer it 
shares with StandAloneMM, not u-boot [1]

[...]

[1] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/stmm_sp.c#L73
Cheers
/Ilias
Jens Wiklander Dec. 27, 2021, 8:26 a.m. UTC | #8 
Hi Ilias,

On Sat, Dec 25, 2021 at 8:39 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote:
> > On 12/25/21 16:04, Ilias Apalodimas wrote:
> > >
> > >
> > > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
> > > <mailto:xypron.glpk@gmx.de>> wrote:
> > >
> > >
> > >
> > >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
> > >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
> > >      >> >
> > >      >[...]
> > >      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
> > >      >> >    tee_shm_free(shm);
> > >      >> > +  /*
> > >      >> > +   * Although the max payload is configurable on StMM, we
> > >     only share
> > >      >> > +   * four pages from OP-TEE for the non-secure buffer used to
> > >     communicate
> > >      >> > +   * with StMM. OP-TEE will reject anything bigger than that
> > >     and will
> > >      >> > +   * return.  So le'ts at least warn users
> > >      >> > +   */
> >
> > The comment mentioning four pages does not make too much sense to me as
> > both OP-TEE as well as U-Boot can be configured to other sizes.
> >
>
> The pages that op-tee uses are not configurable.  What is configurable is
> the number of pages you can request op-tee to map from the non-secure
> world for u-boot sharing.  However the four page restriction I refer to
> is an internal op-tee one and refers to the non-secure world buffer it
> shares with StandAloneMM, not u-boot [1]

The commit message suggests that we may try to use an even larger
buffer if needed. A comment here in the code mentioning how this works
should be useful.

Cheers,
Jens

>
> [...]
>
> [1] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/stmm_sp.c#L73
> Cheers
> /Ilias
Ilias Apalodimas Dec. 27, 2021, 8:29 a.m. UTC | #9 
Hi Jens,

On Mon, 27 Dec 2021 at 10:26, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Ilias,
>
> On Sat, Dec 25, 2021 at 8:39 PM Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote:
> > > On 12/25/21 16:04, Ilias Apalodimas wrote:
> > > >
> > > >
> > > > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
> > > > <mailto:xypron.glpk@gmx.de>> wrote:
> > > >
> > > >
> > > >
> > > >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
> > > >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
> > > >      >> >
> > > >      >[...]
> > > >      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
> > > >      >> >    tee_shm_free(shm);
> > > >      >> > +  /*
> > > >      >> > +   * Although the max payload is configurable on StMM, we
> > > >     only share
> > > >      >> > +   * four pages from OP-TEE for the non-secure buffer used to
> > > >     communicate
> > > >      >> > +   * with StMM. OP-TEE will reject anything bigger than that
> > > >     and will
> > > >      >> > +   * return.  So le'ts at least warn users
> > > >      >> > +   */
> > >
> > > The comment mentioning four pages does not make too much sense to me as
> > > both OP-TEE as well as U-Boot can be configured to other sizes.
> > >
> >
> > The pages that op-tee uses are not configurable.  What is configurable is
> > the number of pages you can request op-tee to map from the non-secure
> > world for u-boot sharing.  However the four page restriction I refer to
> > is an internal op-tee one and refers to the non-secure world buffer it
> > shares with StandAloneMM, not u-boot [1]
>
> The commit message suggests that we may try to use an even larger
> buffer if needed. A comment here in the code mentioning how this works
> should be useful.
>

Fair enough,
I'll send a v3 adding that

Thanks
/Ilias
> Cheers,
> Jens
>
> >
> > [...]
> >
> > [1] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/stmm_sp.c#L73
> > Cheers
> > /Ilias
Heinrich Schuchardt Dec. 27, 2021, 8:35 a.m. UTC | #10 
Am 27. Dezember 2021 09:29:32 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>Hi Jens,
>
>On Mon, 27 Dec 2021 at 10:26, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>>
>> Hi Ilias,
>>
>> On Sat, Dec 25, 2021 at 8:39 PM Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>> >
>> > On Sat, Dec 25, 2021 at 05:13:23PM +0100, Heinrich Schuchardt wrote:
>> > > On 12/25/21 16:04, Ilias Apalodimas wrote:
>> > > >
>> > > >
>> > > > On Sat, 25 Dec 2021, 16:28 Heinrich Schuchardt, <xypron.glpk@gmx.de
>> > > > <mailto:xypron.glpk@gmx.de>> wrote:
>> > > >
>> > > >
>> > > >
>> > > >     Am 25. Dezember 2021 12:16:29 MEZ schrieb Ilias Apalodimas
>> > > >     <ilias.apalodimas@linaro.org <mailto:ilias.apalodimas@linaro.org>>:
>> > > >      >> >
>> > > >      >[...]
>> > > >      >> >    rc = tee_invoke_func(conn.tee, &arg, 2, param);
>> > > >      >> >    tee_shm_free(shm);
>> > > >      >> > +  /*
>> > > >      >> > +   * Although the max payload is configurable on StMM, we
>> > > >     only share
>> > > >      >> > +   * four pages from OP-TEE for the non-secure buffer used to
>> > > >     communicate
>> > > >      >> > +   * with StMM. OP-TEE will reject anything bigger than that
>> > > >     and will
>> > > >      >> > +   * return.  So le'ts at least warn users
>> > > >      >> > +   */
>> > >
>> > > The comment mentioning four pages does not make too much sense to me as
>> > > both OP-TEE as well as U-Boot can be configured to other sizes.
>> > >
>> >
>> > The pages that op-tee uses are not configurable.  What is configurable is
>> > the number of pages you can request op-tee to map from the non-secure
>> > world for u-boot sharing.  However the four page restriction I refer to
>> > is an internal op-tee one and refers to the non-secure world buffer it
>> > shares with StandAloneMM, not u-boot [1]
>>
>> The commit message suggests that we may try to use an even larger
>> buffer if needed. A comment here in the code mentioning how this works
>> should be useful.
>>
>
>Fair enough,
>I'll send a v3 adding that

I think the code is the wrong place. Put this into the HTML doc and maybe cross reference it in any relevant Kconfig symbol.

Best regards

Heinrich

>
>Thanks
>/Ilias
>> Cheers,
>> Jens
>>
>> >
>> > [...]
>> >
>> > [1] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/stmm_sp.c#L73
>> > Cheers
>> > /Ilias
diff mbox series

Patch

diff --git a/include/tee.h b/include/tee.h
index 44e9cd4321bc..087810bd12e4 100644
--- a/include/tee.h
+++ b/include/tee.h
@@ -39,6 +39,7 @@ 
 #define TEE_SUCCESS			0x00000000
 #define TEE_ERROR_STORAGE_NOT_AVAILABLE	0xf0100003
 #define TEE_ERROR_GENERIC		0xffff0000
+#define TEE_ERROR_EXCESS_DATA		0xffff0004
 #define TEE_ERROR_BAD_PARAMETERS	0xffff0006
 #define TEE_ERROR_ITEM_NOT_FOUND	0xffff0008
 #define TEE_ERROR_NOT_IMPLEMENTED	0xffff0009
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index 281f886124af..b2d1513bea5d 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -15,7 +15,6 @@ 
 #include <malloc.h>
 #include <mm_communication.h>
 
-#define OPTEE_PAGE_SIZE BIT(12)
 extern struct efi_var_file __efi_runtime_data *efi_var_buf;
 static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
 static efi_uintn_t max_payload_size;	/* func + data */
@@ -113,9 +112,18 @@  static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
 
 	rc = tee_invoke_func(conn.tee, &arg, 2, param);
 	tee_shm_free(shm);
+	/*
+	 * Although the max payload is configurable on StMM, we only share
+	 * four pages from OP-TEE for the non-secure buffer used to communicate
+	 * with StMM. OP-TEE will reject anything bigger than that and will
+	 * return.  So le'ts at least warn users
+	 */
 	tee_close_session(conn.tee, conn.session);
-	if (rc || arg.ret != TEE_SUCCESS)
+	if (rc || arg.ret != TEE_SUCCESS) {
+		if (arg.ret == TEE_ERROR_EXCESS_DATA)
+			log_err("Variable payload too large\n");
 		return EFI_DEVICE_ERROR;
+	}
 
 	switch (param[1].u.value.a) {
 	case ARM_SVC_SPM_RET_SUCCESS:
@@ -255,15 +263,6 @@  efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
 		goto out;
 	}
 	*size = var_payload->size;
-	/*
-	 * Although the max payload is configurable on StMM, we only share a
-	 * single page from OP-TEE for the non-secure buffer used to communicate
-	 * 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  -
-			MM_VARIABLE_COMMUNICATE_SIZE;
 	/*
 	 * There seems to be a bug in EDK2 miscalculating the boundaries and
 	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix