[PATCH] scsi: 3w-9xxx: validate ioctl result buffer lengths

[PATCH] scsi: 3w-9xxx: validate ioctl result buffer lengths

[Yousef Alhouseen]

Several management ioctls copy a fixed-size event, compatibility record,
or lock structure into the flexible data buffer without checking the
user-supplied buffer length. A short length allocates too little
coherent memory and lets the fixed-size copy write past the allocation.

Determine the minimum data length for each fixed-result command before
allocating the ioctl buffer and reject undersized requests.

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 9b93a2440af8..b8a0b8410a39 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -642,6 +642,7 @@ static long twa_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long
 	unsigned long *cpu_addr, data_buffer_length_adjusted = 0, flags = 0;
 	dma_addr_t dma_handle;
 	int request_id = 0;
+	size_t min_buffer_length = 0;
 	unsigned int sequence_id = 0;
 	unsigned char event_index, start_index;
 	TW_Ioctl_Driver_Command driver_command;
@@ -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_buffer_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_buffer_length = sizeof(TW_Event);
+		break;
+	case TW_IOCTL_GET_LOCK:
+		min_buffer_length = sizeof(TW_Lock);
+		break;
+	}
+
+	if (driver_command.buffer_length < min_buffer_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.55.0

Re: [PATCH] scsi: 3w-9xxx: validate ioctl result buffer lengths

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unvalidated oldcommand->size in TW_IOCTL_FIRMWARE_PASS_THROUGH allows arbitrary physical memory DMA (Privilege Escalation).
- [High] Missing buffer length check for TW_OP_AEN_LISTEN in 3w-xxxx.c allows a 0-length allocation and an out-of-bounds write.
- [Medium] Lockless access to the event queue in twa_chrdev_ioctl leads to data races with the hardware interrupt handler.
--

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

scsi: 3w-9xxx: validate ioctl result buffer lengths

Several management ioctls copy a fixed-size event, compatibility record,
or lock structure into the flexible data buffer without checking the
user-supplied buffer length. This commit determines the minimum data length
for each fixed-result command before allocating the ioctl buffer and rejects
undersized requests.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index a56a7b243e91f..96ef9ca346c6a 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
[ ... ]
> @@ -675,6 +676,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_buffer_length = sizeof(TW_Compatibility_Info);
> +		break;
> +	case TW_IOCTL_GET_LAST_EVENT:

[Severity: Medium]
This is a pre-existing issue, but does lockless access to the event queue in
twa_chrdev_ioctl() lead to data races with the hardware interrupt handler?

In twa_chrdev_ioctl(), event retrieval reads tw_dev->error_index, copies from
tw_dev->event_queue, and updates its retrieved status while holding only the
ioctl_lock mutex:

    event_index = (tw_dev->error_index - 1 + TW_Q_LENGTH) % TW_Q_LENGTH;
    memcpy(tw_ioctl->data_buffer, tw_dev->event_queue[event_index], sizeof(TW_Event));
    tw_dev->event_queue[event_index]->retrieved = TW_AEN_RETRIEVED;

Concurrently, a hardware interrupt can fire, acquire host_lock, and execute
twa_aen_queue_event(), which modifies error_index and overwrites elements in
event_queue.

Because the ioctl path fails to acquire host_lock or disable interrupts, it
seems it can race with the ISR. This could lead to corrupted AEN events being
copied to userspace or newly arrived events being erroneously flagged as
retrieved.

> +	case TW_IOCTL_GET_FIRST_EVENT:
> +	case TW_IOCTL_GET_NEXT_EVENT:
> +	case TW_IOCTL_GET_PREVIOUS_EVENT:
> +		min_buffer_length = sizeof(TW_Event);
> +		break;
> +	case TW_IOCTL_GET_LOCK:
> +		min_buffer_length = sizeof(TW_Lock);
> +		break;
> +	}
> +

[Severity: Critical]
This is a pre-existing issue, but can unvalidated oldcommand->size in
TW_IOCTL_FIRMWARE_PASS_THROUGH allow arbitrary physical memory DMA?

A user can issue TW_IOCTL_FIRMWARE_PASS_THROUGH with an oldcommand format and
an artificially large size value. In twa_load_sgl(), the kernel overwrites
only the first scatter-gather (SG) list entry (sgl[0]) to point to its secure
DMA buffer, but it fails to truncate or validate oldcommand->size:

    sgl->address = TW_CPU_TO_SGL(dma_handle + sizeof(TW_Ioctl_Buf_Apache) - 1);
    sgl->length = cpu_to_le32(length);

    oldcommand->size += pae;

It leaves subsequent user-supplied SG entries intact. The hardware firmware
will process size DWORDs and consume the subsequent, maliciously crafted SG
entries (sgl[1], sgl[2], etc.) to perform DMA directly to/from arbitrary
physical addresses.

Since twa_chrdev_open() lacks a capable(CAP_SYS_ADMIN) check, can any
unprivileged user who can open the device node reach this to gain arbitrary
physical memory read/write?

[Severity: High]
This isn't a bug introduced by this patch, but it appears a similar missing
buffer length check was left unpatched in drivers/scsi/3w-xxxx.c for
TW_OP_AEN_LISTEN. Does this allow a 0-length allocation and an out-of-bounds
write?

In tw_chrdev_ioctl(), if a user sets data_buffer_length to 0 during a
TW_OP_AEN_LISTEN operation, data_buffer_length_adjusted becomes 0. The DMA
allocation allocates exactly sizeof(TW_New_Ioctl) - 1 bytes, aligning the end
of the buffer right before tw_ioctl->data_buffer.

The ioctl then executes a memcpy writing beyond the allocated boundary:

        case TW_OP_AEN_LISTEN:
            ...
            memcpy(tw_ioctl->data_buffer, &tw_aen_code, sizeof(tw_aen_code));
            break;

Can this write 2 bytes (sizeof(tw_aen_code)) past the end of the DMA
allocation, potentially causing memory corruption?

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