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

[sashiko-bot@kernel.org]

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