Re: [PATCH 1/2] scsi: 3w-9xxx: sanitize passthrough SGLs

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Failure in `copy_to_user()` is silently ignored for `TW_IOCTL_FIRMWARE_PASS_THROUGH` commands because `retval` is prematurely set to 0.

Pre-existing issues:
- [High] Missing minimum `buffer_length` validation allows an out-of-bounds write of up to 112 bytes on the `dma_alloc_coherent` heap for non-passthrough ioctls.
- [High] A character device ioctl Use-After-Free exists if a controller is removed (hot-unplugged) while a file descriptor remains open.
- [High] The exact same SGL injection and command size vulnerabilities exist in the sibling driver 3w-sas, which is not addressed by this patch series.
--

commit 511c1e3b0d7a73d0dd03209ebbb80ee6964280e4
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>
scsi: 3w-9xxx: sanitize passthrough SGLs

This patch validates legacy SGL placement, enforces command sizing for
single buffers, and clears SGL arrays before copying user requests to
prevent exposing stale coherent memory.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 9b93a2440af8f..22f5fa1e473f6 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
[ ... ]
> @@ -686,6 +688,8 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  	}
>  
>  	tw_ioctl = (TW_Ioctl_Buf_Apache *)cpu_addr;
> +	memset(tw_ioctl, 0, sizeof(TW_Ioctl_Buf_Apache) +
> +	       data_buffer_length_adjusted);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free if the device
is hot-unplugged while the character device file descriptor is open?

In twa_remove(), the device extension is freed, but the stale pointer is not
cleared from the global twa_device_extension_list array:

drivers/scsi/3w-9xxx.c:twa_remove()
   twa_free_device_extension(tw_dev);
   scsi_host_put(tw_dev->host);
   ...
   twa_device_extension_count--;
   
Then in twa_chrdev_ioctl(), the driver fetches the pointer directly from the
array and dereferences it without verifying its lifetime:

drivers/scsi/3w-9xxx.c:twa_chrdev_ioctl()
   TW_Device_Extension *tw_dev = twa_device_extension_list[iminor(inode)];
   ...
   if (mutex_lock_interruptible(&tw_dev->ioctl_lock)) {
   
Could this lead to memory corruption if a user calls ioctl() during removal?

[Severity: High]
This is also a pre-existing issue, but does this allocation lack minimum
buffer_length validation, leading to an out-of-bounds write?

If an event-retrieval ioctl (like TW_IOCTL_GET_COMPATIBILITY_INFO) is called
with buffer_length = 0, data_buffer_length_adjusted will be 0, and
dma_alloc_coherent allocates exactly sizeof(TW_Ioctl_Buf_Apache) bytes.

However, the driver later blindly copies up to 112 bytes into the flexible
array:

drivers/scsi/3w-9xxx.c:twa_chrdev_ioctl()
   memcpy(tw_ioctl->data_buffer, tw_dev->event_queue[event_index], sizeof(TW_Event));
   
Since tw_ioctl->data_buffer sits outside the allocated size when
buffer_length is 0, will this corrupt the DMA heap?

>  
>  	/* Now copy down the entire ioctl */
>  	if (copy_from_user(tw_ioctl, argp, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length))
> @@ -706,7 +710,14 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  		full_command_packet = &tw_ioctl->firmware_command;
>  
>  		/* Load request id and sglist for both command types */
> -		twa_load_sgl(tw_dev, full_command_packet, request_id, dma_handle, data_buffer_length_adjusted);
> +		retval = twa_load_sgl(tw_dev, full_command_packet, request_id,
> +				      dma_handle, data_buffer_length_adjusted);

[Severity: Medium]
Does this assignment cause copy_to_user() failures to be silently ignored?

Because twa_load_sgl() returns 0 on success, retval is overwritten to 0 here.
Later in twa_chrdev_ioctl(), the copy_to_user() check looks like this:

drivers/scsi/3w-9xxx.c:twa_chrdev_ioctl()
   if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length) == 0)
       retval = 0;
       
If copy_to_user() fails, the condition evaluates to false, skipping the
assignment. But since retval is already 0, the error is lost and 0 is
returned to userspace instead of -EFAULT. Will this cause userspace to
process uninitialized or stale data as valid firmware output?

> +		if (retval) {
> +			tw_dev->chrdev_request_id = TW_IOCTL_CHRDEV_FREE;
> +			twa_free_request_id(tw_dev, request_id);
> +			spin_unlock_irqrestore(tw_dev->host->host_lock, flags);
> +			goto out3;
> +		}
[ ... ]

[Severity: High]
This is a pre-existing issue and outside the scope of this file, but does the
exact same SGL injection and command size vulnerability exist in the sibling
driver 3w-sas?

Looking at twl_load_sgl() in drivers/scsi/3w-sas.c, the SGL offset appears
completely controlled by the user, and trailing SGL elements are not zeroed
out:

drivers/scsi/3w-sas.c:twl_load_sgl()
   sgl = (TW_SG_Entry_ISO *)((u32 *)newcommand + newcommand->sgl_offset);
   sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
   sgl->length = cpu_to_le32(length);
   
Since it lacks the bounds checking added here for 3w-9xxx, can a user
with CAP_SYS_ADMIN pass arbitrary physical DMA descriptors to the firmware?

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