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