Re: [PATCH] scsi: ch: publish changer devices after probe setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Spurious udev open failures (Regression) due to deferred idr_replace().
- [Medium] Memory leak of the dynamically allocated `ch->dt` array in the `idr_replace()` error path.

Pre-existing issues:
- [Critical] Race condition between `ch_open()` and `ch_remove()` leads to a Use-After-Free of the `scsi_device` structure.
- [High] Information leak of uninitialized kernel heap memory in the `CHIOGELEM` ioctl handler.
--

commit 63eb67253a3c4a6ab60f598a1642027bbd386913
Author: Ruoyu Wang <ruoyuw560@gmail.com>

scsi: ch: publish changer devices after probe setup

ch_probe() inserts the new changer into ch_index_idr before initializing
the kref, lock and scsi_device pointer. ch_open() looks up the object
directly from the IDR by minor, so a racing open can observe a partially
initialized changer.

Reserve the minor with a NULL IDR entry, finish device setup and element
discovery, then publish the initialized changer with idr_replace().
Early opens continue to fail with -ENXIO until the object is ready.

> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index 4010fdbf813cc..12061e4681ace 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c

[ ... ]

> @@ -951,6 +952,15 @@ static int ch_probe(struct scsi_device *sd)
>  		ch_init_elem(ch);
>  
>  	mutex_unlock(&ch->lock);
> +
> +	spin_lock(&ch_index_lock);
> +	old = idr_replace(&ch_index_idr, ch, ch->minor);
> +	spin_unlock(&ch_index_lock);

[Severity: High]
Does this change introduce spurious udev open failures?

Looking at ch_probe(), device_create() is called earlier which emits a
KOBJ_ADD uevent, prompting udev to immediately create the device node and
attempt to open it.

Because idr_replace() is now deferred until here, ch_open() will observe a
NULL IDR entry during the udev open attempt and return -ENXIO. Since udev
rules do not automatically retry on -ENXIO, won't this permanently fail
userspace device initialization, whereas previously it would gracefully
block on mutex_lock(&ch->lock)?

> +	if (IS_ERR(old)) {
> +		ret = PTR_ERR(old);
> +		goto destroy_dev;

[Severity: Medium]
Does this error path leak the ch->dt array?

If idr_replace() fails, we jump to destroy_dev and eventually to free_ch
where kfree(ch) is called directly. Since ch_readconfig(ch) was
successfully executed earlier in this function, it allocates the ch->dt
array. Does bypassing ch_destroy() leave the ch->dt array permanently
leaked?

> +	}
> +
>  	dev_set_drvdata(dev, ch);
>  	sdev_printk(KERN_INFO, sd, "Attached scsi changer %s\n", ch->name);

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but can a race condition between ch_open()
and ch_remove() lead to a use-after-free of the scsi_device structure?

In ch_open(), after the ch structure is found via the IDR and its kref is
incremented, the ch_index_lock is released:

ch_open() {
    ...
    ch = idr_find(&ch_index_idr, minor);
    if (ch == NULL || !kref_get_unless_zero(&ch->ref)) {
        spin_unlock(&ch_index_lock);
        return -ENXIO;
    }
    spin_unlock(&ch_index_lock);
    if (scsi_device_get(ch->device)) {
    ...
}

Concurrently, if a device removal occurs, ch_remove() drops the driver's
reference to the scsi_device:

ch_remove() {
    ...
    scsi_device_put(ch->device);
    kref_put(&ch->ref, ch_destroy);
    ...
}

If this drops the final reference to the scsi_device, it can be freed
before the preempted ch_open() resumes and calls
scsi_device_get(ch->device). Could this be triggered by userspace
repeatedly opening the device while it is being hot-unplugged?

[Severity: High]
This is a pre-existing issue, but does the CHIOGELEM ioctl leak
uninitialized kernel heap memory to userspace?

The CHIOGELEM ioctl allocates a buffer using kmalloc() without the
__GFP_ZERO flag:

ch_ioctl() {
    ...
    buffer = kmalloc(512, GFP_KERNEL);
    ...
    result = ch_do_scsi(ch, ch_cmd, 12, buffer, 256, REQ_OP_DRV_IN);
    ...
    memcpy(cge.cge_pvoltag,buffer+28,36);
    ...
    if (copy_to_user(argp, &cge, sizeof (cge)))
    ...
}

If the SCSI device returns a short read, the trailing bytes of the buffer
retain uninitialized kernel heap memory. The driver then copies from this
buffer into the cge structure and calls copy_to_user(). Can this leak up
to 72 bytes of potentially sensitive uninitialized memory to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617182122.955546-1-ruoyuw560@gmail.com?part=1