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 <[email protected]> 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-3a78362c4...@oss.qualcomm.com?part=7
