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

Hi Sumit,

Thanks, that makes sense.

I had been conservative about the possibility of num_params being updated
by supp_recv backends, but looking again at the current paths, I agree
that alloc_num_params is unnecessary here.

I'll drop that, rename the helper to free_params(), and send a v2.

Thanks,
Qihang

On Fri, May 1, 2026 at 10:32 PM Sumit Garg <[email protected]> wrote:
>
> On Wed, Apr 29, 2026 at 07:32:19PM +0800, Qihang 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.
> >
> > Since supp_recv backends may update num_params, preserve the original
> > allocated parameter count for cleanup.
> >
> > Signed-off-by: Qihang <[email protected]>
> > ---
> >  drivers/tee/tee_core.c | 49 +++++++++++++++++++-----------------------
> >  1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index ef9642d72672..adad1ea8e31b 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 params_free_decref(struct tee_param *params, size_t num_params)
>
> I would rather rename this API as free_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);
> > -     }
> > -
> > +     params_free_decref(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);
> > -     }
> > +     params_free_decref(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);
> > -     }
> > +     params_free_decref(params, arg.num_params);
> >       return rc;
> >  }
> >
> > @@ -822,6 +814,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx,
> >       struct tee_iocl_supp_recv_arg __user *uarg;
> >       struct tee_param *params;
> >       u32 num_params;
> > +     u32 alloc_num_params;
> >       u32 func;
> >
> >       if (!ctx->teedev->desc->ops->supp_recv)
> > @@ -838,6 +831,8 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx,
> >       if (get_user(num_params, &uarg->num_params))
> >               return -EFAULT;
> >
> > +     alloc_num_params = num_params;
>
> Why is this needed? Shouldn't the updated num_params will point to size
> of params array?
>
> -Sumit
>
> > +
> >       if (size_add(sizeof(*uarg), TEE_IOCTL_PARAM_SIZE(num_params)) != buf.buf_len)
> >               return -EINVAL;
> >
> > @@ -861,7 +856,7 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx,
> >
> >       rc = params_to_supp(ctx, uarg->params, num_params, params);
> >  out:
> > -     kfree(params);
> > +     params_free_decref(params, alloc_num_params);
> >       return rc;
> >  }
> >
> > --
> > 2.39.5 (Apple Git-154)
> >