Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Hi Igor,
On Thu, 4 Apr 2024 at 14:40, Igor Opaniuk wrote:
>
> Ilias,
>
> On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas
> wrote:
>>
>> Hi Igor,
>>
>> On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk wrote:
>> >
>> > Hi Ilias,
>> >
>> > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas
>> > wrote:
>> >>
>> >> Hi Igor,
>> >>
>> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
>> >> 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
>> >> > > ---
>> >> > >
>> >> > > (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, _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, _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;
>> >> > >
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Ilias,
On Thu, Apr 4, 2024 at 1:00 PM Ilias Apalodimas
wrote:
> Hi Igor,
>
> On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk wrote:
> >
> > Hi Ilias,
> >
> > On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <
> ilias.apalodi...@linaro.org> wrote:
> >>
> >> Hi Igor,
> >>
> >> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
> >> 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
> >> > > ---
> >> > >
> >> > > (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, _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.
[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, _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,
> >> > >
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Hi Igor,
On Thu, 4 Apr 2024 at 13:18, Igor Opaniuk wrote:
>
> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas
> wrote:
>>
>> Hi Igor,
>>
>> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
>> 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
>> > > ---
>> > >
>> > > (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, _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, _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, _name);
>> > > - if (rc)
>> > > - return -ENOMEM;
>> > > + if (rc) {
>> > > + rc = -ENOMEM;
>> > > + goto close_session;
>> > > + }
>> >
>> > ditto
>> >
>> > >
>> > > rc = tee_shm_alloc(tee, value_size,
>> > > TEE_SHM_ALLOC, _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.opan...@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
HI Ilias
On Thu, Apr 4, 2024 at 12:15 PM Igor Opaniuk wrote:
> Hi Ilias,
>
> On Thu, Apr 4, 2024 at 10:40 AM Ilias Apalodimas <
> ilias.apalodi...@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.
>
I've added all orphaned tee-related files to MAINTAINERS in [1], so I patman
should generate the CC list correctly next time.
[1]
https://lore.kernel.org/u-boot/20240404105248.3241506-1-igor.opan...@gmail.com/T/#u
>
>
>> 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
>> > ---
>> >
>> > (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, _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, _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, _name);
>> > - if (rc)
>> > - return -ENOMEM;
>> > + if (rc) {
>> > + rc = -ENOMEM;
>> > + goto close_session;
>> > + }
>>
>> ditto
>>
>> >
>> > rc = tee_shm_alloc(tee, value_size,
>> > TEE_SHM_ALLOC, _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.opan...@gmail.com/T/
>
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opan...@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
>
--
Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opan...@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Hi Ilias,
On Thu, Apr 4, 2024 at 10:54 AM Ilias Apalodimas <
ilias.apalodi...@linaro.org> wrote:
> Hi Igor,
>
> On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
> 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
> > > ---
> > >
> > > (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, _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?
>
> Cheers
> /Ilias
> >
> > >
> > > rc = tee_shm_alloc(tee, buffer_size,
> > > TEE_SHM_ALLOC, _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, _name);
> > > - if (rc)
> > > - return -ENOMEM;
> > > + if (rc) {
> > > + rc = -ENOMEM;
> > > + goto close_session;
> > > + }
> >
> > ditto
> >
> > >
> > > rc = tee_shm_alloc(tee, value_size,
> > > TEE_SHM_ALLOC, _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.opan...@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Hi Ilias,
On Thu, Apr 4, 2024 at 10:40 AM Ilias Apalodimas <
ilias.apalodi...@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
> > ---
> >
> > (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, _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, _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, _name);
> > - if (rc)
> > - return -ENOMEM;
> > + if (rc) {
> > + rc = -ENOMEM;
> > + goto close_session;
> > + }
>
> ditto
>
> >
> > rc = tee_shm_alloc(tee, value_size,
> > TEE_SHM_ALLOC, _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.opan...@gmail.com/T/
--
Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opan...@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
Hi Igor,
On Thu, 4 Apr 2024 at 11:40, Ilias Apalodimas
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
> > ---
> >
> > (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, _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, _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, _name);
> > - if (rc)
> > - return -ENOMEM;
> > + if (rc) {
> > + rc = -ENOMEM;
> > + goto close_session;
> > + }
>
> ditto
>
> >
> > rc = tee_shm_alloc(tee, value_size,
> > TEE_SHM_ALLOC, _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
Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session
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
> ---
>
> (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, _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?
>
> rc = tee_shm_alloc(tee, buffer_size,
> TEE_SHM_ALLOC, _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, _name);
> - if (rc)
> - return -ENOMEM;
> + if (rc) {
> + rc = -ENOMEM;
> + goto close_session;
> + }
ditto
>
> rc = tee_shm_alloc(tee, value_size,
> TEE_SHM_ALLOC, _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
[PATCH v4 2/5] cmd: optee_rpmb: close tee session
Add calls for closing tee session after every read/write operation.
Signed-off-by: Igor Opaniuk
---
(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, _name);
- if (rc)
- return -ENOMEM;
+ if (rc) {
+ rc = -ENOMEM;
+ goto close_session;
+ }
rc = tee_shm_alloc(tee, buffer_size,
TEE_SHM_ALLOC, _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, _name);
- if (rc)
- return -ENOMEM;
+ if (rc) {
+ rc = -ENOMEM;
+ goto close_session;
+ }
rc = tee_shm_alloc(tee, value_size,
TEE_SHM_ALLOC, _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
