Re: [PATCH v2] tee: fix missing shm reference cleanup in tee_ioctl_supp_recv

Hi Qihang,

On Tue, May 5, 2026 at 5:30 PM Qihang <[email protected]> wrote:
>
> params_from_user() acquires tee_shm references for MEMREF parameters and
> expects the caller to release those references with tee_shm_put() during
> cleanup.
>
> tee_ioctl_open_session(), tee_ioctl_invoke(), and
> tee_ioctl_object_invoke() all do this, but tee_ioctl_supp_recv() only
> frees the parameter array and does not drop any acquired shared-memory
> references.
>
> Fix this by using a common helper to release MEMREF references before
> freeing the parameter array, and apply it to tee_ioctl_supp_recv() as
> well.
>
> Signed-off-by: Qihang <[email protected]>
> ---
> v2:
> - rename helper to free_params()
> - drop alloc_num_params and use num_params directly
>
>  drivers/tee/tee_core.c | 46 +++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index ef9642d72672..8cdf2ec7e74f 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -530,6 +530,21 @@ static int params_to_user(struct tee_ioctl_param __user *uparams,
>         return 0;
>  }
>
> +static void free_params(struct tee_param *params, size_t num_params)
> +{
> +       size_t n;
> +
> +       if (!params)
> +               return;
> +
> +       for (n = 0; n < num_params; n++)
> +               if (tee_param_is_memref(params + n) &&
> +                   params[n].u.memref.shm)
> +                       tee_shm_put(params[n].u.memref.shm);
> +
> +       kfree(params);
> +}
> +
>  static int tee_ioctl_open_session(struct tee_context *ctx,
>                                   struct tee_ioctl_buf_data __user *ubuf)
>  {
> @@ -595,16 +610,7 @@ static int tee_ioctl_open_session(struct tee_context *ctx,
>          */
>         if (rc && have_session && ctx->teedev->desc->ops->close_session)
>                 ctx->teedev->desc->ops->close_session(ctx, arg.session);
> -
> -       if (params) {
> -               /* Decrease ref count for all valid shared memory pointers */
> -               for (n = 0; n < arg.num_params; n++)
> -                       if (tee_param_is_memref(params + n) &&
> -                           params[n].u.memref.shm)
> -                               tee_shm_put(params[n].u.memref.shm);
> -               kfree(params);
> -       }
> -
> +       free_params(params, arg.num_params);
>         return rc;
>  }
>
> @@ -657,14 +663,7 @@ static int tee_ioctl_invoke(struct tee_context *ctx,
>         }
>         rc = params_to_user(uparams, arg.num_params, params);
>  out:
> -       if (params) {
> -               /* Decrease ref count for all valid shared memory pointers */
> -               for (n = 0; n < arg.num_params; n++)
> -                       if (tee_param_is_memref(params + n) &&
> -                           params[n].u.memref.shm)
> -                               tee_shm_put(params[n].u.memref.shm);
> -               kfree(params);
> -       }
> +       free_params(params, arg.num_params);
>         return rc;
>  }
>
> @@ -716,14 +715,7 @@ static int tee_ioctl_object_invoke(struct tee_context *ctx,
>         }
>         rc = params_to_user(uparams, arg.num_params, params);
>  out:
> -       if (params) {
> -               /* Decrease ref count for all valid shared memory pointers */
> -               for (n = 0; n < arg.num_params; n++)
> -                       if (tee_param_is_memref(params + n) &&
> -                           params[n].u.memref.shm)
> -                               tee_shm_put(params[n].u.memref.shm);
> -               kfree(params);
> -       }
> +       free_params(params, arg.num_params);
>         return rc;
>  }
>
> @@ -861,7 +853,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx,
>
>         rc = params_to_supp(ctx, uarg->params, num_params, params);
>  out:
> -       kfree(params);
> +       free_params(params, num_params);

This doesn't work. The supp_recv() callback is special compared to
open_session(), invoke_func(), and object_invoke_func() callbacks.
supp_recv() replaces the supplied parameters with other parameters.
Eventually inserted memrefs are reference-counted separately, and
adding it here too would only add unnecessary updates (supp_recv()
would need to increase them, only so they can be decreased here).
However, I think a comment before the call to supp_recv() to remind us
of that would be nice.

However, there's a bug that needs to be fixed if params_from_user()
fails. One way of dealing with that is:
        rc = params_from_user(ctx, params, num_params, uarg->params);
        if (rc) {
                free_params(params, num_params);
                return rc;
        }

and then keep the end of tee_ioctl_supp_recv() unchanged.

Cheers,
Jens

>         return rc;
>  }
>
> --
> 2.39.5 (Apple Git-154)
>