[PATCH v4 0/5] TEE: minor cleanup
- 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=...
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
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
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; }
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
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
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
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
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
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:
- 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
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...
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.c...
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
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
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
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>
I've already reviewed this one. [0] I'll add my tag
[0] https://lore.kernel.org/u-boot/CAC_iWjLZcKEo6-0P_gkz5UOe7VanuNv1+=i3xcUTKVo5... 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
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=...
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
participants (2)
-
Igor Opaniuk -
Ilias Apalodimas