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

[Sumit Garg via OP-TEE <op-tee@lists.trustedfirmware.org>]

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 <q.h.hack.winter@gmail.com>
> ---
>  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)
>