[PATCH v4 0/5] TEE: minor cleanup

[PATCH v4 0/5] TEE: minor cleanup

[Igor Opaniuk]

- Address some spelling errors and typos
- Support CMD_OPTEE_RPMB for SANDBOX configurations and add python tests
- Remove common.h inclusion for drivers/tee
- Add calls for closing tee session after every read/write operation.

CI build:
[1] https://dev.azure.com/igoropaniuk/u-boot/_build/results?buildId=31&view=results

Changes in v4:
- Rebased on the latest master and excluded "tee: sandbox: fix spelling errors",
  as it was merged already by Heinrich Schuchardt

Changes in v3:
- Added calls for closing tee session after every read/write operation

Changes in v2:
- Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'
- Applied R-b and T-b tags

Igor Opaniuk (5):
  tee: optee: fix description in Kconfig
  cmd: optee_rpmb: close tee session
  cmd: optee_rpmb: build cmd for sandbox
  test: py: add optee_rpmb tests
  tee: remove common.h inclusion

 cmd/Kconfig                        |  4 +++-
 cmd/optee_rpmb.c                   | 23 +++++++++++++++++------
 drivers/tee/broadcom/chimp_optee.c |  3 ++-
 drivers/tee/optee/Kconfig          |  2 +-
 drivers/tee/optee/core.c           |  1 -
 drivers/tee/optee/i2c.c            |  1 -
 drivers/tee/optee/rpmb.c           |  1 -
 drivers/tee/optee/supplicant.c     |  2 +-
 drivers/tee/sandbox.c              |  2 +-
 drivers/tee/tee-uclass.c           |  1 -
 test/py/tests/test_optee_rpmb.py   | 20 ++++++++++++++++++++
 11 files changed, 45 insertions(+), 15 deletions(-)
 create mode 100644 test/py/tests/test_optee_rpmb.py

-- 
2.34.1

[PATCH v4 1/5] tee: optee: fix description in Kconfig

[Igor Opaniuk]

Fix OPTEE_TA_AVB symbol description in Kconfig:
s/"write"rb"/"write_rb"/g

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

(no changes since v2)

Changes in v2:
- Applied R-b tags

 drivers/tee/optee/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
index 9dc65b0501e..db0bcfa6f15 100644
--- a/drivers/tee/optee/Kconfig
+++ b/drivers/tee/optee/Kconfig
@@ -19,7 +19,7 @@ config OPTEE_TA_AVB
 	default y
 	help
 	  Enables support for the AVB Trusted Application (TA) in OP-TEE.
-	  The TA can support the "avb" subcommands "read_rb", "write"rb"
+	  The TA can support the "avb" subcommands "read_rb", "write_rb"
 	  and "is_unlocked".
 
 config OPTEE_TA_RPC_TEST
-- 
2.34.1

[PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Igor Opaniuk]

Add calls for closing tee session after every read/write operation.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

(no changes since v1)

 cmd/optee_rpmb.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
index e0e44bbed04..b3cafd92410 100644
--- a/cmd/optee_rpmb.c
+++ b/cmd/optee_rpmb.c
@@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
 
 	rc = tee_shm_alloc(tee, name_size,
 			   TEE_SHM_ALLOC, &shm_name);
-	if (rc)
-		return -ENOMEM;
+	if (rc) {
+		rc = -ENOMEM;
+		goto close_session;
+	}
 
 	rc = tee_shm_alloc(tee, buffer_size,
 			   TEE_SHM_ALLOC, &shm_buf);
@@ -125,6 +127,9 @@ out:
 	tee_shm_free(shm_buf);
 free_name:
 	tee_shm_free(shm_name);
+close_session:
+	tee_close_session(tee, session);
+	tee = NULL;
 
 	return rc;
 }
@@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
 	struct tee_param param[2];
 	size_t name_size = strlen(name) + 1;
 
+	if (!value_size)
+		return -EINVAL;
+
 	if (!tee) {
 		if (avb_ta_open_session())
 			return -ENODEV;
 	}
-	if (!value_size)
-		return -EINVAL;
 
 	rc = tee_shm_alloc(tee, name_size,
 			   TEE_SHM_ALLOC, &shm_name);
-	if (rc)
-		return -ENOMEM;
+	if (rc) {
+		rc = -ENOMEM;
+		goto close_session;
+	}
 
 	rc = tee_shm_alloc(tee, value_size,
 			   TEE_SHM_ALLOC, &shm_buf);
@@ -178,6 +186,9 @@ out:
 	tee_shm_free(shm_buf);
 free_name:
 	tee_shm_free(shm_name);
+close_session:
+	tee_close_session(tee, session);
+	tee = NULL;
 
 	return rc;
 }
-- 
2.34.1

[PATCH v4 3/5] cmd: optee_rpmb: build cmd for sandbox

[Igor Opaniuk]

Support CMD_OPTEE_RPMB for SANDBOX configurations.
Test:

$ ./u-boot -d arch/sandbox/dts/test.dtb
...
=> optee_rpmb write_pvalue test_variable test_value
Wrote 11 bytes
=> optee_rpmb read_pvalue test_variable 11
Read 11 bytes, value = test_value

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

(no changes since v2)

Changes in v2:
- Applied R-b and T-b tags

 cmd/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 61e280fb1a4..227d66a7eea 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1394,7 +1394,9 @@ config CMD_CLONE
 
 config CMD_OPTEE_RPMB
 	bool "Enable read/write support on RPMB via OPTEE"
-	depends on SUPPORT_EMMC_RPMB && OPTEE
+	depends on (SUPPORT_EMMC_RPMB && OPTEE) || SANDBOX_TEE
+	default y if SANDBOX_TEE
+	select OPTEE_TA_AVB if SANDBOX_TEE
 	help
 	  Enable the commands for reading, writing persistent named values
 	  in the Replay Protection Memory Block partition in eMMC by
-- 
2.34.1

[PATCH v4 4/5] test: py: add optee_rpmb tests

[Igor Opaniuk]

Add read/write tests for optee_rpmb cmd.

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

(no changes since v1)

 test/py/tests/test_optee_rpmb.py | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 test/py/tests/test_optee_rpmb.py

