Re: [PATCH v8 07/14] remoteproc: qcom: Select QCOM_PAS generic service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Heap buffer overflow (OOB write) and undefined behavior in qcom_pas_tee_get_rsc_table due to missing size bounds and NULL validation.
- [High] Out-of-bounds read in qcom_pas_tee_get_rsc_table during kmemdup.
- [High] State corruption due to unconditional unregistration of PAS ops.
--

commit b487f397efddfdc143f4e42a9e7644cc3aeb39da
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

remoteproc: qcom: Select QCOM_PAS generic service

Select PAS generic service driver to enable support for multiple PAS
backends like OP-TEE in addition to SCM.

> diff --git a/drivers/firmware/qcom/qcom_pas.c b/drivers/firmware/qcom/qcom_pas.c
> new file mode 100644
> index 0000000000000..bc6c42f2b3c61
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas.c

[ ... ]

> +void qcom_pas_ops_register(struct qcom_pas_ops *ops)
> +{
> +	if (!qcom_pas_is_available())
> +		/* Paired with smp_load_acquire() in qcom_pas_is_available() */
> +		smp_store_release(&ops_ptr, ops);
> +	else
> +		pr_err("qcom_pas: ops already registered by %s\n",
> +		       ops_ptr->drv_name);
> +}
> +EXPORT_SYMBOL_GPL(qcom_pas_ops_register);
> +
> +void qcom_pas_ops_unregister(void)
> +{
> +	/* Paired with smp_load_acquire() in qcom_pas_is_available() */
> +	smp_store_release(&ops_ptr, NULL);

[Severity: High]
Can this lead to state corruption if a second backend driver fails to register
but subsequently unregisters?

Since qcom_pas_ops_register() returns void instead of an error code, if a
second PAS backend (e.g., TEE) probes after the first (e.g., SCM) has
registered, its probe succeeds without owning the registration.

If the second backend is later removed, its remove callback will invoke
qcom_pas_ops_unregister() and unconditionally clear the legitimately registered
ops_ptr. Could this cause a denial of service for remoteproc subsystems
since subsequent PAS operations would fail with -ENODEV?

> +}
> +EXPORT_SYMBOL_GPL(qcom_pas_ops_unregister);

[ ... ]

> diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
> new file mode 100644
> index 0000000000000..a2ba3af05a502
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas_tee.c

[ ... ]

> +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);
> +		void *rt_shm_va;
> +
> +		if (IS_ERR_OR_NULL(rt_shm)) {
> +			dev_err(dev, "rt_shm allocation failed\n");
> +			rt_shm = NULL;
> +			return ERR_PTR(-ENOMEM);
> +		}
> +
> +		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);

[Severity: High]
Is there a missing bounds check and NULL check here?

The TEE Trusted Application returns an expected buffer size in
param[1].u.memref.size, which is used to allocate the rt_shm buffer. However,
this memcpy unconditionally copies input_rt_size bytes.

If a compromised or malfunctioning TA returns a size smaller than
input_rt_size, could this result in a heap buffer overflow?

Additionally, if input_rt is NULL and input_rt_size is 0 (which is documented
for firmwares without an embedded resource table), wouldn't this result in
undefined behavior by passing a NULL pointer to memcpy?

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

[Severity: High]
Could this kmemdup read out of bounds?

After the second TEE invocation, the TA can modify param[1].u.memref.size,
which is blindly assigned to *output_rt_size and used in kmemdup.

If the TA increases the size beyond the initially allocated rt_shm buffer size,
wouldn't kmemdup read past the end of the buffer?

> +			if (!rt_buf)
> +				return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return rt_buf;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626133440.692849-1-sumit.garg@kernel.org?part=7