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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mhi: sahara: dangling devres and sysfs callbacks on module unload
- [High] mhi: sahara: ddr_training_data sysfs read exposes incomplete data
- [High] sahara: sleep in atomic context in sahara_mhi_dl_xfer_cb()
- [High] mhi/sahara: Use-After-Free of devres training data in sysfs read
- [Medium] bus: mhi: sahara: TOCTOU race in sahara_cntrl_training_get causes split-brain and leak
--

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

bus: mhi: Expose DDR training data via controller sysfs

This commit exposes DDR training data captured during Sahara command mode
via a read-only binary sysfs attribute on the MHI controller device. The data
is served directly from controller scoped storage to userspace to optimize
subsequent boots.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index 11f65cbfa66b8..2f2d18e4ed4da 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
> @@ -273,6 +273,66 @@ static struct sahara_cntrl_training_data *sahara_cntrl_training_get(struct devic
>  	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);

[Severity: Medium]
Since this sysfs read path doesn't hold any lock before calling
sahara_cntrl_training_get(), could we hit a race here?

sahara_cntrl_training_get() performs a check and then allocates:

	ct = devres_find(dev, sahara_cntrl_training_release, ...);
	if (ct)
		return ct;
	ct = devres_alloc(..., GFP_KERNEL);
	...
	devres_add(dev, ct);

If a userspace process reads this sysfs file concurrently while the MHI
device is receiving training data (which also enters this function via
sahara_mhi_dl_xfer_cb() or sahara_processing()), could two concurrent callers
bypass the singleton check, allocating and adding separate instances to the
devres list?

This would cause a memory leak and a split-brain state where the sysfs
reader and the MHI driver operate on different instances.

[Severity: High]
Could this devres allocation sequence also lead to a use-after-free during
controller unbind?

When ddr_training_data_read() creates ct via sahara_cntrl_training_get(),
it adds it to the controller device's devres list after the sysfs cookie
was already added during probe. Because devres lists are LIFO, ct will be
freed before the sysfs cookie is processed.

If the controller device unbinds concurrently, devres_release_all() will
free ct first, and only then call device_remove_bin_file() to drain
active sysfs readers. Would the active sysfs reader continue executing and
call mutex_lock(&ct->lock) on the now-freed ct pointer?

> +	if (!ct)
> +		return -ENODEV;
> +
> +	mutex_lock(&ct->lock);

[Severity: High]
Does taking this mutex cause a scheduling while atomic bug when interacting
with the MHI transfer callbacks?

The MHI transfer callback sahara_mhi_dl_xfer_cb() is executed from the MHI
event tasklet (mhi_ev_task()), which is an atomic context. In that context,
it calls sahara_cntrl_training_get() (which performs a sleeping GFP_KERNEL
allocation) and then acquires this same mutex via mutex_lock(&ct->lock).

Is it safe to sleep in this path?

> +
> +	/* No data yet or offset past end */
> +	if (!ct->data || offset >= ct->size) {
> +		mutex_unlock(&ct->lock);
> +		return 0;
> +	}
> +
> +	available = ct->size - offset;
> +	count = min(count, available);
> +	memcpy(buf, (u8 *)ct->data + offset, count);

[Severity: High]
Could this read expose incomplete or uninitialized data to userspace?

The read computes available data based on ct->size without checking if the
asynchronous transfer is still ongoing (i.e., whether ct->receiving is true).

ct->size is initialized to the full target length in
sahara_command_execute_resp() before the data is actually received via DMA.
Data is then incrementally copied into the buffer from the DL callback
(sahara_mhi_dl_xfer_cb) using ct->copied.

If a userspace process reads the sysfs file concurrently while the transfer
is in progress, wouldn't it receive a partially zero-padded buffer instead
of the actual DDR training data?

Should this rely on ct->copied or check ct->receiving instead?

> +
> +	mutex_unlock(&ct->lock);
> +
> +	return count;
> +}
> +static BIN_ATTR_RO(ddr_training_data, 0);
> +
> +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);
> +	if (ret) {
> +		dev_warn(&mhi_dev->dev,
> +			 "Failed to create DDR training sysfs node (%d)\n", ret);
> +		return;
> +	}
> +
> +	cookie = devres_alloc(sahara_sysfs_devres_release, 1, GFP_KERNEL);
> +	if (!cookie) {
> +		device_remove_bin_file(dev, &bin_attr_ddr_training_data);
> +		return;
> +	}
> +
> +	devres_add(dev, cookie);
> +}

[Severity: High]
Does this leave dangling pointers when the driver module is unloaded?

sahara_sysfs_create() attaches a module-scoped devres release callback
(sahara_sysfs_devres_release) and a sysfs attribute
(bin_attr_ddr_training_data) to the long-lived controller device
(&mhi_dev->mhi_cntrl->mhi_dev->dev).

Since the controller device outlives the client device, and
sahara_mhi_remove() doesn't manually remove the sysfs file and devres
callbacks, won't they become dangling pointers when the module is unloaded?

If the module is unloaded (e.g., rmmod mhi_sahara), any subsequent
teardown of the controller device or a userspace read of the sysfs file
would dereference these unmapped pointers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-sahara_protocol_new_v2-v7-0-90d278bd7414@oss.qualcomm.com?part=7