Re: [PATCH] scsi: pmcraid: check device_create() return value

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 <[email protected]>

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/[email protected]?part=1