diff --git a/test/py/tests/test_optee_rpmb.py b/test/py/tests/test_optee_rpmb.py
new file mode 100644
index 00000000000..8a081b5c494
--- /dev/null
+++ b/test/py/tests/test_optee_rpmb.py
@@ -0,0 +1,20 @@
+# SPDX-License-Identifier:  GPL-2.0+
+#
+# Tests for OP-TEE RPMB read/write support
+
+"""
+This tests optee_rpmb cmd in U-Boot
+"""
+
+import pytest
+import u_boot_utils as util
+
+@pytest.mark.buildconfigspec('cmd_optee_rpmb')
+def test_optee_rpmb_read_write(u_boot_console):
+    """Test OP-TEE RPMB cmd read/write
+    """
+    response = u_boot_console.run_command('optee_rpmb write_pvalue test_variable test_value')
+    assert response == 'Wrote 11 bytes'
+
+    response = u_boot_console.run_command('optee_rpmb read_pvalue test_variable 11')
+    assert response == 'Read 11 bytes, value = test_value'
\ No newline at end of file
-- 
2.34.1

[PATCH v4 5/5] tee: remove common.h inclusion

[Igor Opaniuk]

The usage of the common.h include file is deprecated [1], and has already
been removed from several files.
Get rid of all inclusions in the "drivers/tee" directory, and replace it
with required include files directly where needed.

[1] doc/develop/codingstyle.rst

Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
---

(no changes since v3)

Changes in v3:
- Added calls for closing tee session after every read/write operation
- Added calls for closing tee session after every read/write operation

Changes in v2:
- Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'
- Applied R-b and T-b tags
- Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'

 drivers/tee/broadcom/chimp_optee.c | 3 ++-
 drivers/tee/optee/core.c           | 1 -
 drivers/tee/optee/i2c.c            | 1 -
 drivers/tee/optee/rpmb.c           | 1 -
 drivers/tee/optee/supplicant.c     | 2 +-
 drivers/tee/sandbox.c              | 2 +-
 drivers/tee/tee-uclass.c           | 1 -
 7 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/tee/broadcom/chimp_optee.c b/drivers/tee/broadcom/chimp_optee.c
index 37f9b094f76..bd146ef2899 100644
--- a/drivers/tee/broadcom/chimp_optee.c
+++ b/drivers/tee/broadcom/chimp_optee.c
@@ -3,9 +3,10 @@
  * Copyright 2020 Broadcom.
  */
 
-#include <common.h>
 #include <tee.h>
 #include <broadcom/chimp.h>
+#include <linux/errno.h>
+#include <string.h>
 
 #ifdef CONFIG_CHIMP_OPTEE
 
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 47f845cffe3..5fc0505c788 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2018-2020 Linaro Limited
  */
 
-#include <common.h>
 #include <cpu_func.h>
 #include <dm.h>
 #include <dm/device_compat.h>
diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
index ef4e10f9912..e3fb99897c5 100644
--- a/drivers/tee/optee/i2c.c
+++ b/drivers/tee/optee/i2c.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2020 Foundries.io Ltd
  */
 
-#include <common.h>
 #include <dm.h>
 #include <i2c.h>
 #include <tee.h>
diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
index 5bc13757ea8..bacced6af6c 100644
--- a/drivers/tee/optee/rpmb.c
+++ b/drivers/tee/optee/rpmb.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2018 Linaro Limited
  */
 
-#include <common.h>
 #include <dm.h>
 #include <log.h>
 #include <tee.h>
diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
index f9dd874b594..8a426f53ba8 100644
--- a/drivers/tee/optee/supplicant.c
+++ b/drivers/tee/optee/supplicant.c
@@ -3,10 +3,10 @@
  * Copyright (c) 2018, Linaro Limited
  */
 
-#include <common.h>
 #include <log.h>
 #include <malloc.h>
 #include <tee.h>
+#include <linux/errno.h>
 #include <linux/types.h>
 
 #include "optee_msg.h"
diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
index ec66401878c..8ad7c09efdd 100644
--- a/drivers/tee/sandbox.c
+++ b/drivers/tee/sandbox.c
@@ -2,7 +2,7 @@
 /*
  * Copyright (C) 2018 Linaro Limited
  */
-#include <common.h>
+
 #include <dm.h>
 #include <sandboxtee.h>
 #include <tee.h>
diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
index 52412a4098e..0194d732193 100644
--- a/drivers/tee/tee-uclass.c
+++ b/drivers/tee/tee-uclass.c
@@ -5,7 +5,6 @@
 
 #define LOG_CATEGORY UCLASS_TEE
 
-#include <common.h>
 #include <cpu_func.h>
 #include <dm.h>
 #include <log.h>
-- 
2.34.1

Re: [PATCH v4 0/5] TEE: minor cleanup

[Ilias Apalodimas]

Thanks Igor

I'll pick this up

On Thu, 4 Apr 2024 at 11:13, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> - Address some spelling errors and typos
> - Support CMD_OPTEE_RPMB for SANDBOX configurations and add python tests
> - Remove common.h inclusion for drivers/tee
> - Add calls for closing tee session after every read/write operation.
>
> CI build:
> [1] https://dev.azure.com/igoropaniuk/u-boot/_build/results?buildId=31&view=results
>
> Changes in v4:
> - Rebased on the latest master and excluded "tee: sandbox: fix spelling errors",
>   as it was merged already by Heinrich Schuchardt
>
> Changes in v3:
> - Added calls for closing tee session after every read/write operation
>
> Changes in v2:
> - Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'
> - Applied R-b and T-b tags
>
> Igor Opaniuk (5):
>   tee: optee: fix description in Kconfig
>   cmd: optee_rpmb: close tee session
>   cmd: optee_rpmb: build cmd for sandbox
>   test: py: add optee_rpmb tests
>   tee: remove common.h inclusion
>
>  cmd/Kconfig                        |  4 +++-
>  cmd/optee_rpmb.c                   | 23 +++++++++++++++++------
>  drivers/tee/broadcom/chimp_optee.c |  3 ++-
>  drivers/tee/optee/Kconfig          |  2 +-
>  drivers/tee/optee/core.c           |  1 -
>  drivers/tee/optee/i2c.c            |  1 -
>  drivers/tee/optee/rpmb.c           |  1 -
>  drivers/tee/optee/supplicant.c     |  2 +-
>  drivers/tee/sandbox.c              |  2 +-
>  drivers/tee/tee-uclass.c           |  1 -
>  test/py/tests/test_optee_rpmb.py   | 20 ++++++++++++++++++++
>  11 files changed, 45 insertions(+), 15 deletions(-)
>  create mode 100644 test/py/tests/test_optee_rpmb.py
>
> --
> 2.34.1
>

