Re: [PATCH RFC v2 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: use-after-free of client callback and data due to asynchronous notify_relinquish
--

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

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

Add signal buses for asynchronous notifications between Linux and TEE
endpoints. Allow service drivers to reserve, relinquish, and raise signals,
dispatching incoming notifications via client callbacks.

diff --git a/drivers/firmware/riscv_rpmi_tee/driver.c b/drivers/firmware/riscv_rpmi_tee/driver.c
--- 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]
Is there a risk of a use-after-free or an execution of unmapped module memory
here if a client module unbinds concurrently?

> +
> +	return 0;
> +}

[ ... ]

> +/* 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;
> +
> +	guard(mutex)(&priv->notif.ops_lock);
> +	bus = rpmi_tee_find_signal_bus(priv, rdev->endpoint_id);
> +	if (!bus)
> +		return -EOPNOTSUPP;
> +
> +	if (signal >= bus->tee_to_ree_count)
> +		return -EINVAL;
> +
> +	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 this returns immediately after queuing the release work, does this
leave clients without a way to wait for in-flight callbacks?

If a client driver calls rdev->ops->notifier_ops->notify_relinquish() during
its remove routine, frees its context, and finishes unloading, the worker
thread running rpmi_tee_dispatch_signal() may have already read the cb and
cb_data pointers.

Since there is no synchronous release API (such as one using flush_work()
or a wait mechanism) for clients to guarantee that no callbacks are executing
before module teardown, could this lead to executing a callback after the
client module is unloaded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-riscv-rpmi-tee-abi-v2-0-45939a13d223@oss.qualcomm.com?part=10