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

[Heinrich Schuchardt]

On 12/24/21 09:08, Ilias Apalodimas wrote:
> From: Ilias Apalodimas <apalos at 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 at linaro.org>
> Tested-by: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
>
> Signed-off-by: Ilias Apalodimas <apalos at 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