Re: [PATCH v4 5/5] tee: remove common.h inclusion

[Ilias Apalodimas]

I've already reviewed this one. [0]
I'll add my tag

[0] https://lore.kernel.org/u-boot/CAC_iWjLZcKEo6-0P_gkz5UOe7VanuNv1+=i3xcUTKVo5B1tpQg@mail.gmail.com/
Cheers
/Ilias
On Thu, 4 Apr 2024 at 11:14, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> The usage of the common.h include file is deprecated [1], and has already
> been removed from several files.
> Get rid of all inclusions in the "drivers/tee" directory, and replace it
> with required include files directly where needed.
>
> [1] doc/develop/codingstyle.rst
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> ---
>
> (no changes since v3)
>
> Changes in v3:
> - Added calls for closing tee session after every read/write operation
> - Added calls for closing tee session after every read/write operation
>
> Changes in v2:
> - Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'
> - Applied R-b and T-b tags
> - Fixed chimp_optee.c:37:9: error: implicit declaration of function 'memset'
>
>  drivers/tee/broadcom/chimp_optee.c | 3 ++-
>  drivers/tee/optee/core.c           | 1 -
>  drivers/tee/optee/i2c.c            | 1 -
>  drivers/tee/optee/rpmb.c           | 1 -
>  drivers/tee/optee/supplicant.c     | 2 +-
>  drivers/tee/sandbox.c              | 2 +-
>  drivers/tee/tee-uclass.c           | 1 -
>  7 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tee/broadcom/chimp_optee.c b/drivers/tee/broadcom/chimp_optee.c
> index 37f9b094f76..bd146ef2899 100644
> --- a/drivers/tee/broadcom/chimp_optee.c
> +++ b/drivers/tee/broadcom/chimp_optee.c
> @@ -3,9 +3,10 @@
>   * Copyright 2020 Broadcom.
>   */
>
> -#include <common.h>
>  #include <tee.h>
>  #include <broadcom/chimp.h>
> +#include <linux/errno.h>
> +#include <string.h>
>
>  #ifdef CONFIG_CHIMP_OPTEE
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index 47f845cffe3..5fc0505c788 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -3,7 +3,6 @@
>   * Copyright (c) 2018-2020 Linaro Limited
>   */
>
> -#include <common.h>
>  #include <cpu_func.h>
>  #include <dm.h>
>  #include <dm/device_compat.h>
> diff --git a/drivers/tee/optee/i2c.c b/drivers/tee/optee/i2c.c
> index ef4e10f9912..e3fb99897c5 100644
> --- a/drivers/tee/optee/i2c.c
> +++ b/drivers/tee/optee/i2c.c
> @@ -3,7 +3,6 @@
>   * Copyright (c) 2020 Foundries.io Ltd
>   */
>
> -#include <common.h>
>  #include <dm.h>
>  #include <i2c.h>
>  #include <tee.h>
> diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> index 5bc13757ea8..bacced6af6c 100644
> --- a/drivers/tee/optee/rpmb.c
> +++ b/drivers/tee/optee/rpmb.c
> @@ -3,7 +3,6 @@
>   * Copyright (c) 2018 Linaro Limited
>   */
>
> -#include <common.h>
>  #include <dm.h>
>  #include <log.h>
>  #include <tee.h>
> diff --git a/drivers/tee/optee/supplicant.c b/drivers/tee/optee/supplicant.c
> index f9dd874b594..8a426f53ba8 100644
> --- a/drivers/tee/optee/supplicant.c
> +++ b/drivers/tee/optee/supplicant.c
> @@ -3,10 +3,10 @@
>   * Copyright (c) 2018, Linaro Limited
>   */
>
> -#include <common.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <tee.h>
> +#include <linux/errno.h>
>  #include <linux/types.h>
>
>  #include "optee_msg.h"
> diff --git a/drivers/tee/sandbox.c b/drivers/tee/sandbox.c
> index ec66401878c..8ad7c09efdd 100644
> --- a/drivers/tee/sandbox.c
> +++ b/drivers/tee/sandbox.c
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2018 Linaro Limited
>   */
> -#include <common.h>
> +
>  #include <dm.h>
>  #include <sandboxtee.h>
>  #include <tee.h>
> diff --git a/drivers/tee/tee-uclass.c b/drivers/tee/tee-uclass.c
> index 52412a4098e..0194d732193 100644
> --- a/drivers/tee/tee-uclass.c
> +++ b/drivers/tee/tee-uclass.c
> @@ -5,7 +5,6 @@
>
>  #define LOG_CATEGORY UCLASS_TEE
>
> -#include <common.h>
>  #include <cpu_func.h>
>  #include <dm.h>
>  #include <log.h>
> --
> 2.34.1
>

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Ilias Apalodimas]

Hi Igor,

I was about to apply the series, but noticed neither me or Jens were cc'ed
on this. Adding Jens to the party

On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
> Add calls for closing tee session after every read/write operation.

What the patch is pretty obvious, but I am missing an explanation of why
this is needed

>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> ---
>
> (no changes since v1)
>
>  cmd/optee_rpmb.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
> index e0e44bbed04..b3cafd92410 100644
> --- a/cmd/optee_rpmb.c
> +++ b/cmd/optee_rpmb.c
> @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
>
>  	rc = tee_shm_alloc(tee, name_size,
>  			   TEE_SHM_ALLOC, &shm_name);
> -	if (rc)
> -		return -ENOMEM;
> +	if (rc) {
> +		rc = -ENOMEM;
> +		goto close_session;
> +	}

I don't think you need the session to be opened to allocate memory.
So instead of of doing this, why don't we just move the alloc call before
opening the session?

