Re: [PATCH v6 19/40] scsi: 3w-xxxx: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A character device ioctl use-after-free exists due to unbalanced lifecycle operations during PCI removal and lack of per-device reference counting.
--

commit 97249c8ac47fc8b50b4db175724db5a0b515a3d1
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: 3w-xxxx: use 16-bits defined sense codes
    
Modify the function tw_scsi_queue_lck() of the 3w-xxxx driver to use
scsi_set_sense() and replace the hard-coded additional sense codes and
additional sense code qualifiers with the enum values defined in
include/scsi/scsi_sense.h. This helps with code clarity as the sense codes
being processed are easier to test and self-documented.

No functional change intended.

> diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
> index c68678fa72c1e..1e1692f776d31 100644
> --- a/drivers/scsi/3w-xxxx.c
> +++ b/drivers/scsi/3w-xxxx.c

[ ... ]

> @@ -1977,7 +1977,8 @@ static enum scsi_qc_status tw_scsi_queue_lck(struct scsi_cmnd *SCpnt)
>  		printk(KERN_NOTICE "3w-xxxx: scsi%d: Unknown scsi opcode: 0x%x\n", tw_dev->host->host_no, *command);
>  		tw_dev->state[request_id] = TW_S_COMPLETED;
>  		tw_state_request_finish(tw_dev, request_id);
> -		scsi_build_sense(SCpnt, 1, ILLEGAL_REQUEST, 0x20, 0);
> +		scsi_set_sense(SCpnt, 1, ILLEGAL_REQUEST,
> +			       INVALID_COMMAND_OP_CODE);
>  		done(SCpnt);
>  		retval = 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does this driver have a use-after-free
during device removal?

Looking at tw_remove(), when a device is removed, it drops the host refcount
and frees the device extension, but doesn't clear it from the global array:

drivers/scsi/3w-xxxx.c:tw_remove() {
	...
	if (twe_major >= 0) {
		unregister_chrdev(twe_major, "twe");
		twe_major = -1;
	}
	...
	scsi_host_put(tw_dev->host);
	pci_disable_device(pdev);
	tw_device_extension_count--;
}

If a user with CAP_SYS_ADMIN already has the character device open, can they
trigger a use-after-free by calling an ioctl after removal?

drivers/scsi/3w-xxxx.c:tw_chrdev_ioctl() {
	...
	TW_Device_Extension *tw_dev = tw_device_extension_list[iminor(inode)];
	...
	mutex_lock(&tw_mutex);
	/* Only let one of these through at a time */
	if (mutex_lock_interruptible(&tw_dev->ioctl_lock)) {
	...
}

Will this retrieve the freed tw_dev from tw_device_extension_list and
dereference it? Also, does unregister_chrdev() in tw_remove() break the
character device interface for all other remaining controllers by
unconditionally removing the shared major number?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090308.1085097-1-dlemoal@kernel.org?part=19