Re: [PATCH v3] tee: fix params_from_user() error path in tee_ioctl_supp_recv

Hi,

On Thu, May 7, 2026 at 11:46 AM Qihang <[email protected]> wrote:
>
> params_from_user() may acquire tee_shm references for MEMREF parameters
> before failing after partially processing the supplied parameter array.
>
> In tee_ioctl_supp_recv(), those references are currently not released on
> that error path.
>
> Fix this by freeing MEMREF references before returning when
> params_from_user() fails.
>
> Keep the final cleanup path in tee_ioctl_supp_recv() unchanged since
> supp_recv() may consume and replace the supplied parameters, unlike the
> other TEE ioctl callback paths.
>
> Signed-off-by: Qihang <[email protected]>
> ---
> v3:
> - only free MEMREF references when params_from_user() fails
> - keep tee_ioctl_supp_recv() final cleanup unchanged
> - follow Jens' review on supp_recv() parameter ownership semantics
>
> v2:
> - rename helper to free_params()
> - drop alloc_num_params and use num_params directly
>
>  drivers/tee/tee_core.c | 54 ++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index ef9642d72672..2fd3a00b47c7 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)

Please fold up. It fits on one line.

> +                       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;
>  }
>
> @@ -846,9 +838,15 @@ static int tee_ioctl_supp_recv(struct tee_context *ctx,
>                 return -ENOMEM;
>
>         rc = params_from_user(ctx, params, num_params, uarg->params);
> -       if (rc)
> -               goto out;
> +       if (rc) {
> +               free_params(params, num_params);
> +               return rc;
> +       }
>
> +       /*
> +        * supp_recv() may consume and replace the supplied parameters, so the
> +        * final cleanup cannot use free_params() like the other ioctl paths.
> +        */
>         rc = ctx->teedev->desc->ops->supp_recv(ctx, &func, &num_params, params);
>         if (rc)
>                 goto out;
> --
> 2.39.5 (Apple Git-154)
>

There are a few warnings:
  AR      drivers/base/firmware_loader/built-in.a
  AR      drivers/base/built-in.a
drivers/tee/tee_core.c: In function ‘tee_ioctl_open_session’:
drivers/tee/tee_core.c:552:16: warning: unused variable ‘n’ [-Wunused-variable]
  552 |         size_t n;
      |                ^
drivers/tee/tee_core.c: In function ‘tee_ioctl_invoke’:
drivers/tee/tee_core.c:622:16: warning: unused variable ‘n’ [-Wunused-variable]
  622 |         size_t n;
      |                ^
drivers/tee/tee_core.c: In function ‘tee_ioctl_object_invoke’:
drivers/tee/tee_core.c:676:16: warning: unused variable ‘n’ [-Wunused-variable]
  676 |         size_t n;
      |                ^

Cheers,
Jens