>
>  	rc = tee_shm_alloc(tee, buffer_size,
>  			   TEE_SHM_ALLOC, &shm_buf);
> @@ -125,6 +127,9 @@ out:
>  	tee_shm_free(shm_buf);
>  free_name:
>  	tee_shm_free(shm_name);
> +close_session:
> +	tee_close_session(tee, session);
> +	tee = NULL;
>
>  	return rc;
>  }
> @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
>  	struct tee_param param[2];
>  	size_t name_size = strlen(name) + 1;
>
> +	if (!value_size)
> +		return -EINVAL;
> +
>  	if (!tee) {
>  		if (avb_ta_open_session())
>  			return -ENODEV;
>  	}
> -	if (!value_size)
> -		return -EINVAL;
>
>  	rc = tee_shm_alloc(tee, name_size,
>  			   TEE_SHM_ALLOC, &shm_name);
> -	if (rc)
> -		return -ENOMEM;
> +	if (rc) {
> +		rc = -ENOMEM;
> +		goto close_session;
> +	}

ditto

>
>  	rc = tee_shm_alloc(tee, value_size,
>  			   TEE_SHM_ALLOC, &shm_buf);
> @@ -178,6 +186,9 @@ out:
>  	tee_shm_free(shm_buf);
>  free_name:
>  	tee_shm_free(shm_name);
> +close_session:
> +	tee_close_session(tee, session);
> +	tee = NULL;
>
>  	return rc;
>  }
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Ilias Apalodimas]

Hi Igor,

On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Igor,
>
> I was about to apply the series, but noticed neither me or Jens were cc'ed
> on this. Adding Jens to the party
>
> On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
> > Add calls for closing tee session after every read/write operation.
>
> What the patch is pretty obvious, but I am missing an explanation of why
> this is needed
>
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
> > index e0e44bbed04..b3cafd92410 100644
> > --- a/cmd/optee_rpmb.c
> > +++ b/cmd/optee_rpmb.c
> > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
> >
> >       rc = tee_shm_alloc(tee, name_size,
> >                          TEE_SHM_ALLOC, &shm_name);
> > -     if (rc)
> > -             return -ENOMEM;
> > +     if (rc) {
> > +             rc = -ENOMEM;
> > +             goto close_session;
> > +     }
>
> I don't think you need the session to be opened to allocate memory.
> So instead of of doing this, why don't we just move the alloc call before
> opening the session?

Looking at it again, we do need tee != NULL, but I think it's cleaner
to add something like
if (!dev)
    return -ENODEV
to __tee_shm_add() instead.

Cheers
/Ilias
>
> >
> >       rc = tee_shm_alloc(tee, buffer_size,
> >                          TEE_SHM_ALLOC, &shm_buf);
> > @@ -125,6 +127,9 @@ out:
> >       tee_shm_free(shm_buf);
> >  free_name:
> >       tee_shm_free(shm_name);
> > +close_session:
> > +     tee_close_session(tee, session);
> > +     tee = NULL;
> >
> >       return rc;
> >  }
> > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
> >       struct tee_param param[2];
> >       size_t name_size = strlen(name) + 1;
> >
> > +     if (!value_size)
> > +             return -EINVAL;
> > +
> >       if (!tee) {
> >               if (avb_ta_open_session())
> >                       return -ENODEV;
> >       }
> > -     if (!value_size)
> > -             return -EINVAL;
> >
> >       rc = tee_shm_alloc(tee, name_size,
> >                          TEE_SHM_ALLOC, &shm_name);
> > -     if (rc)
> > -             return -ENOMEM;
> > +     if (rc) {
> > +             rc = -ENOMEM;
> > +             goto close_session;
> > +     }
>
> ditto
>
> >
> >       rc = tee_shm_alloc(tee, value_size,
> >                          TEE_SHM_ALLOC, &shm_buf);
> > @@ -178,6 +186,9 @@ out:
> >       tee_shm_free(shm_buf);
> >  free_name:
> >       tee_shm_free(shm_name);
> > +close_session:
> > +     tee_close_session(tee, session);
> > +     tee = NULL;
> >
> >       return rc;
> >  }
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Igor Opaniuk]

Hi Ilias,

On Thu, Apr 4, 2024 at 10:40 AM Ilias Apalodimas <
ilias.apalodimas@linaro.org> wrote:

> Hi Igor,
>
> I was about to apply the series, but noticed neither me or Jens were cc'ed
> on this. Adding Jens to the party
>
I usually rely on automatic CC-list creation by patman. Looks like there is
no
entry in MAINTAINERS for this specific file, that's why only Tom was added.
I'll send a patch for that if you don't mind.


> On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
> > Add calls for closing tee session after every read/write operation.
>
> What the patch is pretty obvious, but I am missing an explanation of why
> this is needed
>
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
> > index e0e44bbed04..b3cafd92410 100644
> > --- a/cmd/optee_rpmb.c
> > +++ b/cmd/optee_rpmb.c
> > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
> >
> >       rc = tee_shm_alloc(tee, name_size,
> >                          TEE_SHM_ALLOC, &shm_name);
> > -     if (rc)
> > -             return -ENOMEM;
> > +     if (rc) {
> > +             rc = -ENOMEM;
> > +             goto close_session;
> > +     }
>
> I don't think you need the session to be opened to allocate memory.
> So instead of of doing this, why don't we just move the alloc call before
> opening the session?
>
That's correct, but avb_ta_open_session() wrapper calls
both tee_find_device()/
tee_open_session(), and tee_shm_alloc() expects valid tee device as param,
which
is initialized by tee_find_device(). My intention was to address the only
one particular issue
that was catched by CI and explained in [1] instead of doing overhaul of
the whole optee_rpmb cmd implementation.


> >
> >       rc = tee_shm_alloc(tee, buffer_size,
> >                          TEE_SHM_ALLOC, &shm_buf);
> > @@ -125,6 +127,9 @@ out:
> >       tee_shm_free(shm_buf);
> >  free_name:
> >       tee_shm_free(shm_name);
> > +close_session:
> > +     tee_close_session(tee, session);
> > +     tee = NULL;
> >
> >       return rc;
> >  }
> > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
> >       struct tee_param param[2];
> >       size_t name_size = strlen(name) + 1;
> >
> > +     if (!value_size)
> > +             return -EINVAL;
> > +
> >       if (!tee) {
> >               if (avb_ta_open_session())
> >                       return -ENODEV;
> >       }
> > -     if (!value_size)
> > -             return -EINVAL;
> >
> >       rc = tee_shm_alloc(tee, name_size,
> >                          TEE_SHM_ALLOC, &shm_name);
> > -     if (rc)
> > -             return -ENOMEM;
> > +     if (rc) {
> > +             rc = -ENOMEM;
> > +             goto close_session;
> > +     }
>
> ditto
>
> >
> >       rc = tee_shm_alloc(tee, value_size,
> >                          TEE_SHM_ALLOC, &shm_buf);
> > @@ -178,6 +186,9 @@ out:
> >       tee_shm_free(shm_buf);
> >  free_name:
> >       tee_shm_free(shm_name);
> > +close_session:
> > +     tee_close_session(tee, session);
> > +     tee = NULL;
> >
> >       return rc;
> >  }
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias
>

[1]
https://lore.kernel.org/all/20240302220108.720637-5-igor.opaniuk@gmail.com/T/

-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Igor Opaniuk]

Hi Ilias,

On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <
ilias.apalodimas@linaro.org> wrote:

> Hi Igor,
>
> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Igor,
> >
> > I was about to apply the series, but noticed neither me or Jens were
> cc'ed
> > on this. Adding Jens to the party
> >
> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
> > > Add calls for closing tee session after every read/write operation.
> >
> > What the patch is pretty obvious, but I am missing an explanation of why
> > this is needed
> >
> > >
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
> > > index e0e44bbed04..b3cafd92410 100644
> > > --- a/cmd/optee_rpmb.c
> > > +++ b/cmd/optee_rpmb.c
> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
> > >
> > >       rc = tee_shm_alloc(tee, name_size,
> > >                          TEE_SHM_ALLOC, &shm_name);
> > > -     if (rc)
> > > -             return -ENOMEM;
> > > +     if (rc) {
> > > +             rc = -ENOMEM;
> > > +             goto close_session;
> > > +     }
> >
> > I don't think you need the session to be opened to allocate memory.
> > So instead of of doing this, why don't we just move the alloc call before
> > opening the session?
>
> Looking at it again, we do need tee != NULL, but I think it's cleaner
> to add something like
> if (!dev)
>     return -ENODEV
> to __tee_shm_add() instead.
>
Do you mind if I address that in a separate patch series, as tbh I
don't want to add additional patches to the existing series?


>
> Cheers
> /Ilias
> >
> > >
> > >       rc = tee_shm_alloc(tee, buffer_size,
> > >                          TEE_SHM_ALLOC, &shm_buf);
> > > @@ -125,6 +127,9 @@ out:
> > >       tee_shm_free(shm_buf);
> > >  free_name:
> > >       tee_shm_free(shm_name);
> > > +close_session:
> > > +     tee_close_session(tee, session);
> > > +     tee = NULL;
> > >
> > >       return rc;
> > >  }
> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char
> *name,
> > >       struct tee_param param[2];
> > >       size_t name_size = strlen(name) + 1;
> > >
> > > +     if (!value_size)
> > > +             return -EINVAL;
> > > +
> > >       if (!tee) {
> > >               if (avb_ta_open_session())
> > >                       return -ENODEV;
> > >       }
> > > -     if (!value_size)
> > > -             return -EINVAL;
> > >
> > >       rc = tee_shm_alloc(tee, name_size,
> > >                          TEE_SHM_ALLOC, &shm_name);
> > > -     if (rc)
> > > -             return -ENOMEM;
> > > +     if (rc) {
> > > +             rc = -ENOMEM;
> > > +             goto close_session;
> > > +     }
> >
> > ditto
> >
> > >
> > >       rc = tee_shm_alloc(tee, value_size,
> > >                          TEE_SHM_ALLOC, &shm_buf);
> > > @@ -178,6 +186,9 @@ out:
> > >       tee_shm_free(shm_buf);
> > >  free_name:
> > >       tee_shm_free(shm_name);
> > > +close_session:
> > > +     tee_close_session(tee, session);
> > > +     tee = NULL;
> > >
> > >       return rc;
> > >  }
> > > --
> > > 2.34.1
> > >
> >
> > Thanks
> > /Ilias
>



-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Igor Opaniuk]

HI Ilias

On Thu, Apr 4, 2024 at 12:15 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote:

> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:40 AM Ilias Apalodimas <
> ilias.apalodimas@linaro.org> wrote:
>
>> Hi Igor,
>>
>> I was about to apply the series, but noticed neither me or Jens were cc'ed
>> on this. Adding Jens to the party
>>
> I usually rely on automatic CC-list creation by patman. Looks like there
> is no
> entry in MAINTAINERS for this specific file, that's why only Tom was added.
> I'll send a patch for that if you don't mind.
>

I've added all orphaned tee-related files to MAINTAINERS in [1], so I patman
should  generate the CC list correctly next time.

[1]
https://lore.kernel.org/u-boot/20240404105248.3241506-1-igor.opaniuk@gmail.com/T/#u


