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

4 Apr 2024

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

    