Re: [PATCH v6 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:

New issues:
- [Critical] Module lifetime mismatch leads to Use-After-Free for sysfs attribute and devres release callback.
- [High] Devres LIFO teardown ordering causes Use-After-Free during sysfs read.
- [High] Sysfs read callback returns incomplete or zero-padded data during ongoing transfers.

Pre-existing issues:
- [Critical] Module lifetime mismatch leads to Use-After-Free for pre-existing training data devres.
- [High] Sleeping in atomic context during MHI download transfer callback.
- [High] Missing cancellation of `read_data_work` causes Use-After-Free on removal.
- [Medium] TOCTOU race condition in `sahara_cntrl_training_get()` causes memory leak.
--

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

bus: mhi: Expose DDR training data via controller sysfs

This commit exposes captured DDR training data to userspace via a read-only 
binary sysfs attribute on the MHI controller device. It adds a sysfs read 
callback to serve data from the controller-scoped storage and manages the 
attribute's lifetime using devres.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index 07bc743aa061d..72ac751c302a9 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;
>  }

[Severity: Critical]
This is a pre-existing issue, but does sahara_cntrl_training_get() have a
module lifetime mismatch?

When sahara_cntrl_training_get() allocates and attaches a devres structure to
the parent MHI controller device:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_cntrl_training_get() {
    ...
    ct = devres_alloc(sahara_cntrl_training_release, sizeof(*ct), GFP_KERNEL);
    ...
    devres_add(dev, ct);
}

If the Sahara module is unloaded, the release callback function pointer
sahara_cntrl_training_release becomes dangling. When the controller device is
eventually removed, will the driver core execute the dangling devres release
function and cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but can sahara_cntrl_training_get() be called
from an atomic context?

The MHI subsystem invokes the dl_xfer_cb callback in an atomic SoftIRQ/tasklet
context, and it calls sahara_cntrl_training_get():

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_dl_xfer_cb() {
    ...
    ct = sahara_cntrl_training_get(dev);
    ...
    mutex_lock(&ct->lock);
}

Since sahara_cntrl_training_get() uses GFP_KERNEL unconditionally, and
mutex_lock() can sleep, will this cause a scheduling while atomic panic when
DDR training data is received?

[Severity: Medium]
This is a pre-existing issue, but is there a race condition in
sahara_cntrl_training_get() when setting up the devres structure?

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_cntrl_training_get() {
    ...
    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);
    ...
    devres_add(dev, ct);
}

If a sysfs reader and the MHI receive path call this concurrently, couldn't
both observe NULL and allocate separate structures? Because devres_find()
searches in reverse, the later allocation would shadow the earlier one, causing
the earlier one to leak memory and lose its lock/data state.

> +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]
Can ct be freed while we are trying to acquire the lock?

Devres resources are freed in Last-In-First-Out (LIFO) order. Since the sysfs
devres is added during probe, and the training data devres (ct) is added later
during transfer, ct is freed first on controller removal, and the sysfs file
is removed second.

If userspace reads the sysfs file concurrently with device removal, could the
read callback retrieve the ct pointer just before it is freed by
devres_release_all, leading to a use-after-free when attempting to acquire
mutex_lock(&ct->lock)?

> +	/* 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]
Does this correctly bound the read size during an ongoing transfer?

This logic uses ct->size (the expected total size) rather than ct->copied
(actual received bytes), and ignores ct->receiving.

Since the buffer is zero-filled by kzalloc(), if userspace reads the file
before the transfer completes, won't it successfully read the unwritten
portion as zeroes? This could cause the userspace tooling to persist a
truncated or corrupted DDR training image.

[ ... ]
> +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: Critical]
Since sahara is a loadable module, what happens if the module is unloaded?

If the Sahara module is unloaded, the client device unbinds and the module
memory is freed. But since dev points to the parent MHI controller device, the
sysfs file and devres hook remain active on the controller device.

Will a subsequent userspace read of the sysfs file dereference
bin_attr_ddr_training_data and execute ddr_training_data_read in unmapped
memory, leading to a use-after-free?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does sahara_mhi_remove() fail to cancel
read_data_work?

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_remove() {
    ...
    cancel_work_sync(&context->fw_work);
    cancel_work_sync(&context->dump_work);
    cancel_work_sync(&context->cmd_work);
    vfree(context->mem_dump);
    ...
}

The driver initializes context->read_data_work and schedules it asynchronously
in sahara_mhi_ul_xfer_cb(). Since cancel_work_sync(&context->read_data_work)
is omitted during removal, if the work is scheduled just before device removal
completes, will the workqueue execute on the devm-freed context memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701-sahara_protocol_new_v2-v6-0-3a78362c4741@oss.qualcomm.com?part=7