Re: [PATCH v8 7/7] bus: mhi: Expose DDR training data via controller sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] mhi: use-after-free on module unload due to controller-tied sysfs attr
- [High] mhi: use-after-free in sysfs read due to devres LIFO removal order
- [High] bus/mhi: sleep in atomic context in sahara_mhi_dl_xfer_cb
- [High] mhi: sahara: unbounded kzalloc due to statically false unsigned check
- [High] mhi/sahara: devres cleanup order causes UAF in ddr_training_data_read
- [Medium] mhi/sahara: TOCTOU race in devres allocation via sysfs read
- [Medium] mhi/sahara: concurrent sahara_cntrl_training_get calls corrupt devres state
--

commit e76bebb5952f7ff670194a8d2b492b0ba3646b57
Author: Kishore Batta <kishore.batta@oss.qualcomm.com>

bus: mhi: Expose DDR training data via controller sysfs
    
DDR training data captured during Sahara command mode needs to be
accessible to userspace so it can be persisted and reused on subsequent
boots. Expose the captured DDR training data via a read-only binary sysfs
attribute on the MHI controller device.

> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
[ ... ]
>  static struct sahara_cntrl_training_data *sahara_cntrl_training_get(struct device *dev)
>  {
>  	struct sahara_cntrl_training_data *ct;
>  
>  	ct = devres_find(dev, sahara_cntrl_training_release,
>  			 sahara_cntrl_training_match, NULL);
>  	if (ct)
>  		return ct;
>  
>  	ct = devres_alloc(sahara_cntrl_training_release, sizeof(*ct), GFP_KERNEL);
[Severity: Medium]
Since this function is now exposed to userspace via the
ddr_training_data_read() sysfs attribute, could concurrent reads race here?

If a sysfs read races with the driver's internal state machine (cmd_work),
both will find NULL and add a new devres object, and devres_find() will
subsequently return the most recently added object.

If the sysfs-created object (which lacks actual data) is added last and shadows
the driver-created object, wouldn't subsequent MHI RX callbacks write to the
wrong object or discard the data due to a 0-sized copy limit, indefinitely
stalling the DDR training data transfer?

>  	if (!ct)
>  		return NULL;
>  
>  	mutex_init(&ct->lock);
>  	ct->data = NULL;
>  	ct->size = 0;
>  	ct->copied = 0;
>  	ct->receiving = false;
>  
>  	devres_add(dev, ct);
>  	return ct;
>  }
>  
> +static ssize_t ddr_training_data_read(struct file *filp, struct kobject *kobj,
> +				      const struct bin_attribute *attr, char *buf,
> +				      loff_t offset, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct sahara_cntrl_training_data *ct;
> +	size_t available;
> +
> +	ct = sahara_cntrl_training_get(dev);
> +	if (!ct)
> +		return -ENODEV;
> +
> +	mutex_lock(&ct->lock);
[Severity: High]
Does adding the ct structure to devres late cause a use-after-free during
sysfs read?

Since sahara_sysfs_create() adds a devres cookie to the controller device
during probe to remove the sysfs file, and devres releases items in reverse
order of addition (LIFO), during controller device removal, ct will be freed
before the sysfs cookie is released.

If a user is concurrently reading the sysfs file, couldn't
ddr_training_data_read() fetch ct, be preempted while device removal frees ct,
and then resume to execute mutex_lock(&ct->lock) on the freed memory before
the sysfs removal can block it?

[ ... ]
> +static void sahara_sysfs_devres_release(struct device *dev, void *res)
> +{
> +	device_remove_bin_file(dev, &bin_attr_ddr_training_data);
> +}
> +
> +static void sahara_sysfs_create(struct mhi_device *mhi_dev)
> +{
> +	struct device *dev = &mhi_dev->mhi_cntrl->mhi_dev->dev;
> +	void *cookie;
> +	int ret;
> +
> +	if (devres_find(dev, sahara_sysfs_devres_release, NULL, NULL))
> +		return;
> +
> +	ret = device_create_bin_file(dev, &bin_attr_ddr_training_data);
[Severity: High]
Does this cause a use-after-free on module unload?

When the Sahara module is unloaded, sahara_mhi_remove() unbinds the client
device but explicitly leaves the sysfs node and devres callbacks attached to
the parent controller.

Since the sysfs attribute structure, its ddr_training_data_read callback,
and the devres release functions all reside in the module memory, won't
accessing the sysfs file from userspace (or removing the controller device
after unload) dereference unmapped memory and cause a kernel panic?

[ ... ]
>  static void sahara_command_execute_resp(struct sahara_context *context)
>  {
>  	struct device *dev = &context->mhi_dev->mhi_cntrl->mhi_dev->dev;
>  	struct sahara_cntrl_training_data *ct;
>  	u32 client_cmd, resp_len;
>  	int ret;
>  	u64 remaining;
>  	u32 i;
>  
>  	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
>  	    le32_to_cpu(context->rx->command_execute_resp.response_length) < 0) {
[Severity: High]
Is the < 0 check statically false?

Since le32_to_cpu() returns an unsigned __u32, this check fails to validate
untrusted input from the device.

>  		dev_err(&context->mhi_dev->dev,
>  			"Malformed command execute resp packet - length %d\n",
>  			le32_to_cpu(context->rx->length));
>  		return;
>  	}
[ ... ]
>  	ct = sahara_cntrl_training_get(dev);
>  	if (!ct) {
>  		context->is_cmd_mode = false;
>  		sahara_switch_mode_to_img_tx(context);
>  		return;
>  	}
>  
>  	mutex_lock(&ct->lock);
>  	kfree(ct->data);
>  	ct->data = kzalloc(resp_len, GFP_KERNEL);
[Severity: High]
Could a malicious or malfunctioning device trigger a warning in the memory
allocator or cause memory exhaustion (OOM) by sending an excessively large size?

Since the response_length is unchecked (due to the statically false < 0 check),
an unbounded size is passed directly to kzalloc().

[ ... ]
>  static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>  {
>  	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
>  	struct sahara_cntrl_training_data *ct;
>  	struct device *dev;
>  	size_t copy;
>  	int ret;
>  	u32 i;
[ ... ]
>  		for (i = 0; i < context->training_nbuf; i++) {
>  			if (mhi_result->buf_addr == context->cmd_buff[i]) {
>  				mutex_lock(&ct->lock);
[Severity: High]
Can this sleep in atomic context?

sahara_mhi_dl_xfer_cb() is an MHI data transfer callback.
The MHI core calls dl_xfer_cb from the event processing tasklet mhi_ev_task()
while holding spin_lock_bh(&mhi_event->lock).

Acquiring a mutex within an atomic context (tasklet + spinlock) causes a
scheduling while atomic panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-sahara_protocol_new_v2-v8-0-e12939d229a1@oss.qualcomm.com?part=7