* Re: [PATCH] scsi: pmcraid: check device_create() return value
2026-08-14 5:45 [PATCH] scsi: pmcraid: check device_create() return value Linkai Gong
@ 2026-08-14 5:58 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-14 5:58 UTC (permalink / raw)
To: Linkai Gong; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Medium] The new error path for `device_create()` failure introduces a Use-After-Free because `cdev_del()` does not revoke active file descriptors.
Pre-existing issues:
- [High] The error path in `pmcraid_probe` frees adapter buffers before disabling hardware or unregistering the interrupt handler, leading to ISR UAF.
- [High] Missing `cancel_work_sync()` on probe error path causes workqueue Use-After-Free.
- [Medium] Missing `scsi_host` refcounting in character device file operations leads to a Use-After-Free upon device removal.
- [Medium] Data race on the `pmcraid_minor` global bitmap during concurrent PCI probing.
- [Medium] A time-of-check to time-of-use (TOCTOU) race condition and missing bounds check allow exceeding `PMCRAID_MAX_ADAPTERS`, leading to an out-of-bounds write.
--
--- Patch [1]: [PATCH] scsi: pmcraid: check device_create() return value ---
commit b3e81f4a9bfa636816d6ecf5e77c7bcf9c823cca
Author: Linkai Gong <gonglinkai@kylinos.cn>
scsi: pmcraid: check device_create() return value
This commit adds a check for the return value of device_create() in
pmcraid_setup_chrdev(). If it fails, the driver now cleans up by deleting
the cdev and releasing the assigned minor number.
> diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
> index 942a993932049..65a87d30e65fe 100644
> --- a/drivers/scsi/pmcraid.c
> +++ b/drivers/scsi/pmcraid.c
> @@ -4721,12 +4721,21 @@ static int pmcraid_setup_chrdev(struct pmcraid_instance *pinstance)
> pinstance->cdev.owner = THIS_MODULE;
>
> error = cdev_add(&pinstance->cdev, MKDEV(pmcraid_major, minor), 1);
> + if (error) {
> + pmcraid_release_minor(minor);
> + return error;
> + }
>
> - if (error)
> + error = PTR_ERR_OR_ZERO(device_create(&pmcraid_class, NULL,
> + MKDEV(pmcraid_major, minor), NULL,
> + "%s%u", PMCRAID_DEVFILE, minor));
> + if (error) {
> + pmcraid_err("failed to create device file for minor %d, error %d\n",
> + minor, error);
> + cdev_del(&pinstance->cdev);
[Severity: Medium]
If a user opens the device right after cdev_add() but before device_create()
fails, does cdev_del() revoke the active file descriptor?
When the probe fails here, it returns an error and eventually calls
scsi_host_put(), freeing pinstance. If the user still holds an open file
descriptor, would any subsequent ioctl() or close() lead to a use-after-free
of the pinstance structure?
> pmcraid_release_minor(minor);
> - else
> - device_create(&pmcraid_class, NULL, MKDEV(pmcraid_major, minor),
> - NULL, "%s%u", PMCRAID_DEVFILE, minor);
> + }
> +
> return error;
> }
[Severity: Medium]
This is a pre-existing issue, but is there missing scsi_host refcounting in
the character device file operations?
drivers/scsi/pmcraid.c:pmcraid_chr_open() {
pinstance = container_of(inode->i_cdev, struct pmcraid_instance, cdev);
filep->private_data = pinstance;
}
Since pmcraid_fops lacks a .release callback to properly balance refcounting
with scsi_host_put(), could this lead to a use-after-free? If an admin
keeps the device open during a PCI hot-remove, pmcraid_remove() calls
scsi_host_put() and frees pinstance. Does this leave the open file descriptor
pointing to freed memory?
[Severity: High]
This is a pre-existing issue, but does the error path in pmcraid_probe() free
adapter buffers before disabling hardware or unregistering the interrupt
handler?
drivers/scsi/pmcraid.c:pmcraid_probe() {
...
out_release_bufs:
pmcraid_release_buffers(pinstance);
out_unregister_isr:
pmcraid_kill_tasklets(pinstance);
pmcraid_unregister_interrupt_handler(pinstance);
}
If pmcraid_probe() fails after pmcraid_reset_bringup() and jumps to
out_release_bufs, the hardware is still fully active while its DMA memory
buffers are freed. If the adapter sends an interrupt (like a Configuration
Change Notification) during this window, can the ISR access freed host
resource request queues and cause a use-after-free?
[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path miss
cancelling the worker queue?
drivers/scsi/pmcraid.c:pmcraid_probe() {
out_remove_host:
scsi_remove_host(host);
out_release_bufs:
pmcraid_release_buffers(pinstance);
}
If a hardware event schedules pinstance->worker_q before a probe failure, the
error teardown frees pinstance without calling cancel_work_sync(). Will the
system workqueue later attempt to execute the callback on freed memory?
[Severity: Medium]
This is a pre-existing issue, but is there a data race on the pmcraid_minor
global bitmap during concurrent PCI probing?
drivers/scsi/pmcraid.c:pmcraid_get_minor() {
minor = find_first_zero_bit(pmcraid_minor, PMCRAID_MAX_ADAPTERS);
__set_bit(minor, pmcraid_minor);
}
Since __set_bit() is non-atomic and there is no lock protection here, could
two threads probing multiple adapters concurrently read the same available bit
and claim the same minor number?
[Severity: Medium]
This is a pre-existing issue, but is there a race condition that can lead to
an out-of-bounds write?
drivers/scsi/pmcraid.c:pmcraid_probe() {
if (atomic_read(&pmcraid_adapter_count) >= PMCRAID_MAX_ADAPTERS) {
...
atomic_inc(&pmcraid_adapter_count);
}
The adapter count check uses atomic_read() followed separately by atomic_inc().
If concurrent probes interleave and bypass this limit, find_first_zero_bit()
in pmcraid_get_minor() will return PMCRAID_MAX_ADAPTERS (1024) since all bits
are set.
Because there is no bounds check before calling __set_bit(), does this result
in writing beyond the end of the pmcraid_minor bitmap array?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260814054513.2519483-1-gonglinkai@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread