Re: [PATCH] scsi: ch: Remove the minor from ch_index_idr after device_destroy()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] scsi: ch: use-after-free in ch_probe() [1]
--

commit 170bbda8f45c76e2fbb5ceaccd99874da50253d8
Author: Changyul Lee <lcy8047@gmail.com>

scsi: ch: Remove the minor from ch_index_idr after device_destroy()

This commit replaces the minor's entry in ch_index_idr with NULL before
calling device_destroy() in ch_remove() and removes the minor from the
IDR only afterward so a concurrent probe cannot reuse the minor while the
sysfs device still exists.

> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index b2c9fb71c9e0a..390425671a3a7 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -952,11 +952,15 @@ static void ch_remove(struct scsi_device *sd)
>  	scsi_changer *ch = dev_get_drvdata(dev);
>  
>  	spin_lock(&ch_index_lock);
> -	idr_remove(&ch_index_idr, ch->minor);
> +	idr_replace(&ch_index_idr, NULL, ch->minor);
>  	dev_set_drvdata(dev, NULL);
>  	spin_unlock(&ch_index_lock);
>  
>  	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
> +
> +	spin_lock(&ch_index_lock);
> +	idr_remove(&ch_index_idr, ch->minor);
> +	spin_unlock(&ch_index_lock);
>  	scsi_device_put(ch->device);
>  	kref_put(&ch->ref, ch_destroy);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but can ch_probe() trigger
a use-after-free on ch [1] if an error occurs after ch is published
in ch_index_idr?

ch_probe() inserts ch directly into ch_index_idr and initializes ch->ref
before calling device_create() and ch_readconfig(), and its error path
frees ch with kfree(ch) instead of kref_put():

drivers/scsi/ch.c:ch_probe() {
	...
	ret = idr_alloc(&ch_index_idr, ch, 0, CH_MAX_DEVS + 1, GFP_NOWAIT);
	...
	kref_init(&ch->ref);
	...
	ret = ch_readconfig(ch);
	if (ret) {
		mutex_unlock(&ch->lock);
		goto destroy_dev;
	}
	...
destroy_dev:
	device_destroy(&ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor));
put_device:
	scsi_device_put(sd);
remove_idr:
	idr_remove(&ch_index_idr, ch->minor);
free_ch:
	kfree(ch);
	return ret;
}

If ch_readconfig() fails after a concurrent ch_open() looks up ch in
ch_index_idr and takes a reference, ch_open() dereferences freed memory:

CPU0                              CPU1
ch_probe()
  idr_alloc(&ch_index_idr, ch, ...)
  kref_init(&ch->ref)
                                  ch_open()
                                    ch = idr_find(&ch_index_idr, minor)
                                    kref_get(&ch->ref)
  ch_readconfig(ch) /* fails */
  idr_remove(&ch_index_idr, ch->minor)
  kfree(ch)
                                    mutex_lock(&ch->lock)  <-- use-after-free

[1] https://sashiko.dev/#/bug/linux-f4be7a8e-e1b8-4f47-9f47-c6a01d983399

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