Re: [PATCH RFC v2 03/10] firmware: add RPMI TEE bus support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] firmware: rpmi_tee_device_register() stub returns NULL breaking IS_ERR() expectations
- [Medium] firmware: rpmi_tee: missing modpost support breaks module auto-loading
- [Medium] firmware: rpmi_tee: UAF on module unload via device release callback
- [Low] sysfs: modalias_show() output lacks a trailing newline
--

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

firmware: add RPMI TEE bus support

RPMI TEE endpoints can provide multiple services identified by UUID.
This patch adds a service bus to allow consumer drivers to bind to these
services, exposing endpoint IDs, UUIDs, and modaliases via sysfs.

> diff --git a/drivers/firmware/riscv_rpmi_tee/bus.c b/drivers/firmware/riscv_rpmi_tee/bus.c
> new file mode 100644
> index 0000000000000..1393adda0d3a7
> --- /dev/null
> +++ b/drivers/firmware/riscv_rpmi_tee/bus.c
[ ... ]
> +#define RPMI_TEE_UEVENT_MODALIAS_FMT	"rpmi_tee:%pUb"
[ ... ]
> +static ssize_t modalias_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct rpmi_tee_device *rdev = to_rpmi_tee_dev(dev);
> +
> +	return sysfs_emit(buf, RPMI_TEE_UEVENT_MODALIAS_FMT, &rdev->uuid);
> +}

[Severity: Low]
Does this sysfs output need a trailing newline? RPMI_TEE_UEVENT_MODALIAS_FMT
is defined without a newline (which is correct for add_uevent_var()), but
reusing it here with sysfs_emit() will result in an output string that doesn't
have one. Should a newline be appended for the sysfs interface?

[ ... ]
> +static void rpmi_tee_device_release(struct device *dev)
> +{
> +	struct rpmi_tee_device *rdev = to_rpmi_tee_dev(dev);
> +
> +	ida_free(&rpmi_tee_bus_id, rdev->id);
> +	kfree(rdev);
> +}
[ ... ]
> +struct rpmi_tee_device *
> +rpmi_tee_device_register(const uuid_t *uuid, u32 endpoint_id,
> +			 const struct rpmi_tee_ops *ops, struct device *parent)
> +{
[ ... ]
> +	rdev->dev.parent = parent;
> +	rdev->dev.bus = &rpmi_tee_bus_type;
> +	rdev->dev.release = rpmi_tee_device_release;

[Severity: Medium]
Could this lead to a use-after-free on module unload? If userspace holds a
sysfs attribute open for an rpmi_tee device, the device kobject's refcount
remains non-zero.

If the module is unloaded during this time, the release function and the
global rpmi_tee_bus_id IDA are unmapped.

When the sysfs file is eventually closed and the final kobject refcount is
dropped, could the kernel jump to the unmapped rpmi_tee_device_release()
and access freed IDA memory?

> diff --git a/include/linux/rpmi_tee.h b/include/linux/rpmi_tee.h
> new file mode 100644
> index 0000000000000..c499f0427833e
> --- /dev/null
> +++ b/include/linux/rpmi_tee.h
[ ... ]
> +struct rpmi_tee_device_id {
> +	uuid_t uuid;
> +};

[Severity: Medium]
Is this missing from include/linux/mod_devicetable.h? Without adding
rpmi_tee_device_id to mod_devicetable.h and scripts/mod/file2alias.c, modpost
will silently ignore MODULE_DEVICE_TABLE(rpmi_tee, ...) in consumer drivers.
Will this break udev-based automatic module loading for TEE service drivers?

[ ... ]
> +#if IS_REACHABLE(CONFIG_RISCV_RPMI_TEE_TRANSPORT)
> +struct rpmi_tee_device *
> +rpmi_tee_device_register(const uuid_t *uuid, u32 endpoint_id,
> +			 const struct rpmi_tee_ops *ops, struct device *parent);
[ ... ]
> +#else
> +static inline struct rpmi_tee_device *
> +rpmi_tee_device_register(const uuid_t *uuid, u32 endpoint_id,
> +			 const struct rpmi_tee_ops *ops, struct device *parent)
> +{
> +	return NULL;
> +}

[Severity: High]
Does this fallback stub break IS_ERR() expectations? The actual implementation
of rpmi_tee_device_register() returns an ERR_PTR() on failure, and callers
typically check the return value using IS_ERR().

Since IS_ERR(NULL) evaluates to false, will callers assume device registration
succeeded and attempt to dereference the NULL pointer? Should this stub return
an error pointer such as ERR_PTR(-ENODEV) instead?

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