[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 at 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 at 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