>
>
>> On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
>> > Add calls for closing tee session after every read/write operation.
>>
>> What the patch is pretty obvious, but I am missing an explanation of why
>> this is needed
>>
>> >
>> > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
>> > ---
>> >
>> > (no changes since v1)
>> >
>> >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
>> >  1 file changed, 17 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
>> > index e0e44bbed04..b3cafd92410 100644
>> > --- a/cmd/optee_rpmb.c
>> > +++ b/cmd/optee_rpmb.c
>> > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
>> >
>> >       rc = tee_shm_alloc(tee, name_size,
>> >                          TEE_SHM_ALLOC, &shm_name);
>> > -     if (rc)
>> > -             return -ENOMEM;
>> > +     if (rc) {
>> > +             rc = -ENOMEM;
>> > +             goto close_session;
>> > +     }
>>
>> I don't think you need the session to be opened to allocate memory.
>> So instead of of doing this, why don't we just move the alloc call before
>> opening the session?
>>
> That's correct, but avb_ta_open_session() wrapper calls
> both tee_find_device()/
> tee_open_session(), and tee_shm_alloc() expects valid tee device as param,
> which
> is initialized by tee_find_device(). My intention was to address the only
> one particular issue
> that was catched by CI and explained in [1] instead of doing overhaul of
> the whole optee_rpmb cmd implementation.
>
>
>> >
>> >       rc = tee_shm_alloc(tee, buffer_size,
>> >                          TEE_SHM_ALLOC, &shm_buf);
>> > @@ -125,6 +127,9 @@ out:
>> >       tee_shm_free(shm_buf);
>> >  free_name:
>> >       tee_shm_free(shm_name);
>> > +close_session:
>> > +     tee_close_session(tee, session);
>> > +     tee = NULL;
>> >
>> >       return rc;
>> >  }
>> > @@ -139,17 +144,20 @@ static int write_persistent_value(const char
>> *name,
>> >       struct tee_param param[2];
>> >       size_t name_size = strlen(name) + 1;
>> >
>> > +     if (!value_size)
>> > +             return -EINVAL;
>> > +
>> >       if (!tee) {
>> >               if (avb_ta_open_session())
>> >                       return -ENODEV;
>> >       }
>> > -     if (!value_size)
>> > -             return -EINVAL;
>> >
>> >       rc = tee_shm_alloc(tee, name_size,
>> >                          TEE_SHM_ALLOC, &shm_name);
>> > -     if (rc)
>> > -             return -ENOMEM;
>> > +     if (rc) {
>> > +             rc = -ENOMEM;
>> > +             goto close_session;
>> > +     }
>>
>> ditto
>>
>> >
>> >       rc = tee_shm_alloc(tee, value_size,
>> >                          TEE_SHM_ALLOC, &shm_buf);
>> > @@ -178,6 +186,9 @@ out:
>> >       tee_shm_free(shm_buf);
>> >  free_name:
>> >       tee_shm_free(shm_name);
>> > +close_session:
>> > +     tee_close_session(tee, session);
>> > +     tee = NULL;
>> >
>> >       return rc;
>> >  }
>> > --
>> > 2.34.1
>> >
>>
>> Thanks
>> /Ilias
>>
>
> [1]
> https://lore.kernel.org/all/20240302220108.720637-5-igor.opaniuk@gmail.com/T/
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>
>


-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Ilias Apalodimas]

Hi Igor,

On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Igor,
>>
>> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>> >
>> > Hi Igor,
>> >
>> > I was about to apply the series, but noticed neither me or Jens were cc'ed
>> > on this. Adding Jens to the party
>> >
>> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
>> > > Add calls for closing tee session after every read/write operation.
>> >
>> > What the patch is pretty obvious, but I am missing an explanation of why
>> > this is needed
>> >
>> > >
>> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
>> > > ---
>> > >
>> > > (no changes since v1)
>> > >
>> > >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
>> > >  1 file changed, 17 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
>> > > index e0e44bbed04..b3cafd92410 100644
>> > > --- a/cmd/optee_rpmb.c
>> > > +++ b/cmd/optee_rpmb.c
>> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
>> > >
>> > >       rc = tee_shm_alloc(tee, name_size,
>> > >                          TEE_SHM_ALLOC, &shm_name);
>> > > -     if (rc)
>> > > -             return -ENOMEM;
>> > > +     if (rc) {
>> > > +             rc = -ENOMEM;
>> > > +             goto close_session;
>> > > +     }
>> >
>> > I don't think you need the session to be opened to allocate memory.
>> > So instead of of doing this, why don't we just move the alloc call before
>> > opening the session?
>>
>> Looking at it again, we do need tee != NULL, but I think it's cleaner
>> to add something like
>> if (!dev)
>>     return -ENODEV
>> to __tee_shm_add() instead.
>
> Do you mind if I address that in a separate patch series, as tbh I
> don't want to add additional patches to the existing series?

We still completely change the functionality. So, the patchset is not
a 'tiny cleanup', it instead closes the session every time instead of
keeping it open.
There are a few things going on, that aren't explained clearly in the
commit message
- Why is this needed? Looking at the code it is an actual problem
since sessions tied to the AVB uuid will remain open
- Is there any side effect by always closing the session?
I don't mind merging it as is with a proper commit message, but I
think the alternative is just easier.

Thanks
/Ilias

>
>>
>>
>> Cheers
>> /Ilias
>> >
>> > >
>> > >       rc = tee_shm_alloc(tee, buffer_size,
>> > >                          TEE_SHM_ALLOC, &shm_buf);
>> > > @@ -125,6 +127,9 @@ out:
>> > >       tee_shm_free(shm_buf);
>> > >  free_name:
>> > >       tee_shm_free(shm_name);
>> > > +close_session:
>> > > +     tee_close_session(tee, session);
>> > > +     tee = NULL;
>> > >
>> > >       return rc;
>> > >  }
>> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
>> > >       struct tee_param param[2];
>> > >       size_t name_size = strlen(name) + 1;
>> > >
>> > > +     if (!value_size)
>> > > +             return -EINVAL;
>> > > +
>> > >       if (!tee) {
>> > >               if (avb_ta_open_session())
>> > >                       return -ENODEV;
>> > >       }
>> > > -     if (!value_size)
>> > > -             return -EINVAL;
>> > >
>> > >       rc = tee_shm_alloc(tee, name_size,
>> > >                          TEE_SHM_ALLOC, &shm_name);
>> > > -     if (rc)
>> > > -             return -ENOMEM;
>> > > +     if (rc) {
>> > > +             rc = -ENOMEM;
>> > > +             goto close_session;
>> > > +     }
>> >
>> > ditto
>> >
>> > >
>> > >       rc = tee_shm_alloc(tee, value_size,
>> > >                          TEE_SHM_ALLOC, &shm_buf);
>> > > @@ -178,6 +186,9 @@ out:
>> > >       tee_shm_free(shm_buf);
>> > >  free_name:
>> > >       tee_shm_free(shm_name);
>> > > +close_session:
>> > > +     tee_close_session(tee, session);
>> > > +     tee = NULL;
>> > >
>> > >       return rc;
>> > >  }
>> > > --
>> > > 2.34.1
>> > >
>> >
>> > Thanks
>> > /Ilias
>
>
>
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Igor Opaniuk]

Ilias,

On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas <ilias.apalodimas@linaro.org>
wrote:

