Re: [PATCH v7 04/15] firmware: qcom: Add a PAS TEE service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Kernel heap buffer overflow in `qcom_pas_tee_get_rsc_table` when copying the input resource table if the TA returns a smaller size than `input_rt_size`.
- [High] Out-of-bounds read in `qcom_pas_tee_get_rsc_table` when duplicating the output resource table if the TA returns a larger size than the allocated buffer.
- [High] `qcom_pas_tee_get_rsc_table` incorrectly handles the TEE size query semantics, ensuring the operation will always fail.
- [High] Unprotected mutation of the global static `qcom_pas_ops_tee` structure during probe leads to state corruption.
- [High] Calling `memcpy` with a NULL pointer and leaking uninitialized kernel memory to the TEE in `qcom_pas_tee_get_rsc_table`.
- [High] Lack of synchronization during driver removal causes a Use-After-Free in active PAS operations.
- [High] TEE shared memory allocated for metadata is permanently leaked if the driver unbinds before metadata release.
- [High] `qcom_pas_tee_probe` ignores registration failures, potentially breaking other PAS backends upon unload.
- [High] TOCTOU race in `qcom_pas_ops_register` allowing concurrent registrations to silently overwrite each other.
- [High] Lockless access to `ops_ptr` in PAS API allows TOCTOU and Use-After-Free if the backend unregisters concurrently.
--

commit c2c7ee05cef157ede957dcc207ab0044f9b4fa4a
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.
The TEE PAS service ABI is designed to be extensible with additional API.

> diff --git a/drivers/firmware/qcom/qcom_pas_tee.c b/drivers/firmware/qcom/qcom_pas_tee.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/firmware/qcom/qcom_pas_tee.c
[ ... ]
> +static int qcom_pas_tee_init_image(struct device *dev, u32 pas_id,
> +				   const void *metadata, size_t size,
> +				   struct qcom_pas_context *ctx)
> +{
[ ... ]
> +	if (ctx)
> +		ctx->ptr = (void *)mdata_shm;
> +	else
> +		tee_shm_free(mdata_shm);
> +
> +	return ret;
> +}

[Severity: High]
Is there a scenario where this memory is permanently leaked?

The memory stored in ctx->ptr is normally freed by
qcom_pas_tee_metadata_release(). If the driver is unbound,
qcom_pas_ops_unregister() clears the subsystem ops_ptr, turning subsequent
qcom_pas_metadata_release() calls in the core into no-ops.

Because qcom_pas_tee_remove() does not clean up active contexts, does this
leave the tee_shm buffer and its TEE context references permanently allocated?

> +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)
> +{
[ ... ]
> +	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);
> +	}

[Severity: High]
Does this logic correctly handle the TEE Client API semantics for size queries?

According to GlobalPlatform TEE Client API specifications, if a TA requires a
larger buffer during a size query, it updates the size field and returns
TEEC_ERROR_SHORT_BUFFER.

Since this code treats any non-zero return code as a fatal error, will it
abort the operation instead of proceeding to the second pass to allocate
the required buffer?

> +	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);
[ ... ]
> +		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: Critical]
Can this memcpy() overflow the newly allocated rt_shm buffer?

If a compromised or buggy Trusted Application returns a required size strictly
smaller than input_rt_size in the first pass, param[1].u.memref.size will be
smaller than input_rt_size. Copying input_rt_size bytes would then write
past the bounds of the allocated kernel buffer.

[Severity: High]
What happens if input_rt is NULL and input_rt_size is 0?

In that scenario, memcpy() is called with a NULL pointer, which is undefined
behavior in C.

Additionally, if the TA requests a size greater than input_rt_size, the
remainder of the allocated rt_shm_va buffer remains uninitialized. Could this
leak uninitialized kernel memory to the TEE when it is passed as an INOUT
parameter?

> +		param[1].u.memref.shm = rt_shm;
> +		ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
[ ... ]
> +		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() cause an out-of-bounds read?

After the second TA invocation, the TA can update param[1].u.memref.size. If
the TA returns a size larger than the initially allocated rt_shm buffer bounds,
using this updated size in kmemdup() will read past the end of the mapping.

> +static struct qcom_pas_ops qcom_pas_ops_tee = {
> +	.drv_name		= "qcom-pas-tee",
[ ... ]
> +static int qcom_pas_tee_probe(struct tee_client_device *pas_dev)
> +{
[ ... ]
> +	data->session_id = sess_arg.session;
> +	dev_set_drvdata(dev, data);
> +	qcom_pas_ops_tee.dev = dev;

[Severity: High]
Can mutating this global static structure during probe lead to state corruption?

If multiple instances probe or if the device is bound and unbound, this global
state is overwritten. A second bound instance will overwrite the .dev pointer
of the active registered instance.

Could subsequent PAS operations on the original instance then use the wrong
device context or a freed device pointer?

> +	qcom_pas_ops_register(&qcom_pas_ops_tee);
> +
> +	return ret;
> +}

[Severity: High]
Should probe fail if qcom_pas_ops_register() fails?

qcom_pas_ops_register() silently rejects registration if another backend like
SCM is already registered, but qcom_pas_tee_probe() ignores this and reports
success.

If the TEE driver is loaded while SCM is active, it will probe successfully
without owning the PAS subsystem. Upon unload, qcom_pas_tee_remove() will
unconditionally call qcom_pas_ops_unregister(), breaking the legitimately
active SCM backend.

[Severity: High]
Is qcom_pas_ops_register() safe against concurrent registrations?

The generic PAS service checks if (!qcom_pas_is_available()) locklessly before
calling smp_store_release(&ops_ptr, ops). If two backends call this
concurrently, both can read ops_ptr as NULL and pass the check.

This allows the second caller to overwrite the first registration without
raising an error.

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

[Severity: High]
Can the lockless design of the PAS API cause a use-after-free here?

The exported PAS API functions check if (!ops_ptr) locklessly before calling
operations like ops_ptr->init_image(). If qcom_pas_ops_unregister() is called
concurrently during module unload, setting ops_ptr to NULL, an active caller
could dereference the NULL pointer or access the backend's dev while it is
being freed via devres.

> +	tee_client_close_session(data->ctx, data->session_id);
> +	tee_client_close_context(data->ctx);
> +}

[Severity: High]
Does this remove sequence need to synchronize with ongoing PAS callbacks?

The driver does not wait for ongoing PAS callbacks to complete before
unregistering the PAS operations and closing the TEE session.

An inflight operation like qcom_pas_tee_auth_and_reset() can still be
executing when remove() returns. When devres frees data, the active callback
could dereference a dangling pointer and attempt to use a closed TEE context.

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