Re: [PATCH v4 2/5] cmd: optee_rpmb: close tee session

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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@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?

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