efi_loader: Check for the native OP-TEE result on mm_communicate calls

Message ID 20200722073222.510135-1-ilias.apalodimas@linaro.org
State Accepted
Commit 9b87d4429c145ebb66895c7e053e8d53192180e2
Headers show
Series
  • efi_loader: Check for the native OP-TEE result on mm_communicate calls
Related show

Commit Message

Ilias Apalodimas July 22, 2020, 7:32 a.m.
Currently we only check for the return value of tee_invoke_func().
Although OP-TEE and StMM will correctly set param[1].u.value.a and we'll
eventually return an error, the correct thing to do is check for the
OP_TEE return code as well.
So let's check for that and move tee_shm_free() and tee_close_session()
before exiting with an error to make sure we always clear the registered
memory.

Fixes: f042e47e8fb43 ("efi_loader: Implement EFI variable handling via OP-TEE")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

---
 lib/efi_loader/efi_variable_tee.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.28.0.rc1

Comments

Heinrich Schuchardt July 22, 2020, 10:21 a.m. | #1
On 22.07.20 09:32, Ilias Apalodimas wrote:
> Currently we only check for the return value of tee_invoke_func().

> Although OP-TEE and StMM will correctly set param[1].u.value.a and we'll

> eventually return an error, the correct thing to do is check for the

> OP_TEE return code as well.

> So let's check for that and move tee_shm_free() and tee_close_session()

> before exiting with an error to make sure we always clear the registered

> memory.

>

> Fixes: f042e47e8fb43 ("efi_loader: Implement EFI variable handling via OP-TEE")

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 c0423489388a..346f4c786f28 100644

> --- a/lib/efi_loader/efi_variable_tee.c

> +++ b/lib/efi_loader/efi_variable_tee.c

> @@ -100,10 +100,10 @@ static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)

>  	param[1].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;

>

>  	rc = tee_invoke_func(conn.tee, &arg, 2, param);

> -	if (rc)

> -		return EFI_INVALID_PARAMETER;

>  	tee_shm_free(shm);

>  	tee_close_session(conn.tee, conn.session);

> +	if (rc || arg.ret != TEE_SUCCESS)

> +		return EFI_INVALID_PARAMETER;


I will use EFI_DEVICE_ERROR here when merging as this indicates an
internal problem in contrast to a problem caused by the caller of the
UEFI API.

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>

>  	switch (param[1].u.value.a) {

>  	case ARM_SMC_MM_RET_SUCCESS:

>

Patch

diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index c0423489388a..346f4c786f28 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -100,10 +100,10 @@  static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
 	param[1].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;
 
 	rc = tee_invoke_func(conn.tee, &arg, 2, param);
-	if (rc)
-		return EFI_INVALID_PARAMETER;
 	tee_shm_free(shm);
 	tee_close_session(conn.tee, conn.session);
+	if (rc || arg.ret != TEE_SUCCESS)
+		return EFI_INVALID_PARAMETER;
 
 	switch (param[1].u.value.a) {
 	case ARM_SMC_MM_RET_SUCCESS: