Re: [PATCH RFC 10/10] firmware: riscv: rpmi-tee: add signal bus support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rpmi_tee: UAF in dispatch_signal() due to async relinquish
--

commit 65e73f5143f8eb36b1c247ca7810975eb375c7cf
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

    firmware: riscv: rpmi-tee: add signal bus support

This commit adds support for RPMI TEE signal buses, enabling bidirectional
asynchronous notifications between service devices and their TEE endpoints.
It introduces a new rpmi_tee_notifier_ops interface for clients to request,
relinquish, and raise signals.

> diff --git a/drivers/firmware/riscv_rpmi_tee/driver.c b/drivers/firmware/riscv_rpmi_tee/driver.c
> index 0004f54c7d899..cc0a42d471e37 100644
> --- a/drivers/firmware/riscv_rpmi_tee/driver.c
> +++ b/drivers/firmware/riscv_rpmi_tee/driver.c
[ ... ]
> +/* Invoke an active signal callback without holding the bus lock. */
> +static int rpmi_tee_dispatch_signal(struct rpmi_tee_signal_bus *bus,
> +				    u32 signal)
> +{
> +	struct rpmi_tee_signal_reservation *resv;
> +	rpmi_tee_notifier_cb cb = NULL;
> +	struct rpmi_tee_device *rdev = NULL;
> +	void *cb_data = NULL;
> +
> +	if (signal >= bus->tee_to_ree_count)
> +		return -EPROTO;
> +
> +	scoped_guard(mutex, &bus->lock) {
> +		resv = xa_load(&bus->reservations, signal);
> +		if (resv && resv->state == RPMI_TEE_SIGNAL_ACTIVE) {
> +			cb = resv->cb;
> +			cb_data = resv->cb_data;
> +			rdev = resv->rdev;
> +		}
> +	}
> +
> +	if (cb)
> +		cb(rdev, signal, cb_data);

[Severity: High]
Does executing the callback without taking a module reference or holding a
lock that synchronizes with driver teardown leave us vulnerable to a race
condition?

If a client driver module is unloaded while a signal notification is being
dispatched, it appears the dispatch thread could read the callback pointer,
drop the bus lock, and be preempted. If the module unloads and frees its
text before the thread resumes, jumping to the freed callback address would
result in a use-after-free panic.

[ ... ]
> +/* Relinquish a previously reserved TEE-to-REE signal. */
> +static int rpmi_tee_op_notify_relinquish(struct rpmi_tee_device *rdev,
> +					 u32 signal)
> +{
> +	struct rpmi_tee_transport *priv = rpmi_tee_device_to_transport(rdev);
> +	struct rpmi_tee_signal_bus *bus;
[ ... ]
> +	scoped_guard(mutex, &bus->lock) {
> +		struct rpmi_tee_signal_reservation *resv;
> +
> +		resv = xa_load(&bus->reservations, signal);
> +		if (!resv)
> +			return -ENOENT;
> +		/* Release only if @signal belongs to @rdev. */
> +		if (resv->rdev != rdev)
> +			return -EPERM;
> +		if (resv->state == RPMI_TEE_SIGNAL_RELEASING)
> +			return -EALREADY;
> +
> +		resv->state = RPMI_TEE_SIGNAL_RELEASING;
> +	}
> +
> +	queue_work(priv->notif.wq, &priv->notif.work);
> +
> +	return 0;

[Severity: High]
Because relinquishing a signal returns immediately without waiting for
in-flight callbacks to finish, can this allow the client module's remove
function to complete and the module to be unloaded while a callback is
still running or preempted in rpmi_tee_dispatch_signal()?

[ ... ]
> diff --git a/include/linux/rpmi_tee.h b/include/linux/rpmi_tee.h
> index c2dd992934813..5f4a0b19b0e12 100644
> --- a/include/linux/rpmi_tee.h
> +++ b/include/linux/rpmi_tee.h
[ ... ]
> +/* RPMI TEE signal notification operations. */
> +struct rpmi_tee_notifier_ops {
> +	/**
> +	 * @notify_request: Allocate a TEE-to-REE signal and associate it with
> +	 *	@cb and @cb_data. The allocated signal is returned through @signal.
> +	 */
> +	int (*notify_request)(struct rpmi_tee_device *rdev,
> +			      rpmi_tee_notifier_cb cb, void *cb_data, u32 *signal);
> +	/**
> +	 * @notify_relinquish: Stop dispatching a TEE-to-REE @signal previously
> +	 *	allocated for @rdev. The release is asynchronous and a callback may
> +	 *	relinquish its own signal.
> +	 */
> +	int (*notify_relinquish)(struct rpmi_tee_device *rdev, u32 signal);

[Severity: High]
The API seems to provide no flush or synchronize mechanism for clients to
safely unload. Is there a way for client drivers to guarantee that all
asynchronous callbacks have completely finished executing before they free
their resources or allow module unload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-riscv-rpmi-tee-abi-v1-0-04908b81d885@oss.qualcomm.com?part=10