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

[sashiko-bot@kernel.org]

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.

> +		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.

> +
> +		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.

> +			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.

> +	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.

> +
> +	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?

> +	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