[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 at gmail.com> wrote:
>
> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
>>
>> 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.
>
> 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 at gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk