Re: [PATCH RFC 1/5] optee: riscv: add RPMI TEE service group transport

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly maps Linux logical CPU IDs to DT mailbox channels and incorrectly sizes the channel array using `num_possible_cpus()`, breaking on sparse CPU masks and differing logical/physical mappings.
- [Medium] Tristate configuration bug causes `optee_riscv.o` to be built as a separate, useless module and silently disables the feature when `CONFIG_RISCV_SBI_MPXY_MBOX` is built as a module (`=m`).
--

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

optee: riscv: add RPMI TEE service group transport

Add an OP-TEE transport for RISC-V using the RPMI TEE service group over
the SBI MPXY mailbox framework.

> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index ad7049c1c1072..925b8ec7ef688 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -9,6 +9,7 @@ optee-objs += supp.o
>  optee-objs += device.o
>  optee-objs += smc_abi.o
>  optee-objs += ffa_abi.o
> +optee-$(CONFIG_RISCV_SBI_MPXY_MBOX) += optee_riscv.o

[Severity: Medium]
Will this fail to link the object into the main optee module when built as
a tristate module?

If CONFIG_RISCV_SBI_MPXY_MBOX is built as a module (=m), this adds
optee_riscv.o to optee-m rather than optee-objs, creating a separate
optee_riscv.ko module instead of properly linking it into optee.ko.

[ ... ]

> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index aefe1e6f56891..8d22d65e087be 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
[ ... ]
> @@ -426,5 +453,12 @@ int optee_smc_abi_register(void);
>  void optee_smc_abi_unregister(void);
>  int optee_ffa_abi_register(void);
>  void optee_ffa_abi_unregister(void);
> +#ifdef CONFIG_RISCV_SBI_MPXY_MBOX
> +int optee_riscv_abi_register(void);
> +void optee_riscv_abi_unregister(void);
> +#else
> +static inline int optee_riscv_abi_register(void) { return -EOPNOTSUPP; }
> +static inline void optee_riscv_abi_unregister(void) { }
> +#endif

[Severity: Medium]
Does this silently disable ABI registration when built as a module?

If CONFIG_RISCV_SBI_MPXY_MBOX is =m, Kbuild defines
CONFIG_RISCV_SBI_MPXY_MBOX_MODULE instead. This causes the macro to evaluate
to false, compiling optee.ko against the dummy ABI stub and silently
dropping the feature.

[ ... ]

> diff --git a/drivers/tee/optee/optee_riscv.c b/drivers/tee/optee/optee_riscv.c
> new file mode 100644
> index 0000000000000..0fe4edf92fc9b
> --- /dev/null
> +++ b/drivers/tee/optee/optee_riscv.c
[ ... ]
> +static int optee_riscv_send(struct optee *optee, struct rpmi_mbox_message *msg)
> +{
> +	int cpu, ret;
> +
> +	cpu = get_cpu();
> +	if (cpu >= optee->riscv.nr_chan || !optee->riscv.chan[cpu]) {

[Severity: High]
Can this index out of bounds on systems with sparse CPU topologies?

Because cpu is a logical ID from get_cpu(), it can be greater than or equal
to num_possible_cpus() on architectures with non-contiguous CPU IDs. This
would cause optee_riscv_send() to reject valid harts and route messages to
the wrong physical channel.

> +		put_cpu();
> +		return -ENODEV;
> +	}
> +	ret = rpmi_mbox_send_message(optee->riscv.chan[cpu], msg);
> +	put_cpu();
> +
> +	return ret;
> +}
[ ... ]
> +static int optee_riscv_request_channels(struct optee *optee)
> +{
> +	struct device *dev = optee->riscv.dev;
> +	int nr_mboxes;
> +	unsigned int cpuid;
> +
> +	nr_mboxes = of_count_phandle_with_args(dev->of_node, "mboxes",
> +					       "#mbox-cells");
> +	if (nr_mboxes != optee->riscv.nr_chan)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Expected %u mailbox channels, got %d\n",
> +				     optee->riscv.nr_chan, nr_mboxes);
> +
> +	for (cpuid = 0; cpuid < optee->riscv.nr_chan; cpuid++) {
> +		optee->riscv.chan[cpuid] =
> +			mbox_request_channel(optee->riscv.client, cpuid);

[Severity: High]
Does this incorrectly assume a 1:1 identity mapping between the Linux logical
CPU ID sequence and the hardware hart order?

Using the loop counter cpuid as the DT property index maps logical IDs
directly to hardware channels, which will route messages incorrectly if the
topology is sparse or harts are disabled.

> +		if (IS_ERR(optee->riscv.chan[cpuid])) {
> +			int ret = PTR_ERR(optee->riscv.chan[cpuid]);
> +
> +			optee->riscv.chan[cpuid] = NULL;
> +			return dev_err_probe(dev, ret,
> +					     "Failed to request channel %u\n",
> +					     cpuid);
> +		}
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int optee_riscv_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rpmi_mbox_message msg;
> +	struct mbox_client *client;
> +	struct optee *optee;
> +	u32 servicegroup_id;
> +	unsigned int nr_cpus;
> +	int ret;
> +
> +	nr_cpus = num_possible_cpus();
> +	if (!nr_cpus)
> +		return dev_err_probe(dev, -ENODEV, "No harts found\n");
> +
[ ... ]
> +	optee->riscv.dev = dev;
> +	optee->riscv.client = client;
> +	optee->riscv.nr_chan = nr_cpus;
> +	optee->riscv.chan = kcalloc(nr_cpus, sizeof(*optee->riscv.chan),
> +				    GFP_KERNEL);

[Severity: High]
Is num_possible_cpus() the right size for this array?

Since the array is later indexed by the logical CPU ID in
optee_riscv_send(), it should likely be sized by nr_cpu_ids instead to
prevent out-of-bounds access vulnerabilities.

> +	if (!optee->riscv.chan) {
> +		ret = -ENOMEM;
> +		goto err_free_optee;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-rpmi-tee-service-grp-dev-v1-0-1d1d35c2a859@oss.qualcomm.com?part=1