Re: [PATCH v6 04/16] firmware: qcom: Add a PAS TEE service

[Sumit Garg <sumit.garg@kernel.org>]

On Tue, May 19, 2026 at 07:33:53AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Unvalidated buffer sizes from TEE lead to heap buffer overflow and out-of-bounds read.
> - [High] The `shm_free` cleanup wrapper fails to handle `ERR_PTR`, causing a kernel crash on allocation failure.
> - [High] Unconditional unregistration and global static ops structure corruption in probe/remove.
> --
> 
> commit 117a287c92ded04ad43f9ed963b43a8069a13f94
> Author: Sumit Garg <sumit.garg@oss.qualcomm.com>
> 
> firmware: qcom: Add a PAS TEE service
> 
> Add support for Peripheral Authentication Service (PAS) driver based
> on TEE bus with OP-TEE providing the backend PAS service implementation.
> 
> > diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
> > new file mode 100644
> > index 0000000000000..2067a43c44c37
> > --- /dev/null
> > +++ b/drivers/firmware/qcom/qcom_pas_tee.c
> > @@ -0,0 +1,476 @@
> [ ... ]
> > +DEFINE_FREE(shm_free, struct tee_shm *, tee_shm_free(_T))
> 
> Will this crash if an allocation failure occurs? By default, DEFINE_FREE
> only guards against NULL pointers. However, tee_shm_alloc_kernel_buf() can
> return an ERR_PTR, which evaluates to true.
> 
> > +
> > +static void *qcom_pas_tee_get_rsc_table(struct device *dev,
> > +					struct qcom_pas_context *ctx,
> > +					void *input_rt, size_t input_rt_size,
> > +					size_t *output_rt_size)
> > +{
> [ ... ]
> > +	if (param[1].u.memref.size) {
> > +		struct tee_shm *rt_shm __free(shm_free) =
> > +			tee_shm_alloc_kernel_buf(data->ctx,
> > +						 param[1].u.memref.size);
> 
> If this allocation fails, rt_shm will be an ERR_PTR. When the function
> returns early, shm_free will pass the ERR_PTR to tee_shm_free(), triggering
> a kernel panic.

Ack, will fix it.

> 
> > +		void *rt_shm_va;
> > +
> > +		if (IS_ERR(rt_shm)) {
> > +			dev_err(dev, "rt_shm allocation failed\n");
> > +			return rt_shm;
> > +		}
> > +
> > +		rt_shm_va = tee_shm_get_va(rt_shm, 0);
> > +		if (IS_ERR(rt_shm_va)) {
> > +			dev_err(dev, "rt_shm get VA failed\n");
> > +			return ERR_CAST(rt_shm_va);
> > +		}
> > +		memcpy(rt_shm_va, input_rt, input_rt_size);
> 
> Can this cause a heap buffer overflow? The buffer is allocated using
> param[1].u.memref.size, but the code unconditionally copies input_rt_size
> bytes into it.

Nope, param[1].u.memref.size is always more or equal to input_rt_size
based on OP-TEE ABI.

> 
> > +
> > +		param[1].u.memref.shm = rt_shm;
> > +		ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
> > +		if (ret < 0 || inv_arg.ret != 0) {
> > +			dev_err(dev, "PAS get RT failed, pas_id: %d, ret: %d, err: 0x%x\n",
> > +				ctx->pas_id, ret, inv_arg.ret);
> > +			return ret ? ERR_PTR(ret) : ERR_PTR(-EINVAL);
> > +		}
> > +
> > +		if (param[1].u.memref.size) {
> > +			*output_rt_size = param[1].u.memref.size;
> > +			rt_buf = kmemdup(rt_shm_va, *output_rt_size, GFP_KERNEL);
> 
> Does this kmemdup() introduce an out-of-bounds read? The size returned by
> the TEE in param[1].u.memref.size could potentially be larger than the
> originally allocated size.

Nope, this won't happen as it's part of OP-TEE ABI.

> 
> > +			if (!rt_buf)
> > +				return ERR_PTR(-ENOMEM);
> > +		}
> > +	}
> > +
> > +	return rt_buf;
> > +}
> [ ... ]
> > +static struct qcom_pas_ops qcom_pas_ops_tee = {
> > +	.drv_name		= "qcom-pas-tee",
> > +	.supported		= qcom_pas_tee_supported,
> > +	.init_image		= qcom_pas_tee_init_image,
> > +	.mem_setup		= qcom_pas_tee_mem_setup,
> > +	.get_rsc_table		= qcom_pas_tee_get_rsc_table,
> > +	.auth_and_reset		= qcom_pas_tee_auth_and_reset,
> > +	.prepare_and_auth_reset	= qcom_pas_tee_prepare_and_auth_reset,
> > +	.set_remote_state	= qcom_pas_tee_set_remote_state,
> > +	.shutdown		= qcom_pas_tee_shutdown,
> > +	.metadata_release	= qcom_pas_tee_metadata_release,
> > +};
> > +
> > +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > +	return ver->impl_id == TEE_IMPL_ID_OPTEE;
> > +}
> > +
> > +static int qcom_pas_tee_probe(struct tee_client_device *pas_dev)
> > +{
> > +	struct device *dev = &pas_dev->dev;
> > +	struct qcom_pas_tee_private *data;
> [ ... ]
> > +	data->session_id = sess_arg.session;
> > +	dev_set_drvdata(dev, data);
> > +	qcom_pas_ops_tee.dev = dev;
> 
> Could concurrent probes overwrite this shared static structure? Since the ops
> structure is global, multiple devices probing at the same time could clobber
> the dev pointer.

Only single device is supported on TEE bus here.

> 
> > +	qcom_pas_ops_register(&qcom_pas_ops_tee);
> 
> Should this check for a return value or register failure? If another backend
> like SCM is already registered, this operation will fail, but the probe
> routine proceeds as if it was successful.

Only one PAS backend is expected to be present at a time.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static void qcom_pas_tee_remove(struct tee_client_device *pas_dev)
> > +{
> > +	struct device *dev = &pas_dev->dev;
> > +	struct qcom_pas_tee_private *data = dev_get_drvdata(dev);
> > +
> > +	qcom_pas_ops_unregister();
> 
> If the registration failed in probe, will this unconditionally unregister the
> active SCM backend and break PAS functionality system-wide?

Concurrent PAS backends aren't supported.

-Sumit

> 
> > +	tee_client_close_session(data->ctx, data->session_id);
> > +	tee_client_close_context(data->ctx);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260518072856.22790-1-sumit.garg@kernel.org?part=4