Re: [PATCH v9 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: module unload UAF due to controller-scoped devres and sysfs
- [High] mhi: sahara: sleeping mutex_lock() in atomic dl_xfer_cb callback
- [High] bus: mhi: sahara: UAF in ddr_training_data_read due to devres release order
- [Medium] mhi: sahara: lockless devres creation in sysfs read causes memory leak
- [Low] sahara: unsigned length validation checked for < 0 is dead code
--

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

bus: mhi: Expose DDR training data via controller sysfs

This commit exposes DDR training data through a new sysfs binary attribute
attached to the MHI controller device. This enables userspace to persist
the captured training data and reload it on subsequent boots to optimize
boot times.

> diff --git a/drivers/bus/mhi/host/clients/sahara/sahara.c b/drivers/bus/mhi/host/clients/sahara/sahara.c
> index aa684355b7a31..be5318fc5e221 100644
> --- a/drivers/bus/mhi/host/clients/sahara/sahara.c
> +++ b/drivers/bus/mhi/host/clients/sahara/sahara.c
> @@ -272,6 +272,67 @@ 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);
> +	if (!ct)
> +		return -ENODEV;
> +
> +	mutex_lock(&ct->lock);

[Severity: High]
In sahara_sysfs_create(), the sysfs devres cookie is added to the device
during probe. However, sahara_cntrl_training_get() dynamically allocates and
adds the ct structure to the same device's devres list lazily on first use.

Because ct is added to devres after the sysfs cookie, the LIFO nature of
devres_release_all() guarantees ct is freed before the sysfs file is removed.

Could a concurrent sysfs read fetch the ct pointer, get preempted, and attempt
to lock ct->lock after devres has already freed it, leading to a
use-after-free during device removal?

> +
> +	/* No complete data yet or offset past end */
> +	if (!ct->data || ct->receiving || ct->copied != ct->size ||
> +	    offset >= ct->size) {
> +		mutex_unlock(&ct->lock);
> +		return 0;
> +	}
> +
> +	available = ct->size - offset;
> +	count = min(count, available);
> +	memcpy(buf, (u8 *)ct->data + offset, count);
> +
> +	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]
The sahara driver can be built as a loadable module. By attaching the
ddr_training_data sysfs file and devres cleanup cookie to the parent
controller device (mhi_cntrl->mhi_dev->dev), they outlive the sahara client
device.

When the sahara module is unloaded, the sysfs file and devres objects are
not detached. Once the module is unmapped, userspace reading the stale file
or the controller device being removed will cause the kernel to jump to the
freed ddr_training_data_read or sahara_sysfs_devres_release functions.

Does attaching the sysfs node and devres callback to the parent MHI
controller cause a use-after-free on module unload?

> +}

[ ... ]

I also noticed a few other issues in related Sahara functions that were
introduced in this series:

[Severity: Medium]
In sahara_cntrl_training_get(), we have a check-then-act sequence:

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);
	return ct;
}

Since ddr_training_data_read() can be invoked concurrently by multiple
userspace threads, multiple threads could see devres_find() return NULL
and allocate separate ct structures.

Does this permanently leak memory and break synchronization since different
threads would use different mutexes?

[Severity: High]
In sahara_mhi_dl_xfer_cb(), the MHI transfer callback executes in the MHI
event tasklet, which is an atomic context:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_mhi_dl_xfer_cb() {
    ...
		ct = sahara_cntrl_training_get(dev);
		if (!ct)
			return;

		for (i = 0; i < context->training_nbuf; i++) {
			if (mhi_result->buf_addr == context->cmd_buff[i]) {
				mutex_lock(&ct->lock);
    ...
}

Will unconditionally invoking mutex_lock() here result in a sleep-in-atomic bug?

[Severity: Low]
In sahara_command_execute_resp(), there is a negative length validation:

drivers/bus/mhi/host/clients/sahara/sahara.c:sahara_command_execute_resp() {
    ...
	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_EXEC_RESP_LENGTH ||
	    le32_to_cpu(context->rx->command_execute_resp.response_length) < 0) {
    ...
}

Since le32_to_cpu() returns an unsigned 32-bit integer, checking if
response_length is < 0 will always be false.

While an unexpectedly large length would gracefully fail the subsequent
memory allocation, should this check be corrected to prevent bypassing the
validation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-sahara_protocol_new_v2-v9-0-dc9f24357c4a@oss.qualcomm.com?part=7