> Hi Igor,
>
> On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <
> ilias.apalodimas@linaro.org> wrote:
> >>
> >> Hi Igor,
> >>
> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
> >> <ilias.apalodimas@linaro.org> wrote:
> >> >
> >> > Hi Igor,
> >> >
> >> > I was about to apply the series, but noticed neither me or Jens were
> cc'ed
> >> > on this. Adding Jens to the party
> >> >
> >> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
> >> > > Add calls for closing tee session after every read/write operation.
> >> >
> >> > What the patch is pretty obvious, but I am missing an explanation of
> why
> >> > this is needed
> >> >
> >> > >
> >> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
> >> > > ---
> >> > >
> >> > > (no changes since v1)
> >> > >
> >> > >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
> >> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
> >> > > index e0e44bbed04..b3cafd92410 100644
> >> > > --- a/cmd/optee_rpmb.c
> >> > > +++ b/cmd/optee_rpmb.c
> >> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char
> *name,
> >> > >
> >> > >       rc = tee_shm_alloc(tee, name_size,
> >> > >                          TEE_SHM_ALLOC, &shm_name);
> >> > > -     if (rc)
> >> > > -             return -ENOMEM;
> >> > > +     if (rc) {
> >> > > +             rc = -ENOMEM;
> >> > > +             goto close_session;
> >> > > +     }
> >> >
> >> > I don't think you need the session to be opened to allocate memory.
> >> > So instead of of doing this, why don't we just move the alloc call
> before
> >> > opening the session?
> >>
> >> Looking at it again, we do need tee != NULL, but I think it's cleaner
> >> to add something like
> >> if (!dev)
> >>     return -ENODEV
> >> to __tee_shm_add() instead.
> >
> > Do you mind if I address that in a separate patch series, as tbh I
> > don't want to add additional patches to the existing series?
>
> We still completely change the functionality. So, the patchset is not
> a 'tiny cleanup', it instead closes the session every time instead of
> keeping it open.
> There are a few things going on, that aren't explained clearly in the
> commit message
> - Why is this needed? Looking at the code it is an actual problem
> since sessions tied to the AVB uuid will remain open
> - Is there any side effect by always closing the session?
> I don't mind merging it as is with a proper commit message, but I
> think the alternative is just easier.
>
I'll provide a bit more context here. The problem initially was in the TEE
sandbox
driver implementation(drivers/tee/sandbox.c) and it's limitations, which
doesn't
permit to have multiple simultaneous sessions with different TAs.
This is what actually happened in this CI run [1], firstly "optee_rpmb"
cmd was executed (and after execution we had one session open), and
then "scp03", which also makes calls to OP-TEE, however it fails
in sandbox_tee_open_session() because of this check:

if (state->ta) {
    printf("A session is already open\n");
    return -EBUSY;
}

So basically I had two ways in mind to address that:
1. Close a session on each optee_rpmb cmd invocation.
I don't see any reason to keep this session open, as obviously
there is no other mechanism (tbh, I don't know if DM calls ".remove" for
active
devices) to close it automatically before handing over control to
Linux kernel. As a result we might end up with some orphaned sessions
registered in OP-TEE OS core (obvious resource leak).
2. Extend TEE sandbox driver, add support for multiple
simultaneous sessions just to handle the case.

I've chosen the first approach, as IMO it was "kill two birds with one
stone",
I could address resource leak in OP-TEE and bypass limitations of
TEE sandbox driver.

[1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/jobs/792216


>
> Thanks
> /Ilias
>
> >
> >>
> >>
> >> Cheers
> >> /Ilias
> >> >
> >> > >
> >> > >       rc = tee_shm_alloc(tee, buffer_size,
> >> > >                          TEE_SHM_ALLOC, &shm_buf);
> >> > > @@ -125,6 +127,9 @@ out:
> >> > >       tee_shm_free(shm_buf);
> >> > >  free_name:
> >> > >       tee_shm_free(shm_name);
> >> > > +close_session:
> >> > > +     tee_close_session(tee, session);
> >> > > +     tee = NULL;
> >> > >
> >> > >       return rc;
> >> > >  }
> >> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char
> *name,
> >> > >       struct tee_param param[2];
> >> > >       size_t name_size = strlen(name) + 1;
> >> > >
> >> > > +     if (!value_size)
> >> > > +             return -EINVAL;
> >> > > +
> >> > >       if (!tee) {
> >> > >               if (avb_ta_open_session())
> >> > >                       return -ENODEV;
> >> > >       }
> >> > > -     if (!value_size)
> >> > > -             return -EINVAL;
> >> > >
> >> > >       rc = tee_shm_alloc(tee, name_size,
> >> > >                          TEE_SHM_ALLOC, &shm_name);
> >> > > -     if (rc)
> >> > > -             return -ENOMEM;
> >> > > +     if (rc) {
> >> > > +             rc = -ENOMEM;
> >> > > +             goto close_session;
> >> > > +     }
> >> >
> >> > ditto
> >> >
> >> > >
> >> > >       rc = tee_shm_alloc(tee, value_size,
> >> > >                          TEE_SHM_ALLOC, &shm_buf);
> >> > > @@ -178,6 +186,9 @@ out:
> >> > >       tee_shm_free(shm_buf);
> >> > >  free_name:
> >> > >       tee_shm_free(shm_name);
> >> > > +close_session:
> >> > > +     tee_close_session(tee, session);
> >> > > +     tee = NULL;
> >> > >
> >> > >       return rc;
> >> > >  }
> >> > > --
> >> > > 2.34.1
> >> > >
> >> >
> >> > Thanks
> >> > /Ilias
> >
> >
> >
> >
> > --
> > Best regards - Atentamente - Meilleures salutations
> >
> > Igor Opaniuk
> >
> > mailto: igor.opaniuk@gmail.com
> > skype: igor.opanyuk
> > https://www.linkedin.com/in/iopaniuk
>


-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>

Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Ilias Apalodimas]

Hi Igor,


