[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 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
>
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 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?
>
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 at gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>