[PATCH v4 2/5] cmd: optee_rpmb: close tee session
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Apr 4 13:46:10 CEST 2024
Hi Igor,
On Thu, 4 Apr 2024 at 14:40, Igor Opaniuk <igor.opaniuk at gmail.com> wrote:
>
> Ilias,
>
> On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
>>
>> 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.
>
> 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.
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 at gmail.com
>> > skype: igor.opanyuk
>> > https://www.linkedin.com/in/iopaniuk
>
>
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk at gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
More information about the U-Boot
mailing list