[PATCH] scsi: 3w-9xxx: validate ioctl data buffer sizes

[PATCH] scsi: 3w-9xxx: validate ioctl data buffer sizes

[Yousef Alhouseen]

Several 3w-9xxx character ioctls read or write fixed-size structures in
the ioctl data buffer, but allocation is based only on the user supplied
buffer_length. A short buffer can make event, compatibility, or lock
commands access beyond the allocated coherent ioctl buffer.

Require the data buffer to be large enough for the fixed payload used by
each local ioctl before allocating and copying the full request.

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

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index 9b93a2440..a125801e3 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -653,6 +653,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	ktime_t current_time;
 	TW_Device_Extension *tw_dev = twa_device_extension_list[iminor(inode)];
 	int retval = TW_IOCTL_ERROR_OS_EFAULT;
+	unsigned int min_data_length = 0;
 	void __user *argp = (void __user *)arg;
 
 	mutex_lock(&twa_chrdev_mutex);
@@ -673,6 +674,26 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 		goto out2;
 	}
 
+	switch (cmd) {
+	case TW_IOCTL_GET_COMPATIBILITY_INFO:
+		min_data_length = sizeof(TW_Compatibility_Info);
+		break;
+	case TW_IOCTL_GET_LAST_EVENT:
+	case TW_IOCTL_GET_FIRST_EVENT:
+	case TW_IOCTL_GET_NEXT_EVENT:
+	case TW_IOCTL_GET_PREVIOUS_EVENT:
+		min_data_length = sizeof(TW_Event);
+		break;
+	case TW_IOCTL_GET_LOCK:
+		min_data_length = sizeof(TW_Lock);
+		break;
+	}
+
+	if (driver_command.buffer_length < min_data_length) {
+		retval = TW_IOCTL_ERROR_OS_EINVAL;
+		goto out2;
+	}
+
 	/* Hardware can only do multiple of 512 byte transfers */
 	data_buffer_length_adjusted = (driver_command.buffer_length + 511) & ~511;
 
@@ -2302,4 +2323,3 @@ static void __exit twa_exit(void)
 
 module_init(twa_init);
 module_exit(twa_exit);
-
-- 
2.54.0

Re: [PATCH] scsi: 3w-9xxx: validate ioctl data buffer sizes

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Arbitrary physical memory read/write via untruncated user-supplied SGL array in `twa_load_sgl`.
- [High] The patch misses an identical buffer overflow vulnerability in the sibling driver 3w-xxxx.c.
- [High] Hardware DMA Use-After-Free if controller reset fails during command timeout.
- [High] Out-of-bounds write of DMA address and length in `twa_load_sgl` due to unvalidated `oldcommand->size`.
--

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

scsi: 3w-9xxx: validate ioctl data buffer sizes

Several 3w-9xxx character ioctls read or write fixed-size structures in
the ioctl data buffer, but allocation is based only on the user supplied
buffer_length. This patch requires the data buffer to be large enough for
the fixed payload used by each local ioctl before allocating and copying
the full request.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 9b93a2440af8f..a125801e3aad9 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
> @@ -653,6 +653,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  	ktime_t current_time;
>  	TW_Device_Extension *tw_dev = twa_device_extension_list[iminor(inode)];
>  	int retval = TW_IOCTL_ERROR_OS_EFAULT;
> +	unsigned int min_data_length = 0;
>  	void __user *argp = (void __user *)arg;
>  
>  	mutex_lock(&twa_chrdev_mutex);

[ ... ]

> @@ -673,6 +674,26 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
>  		goto out2;
>  	}
>  
> +	switch (cmd) {
> +	case TW_IOCTL_GET_COMPATIBILITY_INFO:
> +		min_data_length = sizeof(TW_Compatibility_Info);
> +		break;
> +	case TW_IOCTL_GET_LAST_EVENT:
> +	case TW_IOCTL_GET_FIRST_EVENT:
> +	case TW_IOCTL_GET_NEXT_EVENT:
> +	case TW_IOCTL_GET_PREVIOUS_EVENT:
> +		min_data_length = sizeof(TW_Event);
> +		break;
> +	case TW_IOCTL_GET_LOCK:
> +		min_data_length = sizeof(TW_Lock);
> +		break;
> +	}
> +
> +	if (driver_command.buffer_length < min_data_length) {
> +		retval = TW_IOCTL_ERROR_OS_EINVAL;
> +		goto out2;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does a similar regression
exist in the sibling driver 3w-xxxx.c?

If the TW_OP_AEN_LISTEN ioctl is issued with data_buffer_length set to 0, the
driver appears to allocate exactly sizeof(TW_New_Ioctl) bytes, leaving 0 bytes
for the flexible array member data_buffer.

drivers/scsi/3w-xxxx.c:tw_chrdev_ioctl() {
    ...
    memcpy(tw_ioctl->data_buffer, &tw_aen_code, sizeof(tw_aen_code));
    ...
}

Could this lead to an out-of-bounds write of 2 bytes into kernel memory?

[Severity: High]
This is a pre-existing issue, but looking further down in twa_chrdev_ioctl(),
could this sequence lead to hardware DMA into freed memory?

If an ioctl command is sent to the hardware but times out, the driver
attempts to reset the controller.

drivers/scsi/3w-9xxx.c:twa_chrdev_ioctl() {
    ...
			twa_reset_device_extension(tw_dev);
			goto out3;
    ...
}

If the reset fails, the hardware might still be active.

Since the code unconditionally jumps to out3 and frees the DMA coherent buffer,
could the hardware still DMA into the freed memory if it completes the command
later?

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about how
the SGL pointer is calculated in twa_load_sgl().

If a user issues TW_IOCTL_FIRMWARE_PASS_THROUGH with a small buffer_length
but a large oldcommand->size:

drivers/scsi/3w-9xxx.c:twa_load_sgl() {
    ...
				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);
    ...
}

Since oldcommand->size is unvalidated, could this calculation result in an
out-of-bounds write of the DMA address and length into kernel memory?

[Severity: Critical]
This is a pre-existing issue, but does the hardware process additional
user-supplied SGL entries?

In twa_load_sgl(), oldcommand->size is modified to account for PAE:

drivers/scsi/3w-9xxx.c:twa_load_sgl() {
    ...
		oldcommand->size += pae;
    ...
}

If the user embeds multiple SGL entries and oldcommand->size is not truncated
to a single entry, could the hardware process subsequent malicious SGL
entries and access arbitrary physical memory?

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