Re: [PATCH] scsi: 3w-xxxx: validate AEN ioctl buffer length

[sashiko-bot@kernel.org]

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 unvalidated scatter-gather lists in TW_CMD_PACKET_WITH_DATA ioctl.
- [High] Out-of-bounds slab writes in 3w-9xxx.c when processing ioctls with small data buffers.
- [High] Use-after-free on tw_dev due to missing Scsi_Host reference holding in character device open.
- [High] DMA use-after-free when hardware reset fails during ioctl command timeout.
--

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

scsi: 3w-xxxx: validate AEN ioctl buffer length

TW_OP_AEN_LISTEN copies a fixed AEN code into the ioctl data buffer, but
the coherent ioctl allocation is sized from the user supplied buffer
length. A zero-length or otherwise short buffer lets the handler write
past the allocated ioctl buffer.

Reject AEN listen requests whose data buffer is too small for the code
returned by the driver.

> diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
> index c68678fa72c1e..147a47e6b3e55 100644
> --- a/drivers/scsi/3w-xxxx.c
> +++ b/drivers/scsi/3w-xxxx.c
> @@ -908,6 +908,12 @@ static long tw_chrdev_ioctl(struct file *file, unsigned int cmd, unsigned long a
>  		goto out;
>  	}
>  
> +	if (cmd == TW_OP_AEN_LISTEN &&
> +	    data_buffer_length < sizeof(tw_aen_code)) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the sibling driver
3w-9xxx.c suffer from the exact same out-of-bounds write that this commit
fixes?

In twa_chrdev_ioctl(), if the user provides a buffer length smaller than the
structure size, the fixed-size copy might overwrite the allocated buffer:

drivers/scsi/3w-9xxx.c:twa_chrdev_ioctl() {
    ...
    tw_compat_info = (TW_Compatibility_Info *)tw_ioctl->data_buffer;
    memcpy(tw_compat_info, &tw_dev->tw_compat_info,
           sizeof(TW_Compatibility_Info));
    ...
}


[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free race
condition regarding the scsi_host reference.

In tw_chrdev_open(), we don't pin the Scsi_Host via scsi_host_get(). If the
device is concurrently hot-removed, tw_remove() frees the host data while
the file descriptor remains open.

Could a subsequent ioctl call dereference the freed tw_dev pointer here?

tw_chrdev_ioctl() {
    ...
    if (mutex_lock_interruptible(&tw_dev->ioctl_lock)) {
    ...
}

>  	/* Hardware can only do multiple of 512 byte transfers */
>  	data_buffer_length_adjusted = (data_buffer_length + 511) & ~511;

[Severity: Critical]
This is a pre-existing issue, but is there a risk of arbitrary physical
memory access when handling TW_CMD_PACKET_WITH_DATA?

Later in tw_chrdev_ioctl(), the switch statement only overwrites sgl[0] for
specific opcode offsets:

tw_chrdev_ioctl() {
    ...
    switch (TW_SGL_OUT(tw_ioctl->firmware_command.opcode__sgloffset)) {
    case 2:
        tw_ioctl->firmware_command.byte8.param.sgl[0].address =
            dma_handle + sizeof(TW_New_Ioctl);
        ...
    }
    
    memcpy(tw_dev->command_packet_virtual_address[request_id], 
           &(tw_ioctl->firmware_command), sizeof(TW_Command));
    ...
}

If a user with CAP_SYS_ADMIN provides an unhandled offset or additional SGL
entries (sgl[1..61]), it seems those unvalidated physical addresses are
copied into the command packet and passed directly to the hardware. Could
this be used to bypass kernel lockdown limits?


[Severity: High]
This is a pre-existing issue, but I have a question about the error handling
if a hardware command times out.

If the hardware fails to reset, we jump to out2:

tw_chrdev_ioctl() {
    ...
    if (tw_dev->chrdev_request_id != TW_IOCTL_CHRDEV_FREE) {
        retval = -EIO;
        if (tw_reset_device_extension(tw_dev)) {
            printk(KERN_WARNING "...");
        }
        goto out2;
    }
    ...
}

At out2, the DMA buffer is freed:

out2:
    dma_free_coherent(&tw_dev->tw_pci_dev->dev, 
                      data_buffer_length_adjusted + sizeof(TW_New_Ioctl), 
                      cpu_addr, dma_handle);

If the hardware is still active because the reset failed, could it continue
processing the stalled command and write into these freed physical memory
pages?

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