On Thu, 4 Apr 2024 at 14:40, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>
> Ilias,
>
> On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>>
>> Hi Igor,
>>
>> On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
>> >
>> > Hi Ilias,
>> >
>> > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>> >>
>> >> Hi Igor,
>> >>
>> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
>> >> <ilias.apalodimas@linaro.org> wrote:
>> >> >
>> >> > Hi Igor,
>> >> >
>> >> > I was about to apply the series, but noticed neither me or Jens were cc'ed
>> >> > on this. Adding Jens to the party
>> >> >
>> >> > On Thu, Apr 04, 2024 at 10:13:49AM +0200, Igor Opaniuk wrote:
>> >> > > Add calls for closing tee session after every read/write operation.
>> >> >
>> >> > What the patch is pretty obvious, but I am missing an explanation of why
>> >> > this is needed
>> >> >
>> >> > >
>> >> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@gmail.com>
>> >> > > ---
>> >> > >
>> >> > > (no changes since v1)
>> >> > >
>> >> > >  cmd/optee_rpmb.c | 23 +++++++++++++++++------
>> >> > >  1 file changed, 17 insertions(+), 6 deletions(-)
>> >> > >
>> >> > > diff --git a/cmd/optee_rpmb.c b/cmd/optee_rpmb.c
>> >> > > index e0e44bbed04..b3cafd92410 100644
>> >> > > --- a/cmd/optee_rpmb.c
>> >> > > +++ b/cmd/optee_rpmb.c
>> >> > > @@ -87,8 +87,10 @@ static int read_persistent_value(const char *name,
>> >> > >
>> >> > >       rc = tee_shm_alloc(tee, name_size,
>> >> > >                          TEE_SHM_ALLOC, &shm_name);
>> >> > > -     if (rc)
>> >> > > -             return -ENOMEM;
>> >> > > +     if (rc) {
>> >> > > +             rc = -ENOMEM;
>> >> > > +             goto close_session;
>> >> > > +     }
>> >> >
>> >> > I don't think you need the session to be opened to allocate memory.
>> >> > So instead of of doing this, why don't we just move the alloc call before
>> >> > opening the session?
>> >>
>> >> Looking at it again, we do need tee != NULL, but I think it's cleaner
>> >> to add something like
>> >> if (!dev)
>> >>     return -ENODEV
>> >> to __tee_shm_add() instead.
>> >
>> > Do you mind if I address that in a separate patch series, as tbh I
>> > don't want to add additional patches to the existing series?
>>
>> We still completely change the functionality. So, the patchset is not
>> a 'tiny cleanup', it instead closes the session every time instead of
>> keeping it open.
>> There are a few things going on, that aren't explained clearly in the
>> commit message
>> - Why is this needed? Looking at the code it is an actual problem
>> since sessions tied to the AVB uuid will remain open
>> - Is there any side effect by always closing the session?
>> I don't mind merging it as is with a proper commit message, but I
>> think the alternative is just easier.
>
> I'll provide a bit more context here. The problem initially was in the TEE sandbox
> driver implementation(drivers/tee/sandbox.c) and it's limitations, which doesn't
> permit to have multiple simultaneous sessions with different TAs.
> This is what actually happened in this CI run [1], firstly "optee_rpmb"
> cmd was executed (and after execution we had one session open), and
> then "scp03", which also makes calls to OP-TEE, however it fails
> in sandbox_tee_open_session() because of this check:
>
> if (state->ta) {
>     printf("A session is already open\n");
>     return -EBUSY;
> }
>
> So basically I had two ways in mind to address that:
> 1. Close a session on each optee_rpmb cmd invocation.
> I don't see any reason to keep this session open, as obviously
> there is no other mechanism (tbh, I don't know if DM calls ".remove" for active
> devices) to close it automatically before handing over control to
> Linux kernel. As a result we might end up with some orphaned sessions
> registered in OP-TEE OS core (obvious resource leak).
> 2. Extend TEE sandbox driver, add support for multiple
> simultaneous sessions just to handle the case.
>
> I've chosen the first approach, as IMO it was "kill two birds with one stone",
> I could address resource leak in OP-TEE and bypass limitations of
> TEE sandbox driver.

Thanks, this helps.
All this needs to be in the commit message, instead of a mail thread.
Can you send a new revision with the commit message amended?

Thanks
/Ilias
>
> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/jobs/792216
>
>>
>>
>> Thanks
>> /Ilias
>>
>> >
>> >>
>> >>
>> >> Cheers
>> >> /Ilias
>> >> >
>> >> > >
>> >> > >       rc = tee_shm_alloc(tee, buffer_size,
>> >> > >                          TEE_SHM_ALLOC, &shm_buf);
>> >> > > @@ -125,6 +127,9 @@ out:
>> >> > >       tee_shm_free(shm_buf);
>> >> > >  free_name:
>> >> > >       tee_shm_free(shm_name);
>> >> > > +close_session:
>> >> > > +     tee_close_session(tee, session);
>> >> > > +     tee = NULL;
>> >> > >
>> >> > >       return rc;
>> >> > >  }
>> >> > > @@ -139,17 +144,20 @@ static int write_persistent_value(const char *name,
>> >> > >       struct tee_param param[2];
>> >> > >       size_t name_size = strlen(name) + 1;
>> >> > >
>> >> > > +     if (!value_size)
>> >> > > +             return -EINVAL;
>> >> > > +
>> >> > >       if (!tee) {
>> >> > >               if (avb_ta_open_session())
>> >> > >                       return -ENODEV;
>> >> > >       }
>> >> > > -     if (!value_size)
>> >> > > -             return -EINVAL;
>> >> > >
>> >> > >       rc = tee_shm_alloc(tee, name_size,
>> >> > >                          TEE_SHM_ALLOC, &shm_name);
>> >> > > -     if (rc)
>> >> > > -             return -ENOMEM;
>> >> > > +     if (rc) {
>> >> > > +             rc = -ENOMEM;
>> >> > > +             goto close_session;
>> >> > > +     }
>> >> >
>> >> > ditto
>> >> >
>> >> > >
>> >> > >       rc = tee_shm_alloc(tee, value_size,
>> >> > >                          TEE_SHM_ALLOC, &shm_buf);
>> >> > > @@ -178,6 +186,9 @@ out:
>> >> > >       tee_shm_free(shm_buf);
>> >> > >  free_name:
>> >> > >       tee_shm_free(shm_name);
>> >> > > +close_session:
>> >> > > +     tee_close_session(tee, session);
>> >> > > +     tee = NULL;
>> >> > >
>> >> > >       return rc;
>> >> > >  }
>> >> > > --
>> >> > > 2.34.1
>> >> > >
>> >> >
>> >> > Thanks
>> >> > /Ilias
>> >
>> >
>> >
>> >
>> > --
>> > Best regards - Atentamente - Meilleures salutations
>> >
>> > Igor Opaniuk
>> >
>> > mailto: igor.opaniuk@gmail.com
>> > skype: igor.opanyuk
>> > https://www.linkedin.com/in/iopaniuk
>
>
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk