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

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

[Yousef Alhouseen]

TW_IOCTL_FIRMWARE_PASS_THROUGH copies a full command packet from userspace
and then overwrites the first SGL entry. The SGL location and command size
remain user-controlled for legacy commands, and any additional
firmware-visible SGL entries can survive in the packet.

Validate the legacy SGL placement before writing it, force the command
size to describe only the single driver-owned data buffer, and clear the
SGL arrays before filling entry zero. Also zero the DMA bounce buffer
before copying the user request so short device writes cannot expose stale
coherent memory on copyout.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/scsi/3w-9xxx.c | 54 ++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index a125801e3..2f399a267 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -137,7 +137,9 @@ static int twa_initconnection(TW_Device_Extension *tw_dev, int message_credits,
 			      unsigned short *fw_on_ctlr_branch,
 			      unsigned short *fw_on_ctlr_build,
 			      u32 *init_connect_result);
-static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_command_packet, int request_id, dma_addr_t dma_handle, int length);
+static int twa_load_sgl(TW_Device_Extension *tw_dev,
+			TW_Command_Full *full_command_packet, int request_id,
+			dma_addr_t dma_handle, int length);
 static int twa_poll_response(TW_Device_Extension *tw_dev, int request_id, int seconds);
 static int twa_poll_status_gone(TW_Device_Extension *tw_dev, u32 flag, int seconds);
 static int twa_post_command_packet(TW_Device_Extension *tw_dev, int request_id, char internal);
@@ -707,6 +709,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);
 
 	/* Now copy down the entire ioctl */
 	if (copy_from_user(tw_ioctl, argp, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length))
@@ -727,7 +731,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);
+		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;
+		}
 
 		memcpy(tw_dev->command_packet_virt[request_id], &(tw_ioctl->firmware_command), sizeof(TW_Command_Full));
 
@@ -1398,11 +1409,14 @@ static irqreturn_t twa_interrupt(int irq, void *dev_instance)
 } /* End twa_interrupt() */
 
 /* This function will load the request id and various sgls for ioctls */
-static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_command_packet, int request_id, dma_addr_t dma_handle, int length)
+static int twa_load_sgl(TW_Device_Extension *tw_dev,
+			TW_Command_Full *full_command_packet, int request_id,
+			dma_addr_t dma_handle, int length)
 {
 	TW_Command *oldcommand;
 	TW_Command_Apache *newcommand;
 	TW_SG_Entry *sgl;
+	unsigned int sgl_offset, sgl_words, max_words;
 	unsigned int pae = 0;
 
 	if ((sizeof(long) < 8) && (sizeof(dma_addr_t) > 4))
@@ -1412,6 +1426,8 @@ static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 		newcommand = &full_command_packet->command.newcommand;
 		newcommand->request_id__lunl =
 			TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->request_id__lunl), request_id);
+		newcommand->sgl_offset = 16;
+		memset(newcommand->sg_list, 0, sizeof(newcommand->sg_list));
 		if (length) {
 			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
 			newcommand->sg_list[0].length = cpu_to_le32(length);
@@ -1421,19 +1437,31 @@ static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 	} else {
 		oldcommand = &full_command_packet->command.oldcommand;
 		oldcommand->request_id = request_id;
+		sgl_offset = TW_SGL_OUT(oldcommand->opcode__sgloffset);
+		if (!sgl_offset)
+			return length ? -EINVAL : 0;
+
+		sgl_words = sizeof(*sgl) / sizeof(u32);
+		max_words = sizeof(*oldcommand) / sizeof(u32);
+
+		if (tw_dev->tw_pci_dev->device == PCI_DEVICE_ID_3WARE_9690SA) {
+			if (oldcommand->size < sgl_words - pae)
+				return -EINVAL;
+			if (oldcommand->size - sgl_words + pae != sgl_offset)
+				return -EINVAL;
+		}
 
-		if (TW_SGL_OUT(oldcommand->opcode__sgloffset)) {
-			/* Load the sg list */
-			if (tw_dev->tw_pci_dev->device == PCI_DEVICE_ID_3WARE_9690SA)
-				sgl = (TW_SG_Entry *)((u32 *)oldcommand+oldcommand->size - (sizeof(TW_SG_Entry)/4) + pae);
-			else
-				sgl = (TW_SG_Entry *)((u32 *)oldcommand+TW_SGL_OUT(oldcommand->opcode__sgloffset));
-			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
-			sgl->length = cpu_to_le32(length);
+		if (sgl_offset > max_words || sgl_words > max_words - sgl_offset)
+			return -EINVAL;
 
-			oldcommand->size += pae;
-		}
+		sgl = (TW_SG_Entry *)((u32 *)oldcommand + sgl_offset);
+		memset(sgl, 0, sizeof(*oldcommand) - sgl_offset * sizeof(u32));
+		sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
+		sgl->length = cpu_to_le32(length);
+		oldcommand->size = sgl_offset + sgl_words;
 	}
+
+	return 0;
 } /* End twa_load_sgl() */
 
 /* This function will poll for a response interrupt of a request */
-- 
2.54.0

[PATCH 2/2] scsi: 3w-xxxx: sanitize passthrough SGLs

[Yousef Alhouseen]

TW_CMD_PACKET_WITH_DATA accepts a command packet from userspace and
patches only the first SGL entry before posting it to the controller. The
command size and remaining SGL contents can still describe user-controlled
DMA descriptors to firmware.

Reject unknown SGL offsets, clear the relevant SGL array, and force the
command size to the single driver-owned DMA buffer. Zero the coherent
ioctl buffer before copying the request so short device writes do not leak
stale memory back to userspace.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/scsi/3w-xxxx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
index 147a47e6b..033f79eaa 100644
--- a/drivers/scsi/3w-xxxx.c
+++ b/drivers/scsi/3w-xxxx.c
@@ -925,6 +925,7 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
 	}
 
 	tw_ioctl = (TW_New_Ioctl *)cpu_addr;
+	memset(tw_ioctl, 0, data_buffer_length_adjusted + sizeof(TW_New_Ioctl));
 
 	/* Now copy down the entire ioctl */
 	if (copy_from_user(tw_ioctl, argp, data_buffer_length + sizeof(TW_New_Ioctl)))
@@ -972,17 +973,31 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
 			/* Load the sg list */
 			switch (TW_SGL_OUT(tw_ioctl->firmware_command.opcode__sgloffset)) {
 			case 2:
+				memset(tw_ioctl->firmware_command.byte8.param.sgl, 0,
+				       sizeof(tw_ioctl->firmware_command.byte8.param.sgl));
+				tw_ioctl->firmware_command.size = 4;
 				tw_ioctl->firmware_command.byte8.param.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				tw_ioctl->firmware_command.byte8.param.sgl[0].length = data_buffer_length_adjusted;
 				break;
 			case 3:
+				memset(tw_ioctl->firmware_command.byte8.io.sgl, 0,
+				       sizeof(tw_ioctl->firmware_command.byte8.io.sgl));
+				tw_ioctl->firmware_command.size = 5;
 				tw_ioctl->firmware_command.byte8.io.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				tw_ioctl->firmware_command.byte8.io.sgl[0].length = data_buffer_length_adjusted;
 				break;
 			case 5:
+				memset(passthru->sg_list, 0, sizeof(passthru->sg_list));
+				passthru->size = 7;
 				passthru->sg_list[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				passthru->sg_list[0].length = data_buffer_length_adjusted;
 				break;
+			default:
+				retval = -EINVAL;
+				tw_dev->chrdev_request_id = TW_IOCTL_CHRDEV_FREE;
+				tw_state_request_finish(tw_dev, request_id);
+				spin_unlock_irqrestore(tw_dev->host->host_lock, flags);
+				goto out2;
 			}
 
 			memcpy(tw_dev->command_packet_virtual_address[request_id], &(tw_ioctl->firmware_command), sizeof(TW_Command));
-- 
2.54.0

Re: [PATCH 2/2] scsi: 3w-xxxx: sanitize passthrough SGLs

[sashiko-bot]

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

Pre-existing issues:
- [High] The DMA buffer is freed while the hardware may still be actively writing to it on timeout failure paths.
--

commit 046079df1a1f193432bc739db902b2c478f12b8c
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

scsi: 3w-xxxx: sanitize passthrough SGLs

TW_CMD_PACKET_WITH_DATA accepts a command packet from userspace and
patches only the first SGL entry before posting it to the controller. The
command size and remaining SGL contents can still describe user-controlled
DMA descriptors to firmware.

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

[ ... ]

> @@ -966,17 +967,31 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
>  			/* Load the sg list */
>  			switch (TW_SGL_OUT(tw_ioctl->firmware_command.opcode__sgloffset)) {
>  			case 2:
> +				memset(tw_ioctl->firmware_command.byte8.param.sgl, 0,
> +				       sizeof(tw_ioctl->firmware_command.byte8.param.sgl));
> +				tw_ioctl->firmware_command.size = 4;
>  				tw_ioctl->firmware_command.byte8.param.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
>  				tw_ioctl->firmware_command.byte8.param.sgl[0].length = data_buffer_length_adjusted;
>  				break;
>  			case 3:
> +				memset(tw_ioctl->firmware_command.byte8.io.sgl, 0,
> +				       sizeof(tw_ioctl->firmware_command.byte8.io.sgl));
> +				tw_ioctl->firmware_command.size = 5;
>  				tw_ioctl->firmware_command.byte8.io.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
>  				tw_ioctl->firmware_command.byte8.io.sgl[0].length = data_buffer_length_adjusted;
>  				break;
>  			case 5:
> +				memset(passthru->sg_list, 0, sizeof(passthru->sg_list));
> +				passthru->size = 7;
>  				passthru->sg_list[0].address = dma_handle + sizeof(TW_New_Ioctl);
>  				passthru->sg_list[0].length = data_buffer_length_adjusted;
>  				break;
> +			default:
> +				retval = -EINVAL;
> +				tw_dev->chrdev_request_id = TW_IOCTL_CHRDEV_FREE;
> +				tw_state_request_finish(tw_dev, request_id);
> +				spin_unlock_irqrestore(tw_dev->host->host_lock, flags);
> +				goto out2;
>  			}
>  
>  			memcpy(tw_dev->command_packet_virtual_address[request_id], &(tw_ioctl->firmware_command), sizeof(TW_Command));

[Severity: High]
This is a pre-existing issue, but does tw_chrdev_ioctl() have a use-after-free
risk when handling command timeouts?

If wait_event_timeout() times out and tw_reset_device_extension() fails, the
driver jumps to out2 and calls dma_free_coherent() on the ioctl buffer:

tw_chrdev_ioctl() {
    ...
    timeout = wait_event_timeout(tw_dev->ioctl_wqueue,
                                 tw_dev->chrdev_request_id == TW_IOCTL_CHRDEV_FREE,
                                 timeout);

    if (tw_dev->chrdev_request_id != TW_IOCTL_CHRDEV_FREE) {
        retval = -EIO;
        if (tw_reset_device_extension(tw_dev)) {
            goto out2;
        }
    }
    ...
out2:
    dma_free_coherent(..., cpu_addr, dma_handle);
}

Since the hardware was not successfully reset, couldn't the controller still
be actively performing DMA writes to this freed page after it is returned to
the page allocator?

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

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

[sashiko-bot]

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

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

[Yousef Alhouseen]

TW_IOCTL_FIRMWARE_PASS_THROUGH copies a full command packet from userspace
and then overwrites the first SGL entry. The SGL location and command
size remain user-controlled for legacy commands, and any additional
firmware-visible SGL entries can survive in the packet.

Validate the legacy SGL placement before writing it, force the command
size to describe only the single driver-owned data buffer, and clear the
SGL arrays before filling entry zero. Also zero the DMA bounce buffer
before copying the user request so short device writes cannot expose
stale coherent memory on copyout.

Since a successful SGL setup leaves retval at zero, reset retval to the
copyout error before the final copy_to_user() so a failed copyout cannot
be reported as success.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
Changes in v2:
- Preserve -EFAULT if the final copy_to_user() fails after successful SGL
  setup.

 drivers/scsi/3w-9xxx.c | 55 ++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index a125801e3..1e48d6280 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -137,7 +137,9 @@ static int twa_initconnection(TW_Device_Extension *tw_dev, int message_credits,
 			      unsigned short *fw_on_ctlr_branch,
 			      unsigned short *fw_on_ctlr_build,
 			      u32 *init_connect_result);
-static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_command_packet, int request_id, dma_addr_t dma_handle, int length);
+static int twa_load_sgl(TW_Device_Extension *tw_dev,
+			TW_Command_Full *full_command_packet, int request_id,
+			dma_addr_t dma_handle, int length);
 static int twa_poll_response(TW_Device_Extension *tw_dev, int request_id, int seconds);
 static int twa_poll_status_gone(TW_Device_Extension *tw_dev, u32 flag, int seconds);
 static int twa_post_command_packet(TW_Device_Extension *tw_dev, int request_id, char internal);
@@ -707,6 +709,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);
 
 	/* Now copy down the entire ioctl */
 	if (copy_from_user(tw_ioctl, argp, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length))
@@ -727,7 +731,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);
+		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;
+		}
 
 		memcpy(tw_dev->command_packet_virt[request_id], &(tw_ioctl->firmware_command), sizeof(TW_Command_Full));
 
@@ -891,6 +902,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	}
 
 	/* Now copy the entire response to userspace */
+	retval = TW_IOCTL_ERROR_OS_EFAULT;
 	if (copy_to_user(argp, tw_ioctl, sizeof(TW_Ioctl_Buf_Apache) + driver_command.buffer_length) == 0)
 		retval = 0;
 out3:
@@ -1398,11 +1410,14 @@ static irqreturn_t twa_interrupt(int irq, void *dev_instance)
 } /* End twa_interrupt() */
 
 /* This function will load the request id and various sgls for ioctls */
-static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_command_packet, int request_id, dma_addr_t dma_handle, int length)
+static int twa_load_sgl(TW_Device_Extension *tw_dev,
+			TW_Command_Full *full_command_packet, int request_id,
+			dma_addr_t dma_handle, int length)
 {
 	TW_Command *oldcommand;
 	TW_Command_Apache *newcommand;
 	TW_SG_Entry *sgl;
+	unsigned int sgl_offset, sgl_words, max_words;
 	unsigned int pae = 0;
 
 	if ((sizeof(long) < 8) && (sizeof(dma_addr_t) > 4))
@@ -1412,6 +1427,8 @@ static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 		newcommand = &full_command_packet->command.newcommand;
 		newcommand->request_id__lunl =
 			TW_REQ_LUN_IN(TW_LUN_OUT(newcommand->request_id__lunl), request_id);
+		newcommand->sgl_offset = 16;
+		memset(newcommand->sg_list, 0, sizeof(newcommand->sg_list));
 		if (length) {
 			newcommand->sg_list[0].address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
 			newcommand->sg_list[0].length = cpu_to_le32(length);
@@ -1421,19 +1438,31 @@ static void twa_load_sgl(TW_Device_Extension *tw_dev, TW_Command_Full *full_comm
 	} else {
 		oldcommand = &full_command_packet->command.oldcommand;
 		oldcommand->request_id = request_id;
+		sgl_offset = TW_SGL_OUT(oldcommand->opcode__sgloffset);
+		if (!sgl_offset)
+			return length ? -EINVAL : 0;
+
+		sgl_words = sizeof(*sgl) / sizeof(u32);
+		max_words = sizeof(*oldcommand) / sizeof(u32);
+
+		if (tw_dev->tw_pci_dev->device == PCI_DEVICE_ID_3WARE_9690SA) {
+			if (oldcommand->size < sgl_words - pae)
+				return -EINVAL;
+			if (oldcommand->size - sgl_words + pae != sgl_offset)
+				return -EINVAL;
+		}
 
-		if (TW_SGL_OUT(oldcommand->opcode__sgloffset)) {
-			/* Load the sg list */
-			if (tw_dev->tw_pci_dev->device == PCI_DEVICE_ID_3WARE_9690SA)
-				sgl = (TW_SG_Entry *)((u32 *)oldcommand+oldcommand->size - (sizeof(TW_SG_Entry)/4) + pae);
-			else
-				sgl = (TW_SG_Entry *)((u32 *)oldcommand+TW_SGL_OUT(oldcommand->opcode__sgloffset));
-			sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
-			sgl->length = cpu_to_le32(length);
+		if (sgl_offset > max_words || sgl_words > max_words - sgl_offset)
+			return -EINVAL;
 
-			oldcommand->size += pae;
-		}
+		sgl = (TW_SG_Entry *)((u32 *)oldcommand + sgl_offset);
+		memset(sgl, 0, sizeof(*oldcommand) - sgl_offset * sizeof(u32));
+		sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache));
+		sgl->length = cpu_to_le32(length);
+		oldcommand->size = sgl_offset + sgl_words;
 	}
+
+	return 0;
 } /* End twa_load_sgl() */
 
 /* This function will poll for a response interrupt of a request */
-- 
2.54.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing validation of `driver_command.buffer_length` allows out-of-bounds writes on the DMA-allocated bounce buffer during various ioctls.
--

commit 13bf465bbd4ac1f90fcf8616ebc1740e8a86ac93
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

scsi: 3w-9xxx: sanitize passthrough SGLs

TW_IOCTL_FIRMWARE_PASS_THROUGH copies a full command packet from userspace
and then overwrites the first SGL entry. The SGL location and command
size remain user-controlled for legacy commands.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 9b93a2440af8f..c27c7b632ec6a 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 does twa_chrdev_ioctl() missing a minimum
size check for driver_command.buffer_length allow out-of-bounds writes?

If a user supplies a buffer_length of 0, data_buffer_length_adjusted
evaluates to 0, which allocates no extra space for the flexible array member
data_buffer.

However, several ioctls blindly write fixed-size structures into this buffer.
For example, TW_IOCTL_GET_COMPATIBILITY_INFO:

	case TW_IOCTL_GET_COMPATIBILITY_INFO:
		tw_ioctl->driver_command.status = 0;
		/* Copy compatibility struct into ioctl data buffer */
		tw_compat_info = (TW_Compatibility_Info *)tw_ioctl->data_buffer;
		memcpy(tw_compat_info, &tw_dev->tw_compat_info, sizeof(TW_Compatibility_Info));
		break;

Does this result in out-of-bounds writes that corrupt neighboring memory on
the DMA heap?

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

[PATCH v2 2/2] scsi: 3w-xxxx: sanitize passthrough SGLs

[Yousef Alhouseen]

TW_CMD_PACKET_WITH_DATA accepts a command packet from userspace and
patches only the first SGL entry before posting it to the controller. The
command size and remaining SGL contents can still describe user-controlled
DMA descriptors to firmware.

Reject unknown SGL offsets, clear the relevant SGL array, and force the
command size to the single driver-owned DMA buffer. Zero the coherent
ioctl buffer before copying the request so short device writes do not leak
stale memory back to userspace.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
Changes in v2:
- No change.

 drivers/scsi/3w-xxxx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
index 147a47e6b..033f79eaa 100644
--- a/drivers/scsi/3w-xxxx.c
+++ b/drivers/scsi/3w-xxxx.c
@@ -925,6 +925,7 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
 	}
 
 	tw_ioctl = (TW_New_Ioctl *)cpu_addr;
+	memset(tw_ioctl, 0, data_buffer_length_adjusted + sizeof(TW_New_Ioctl));
 
 	/* Now copy down the entire ioctl */
 	if (copy_from_user(tw_ioctl, argp, data_buffer_length + sizeof(TW_New_Ioctl)))
@@ -972,17 +973,31 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
 			/* Load the sg list */
 			switch (TW_SGL_OUT(tw_ioctl->firmware_command.opcode__sgloffset)) {
 			case 2:
+				memset(tw_ioctl->firmware_command.byte8.param.sgl, 0,
+				       sizeof(tw_ioctl->firmware_command.byte8.param.sgl));
+				tw_ioctl->firmware_command.size = 4;
 				tw_ioctl->firmware_command.byte8.param.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				tw_ioctl->firmware_command.byte8.param.sgl[0].length = data_buffer_length_adjusted;
 				break;
 			case 3:
+				memset(tw_ioctl->firmware_command.byte8.io.sgl, 0,
+				       sizeof(tw_ioctl->firmware_command.byte8.io.sgl));
+				tw_ioctl->firmware_command.size = 5;
 				tw_ioctl->firmware_command.byte8.io.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				tw_ioctl->firmware_command.byte8.io.sgl[0].length = data_buffer_length_adjusted;
 				break;
 			case 5:
+				memset(passthru->sg_list, 0, sizeof(passthru->sg_list));
+				passthru->size = 7;
 				passthru->sg_list[0].address = dma_handle + sizeof(TW_New_Ioctl);
 				passthru->sg_list[0].length = data_buffer_length_adjusted;
 				break;
+			default:
+				retval = -EINVAL;
+				tw_dev->chrdev_request_id = TW_IOCTL_CHRDEV_FREE;
+				tw_state_request_finish(tw_dev, request_id);
+				spin_unlock_irqrestore(tw_dev->host->host_lock, flags);
+				goto out2;
 			}
 
 			memcpy(tw_dev->command_packet_virtual_address[request_id], &(tw_ioctl->firmware_command), sizeof(TW_Command));
-- 
2.54.0

Re: [PATCH v2 2/2] scsi: 3w-xxxx: sanitize passthrough SGLs

[sashiko-bot]

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

Pre-existing issues:
- [High] Physical DMA address leak to userspace via `TW_CMD_PACKET_WITH_DATA` ioctl response.
--

commit f37c5fb0d6d5db87dd629b44ae0ac174cd9a5912
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

scsi: 3w-xxxx: sanitize passthrough SGLs

This patch updates TW_CMD_PACKET_WITH_DATA to clear SGL arrays, reject
unknown SGL offsets, and fix the command size. It also zeroes the ioctl
buffer before copying the request to prevent leaking stale memory on short
writes.

> diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
> --- a/drivers/scsi/3w-xxxx.c
> +++ b/drivers/scsi/3w-xxxx.c
[ ... ]
> @@ -966,17 +967,31 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
>  			/* Load the sg list */
>  			switch (TW_SGL_OUT(tw_ioctl->firmware_command.opcode__sgloffset)) {
>  			case 2:
> +				memset(tw_ioctl->firmware_command.byte8.param.sgl, 0,
> +				       sizeof(tw_ioctl->firmware_command.byte8.param.sgl));
> +				tw_ioctl->firmware_command.size = 4;
>  				tw_ioctl->firmware_command.byte8.param.sgl[0].address = dma_handle + sizeof(TW_New_Ioctl);

[Severity: High]
This is a pre-existing issue, but does this leak the physical DMA address
to userspace?

In tw_chrdev_ioctl(), the physical DMA address is written into the command
SGL here. After the device finishes processing the request, the driver
copies the command packet back into the firmware_command structure:

memcpy(&(tw_ioctl->firmware_command), 
       tw_dev->command_packet_virtual_address[request_id], 
       sizeof(TW_Command));

Finally, this buffer is copied back to user memory:

if (copy_to_user(argp, tw_ioctl, sizeof(TW_New_Ioctl) + data_buffer_length))

Since the structure still contains the kernel-injected physical DMA address,
could this help userspace map the physical memory structure or bypass
protections?

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