Re: [PATCH v2 1/2] scsi: 3w-9xxx: bound firmware error strings

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Fix / Missing Sibling Driver Fix
- [High] Use-After-Free / Locking Freed Memory on Device Removal
--

--- Patch [1]: [PATCH v2 1/2] scsi: 3w-9xxx: bound firmware error strings ---
commit fea64e8649c4887fbfe30a91a57db19d90edb2d7
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: 3w-9xxx: bound firmware error strings

This commit replaces unbounded strlen() calls with strnlen() when parsing
the firmware response header in the 3w-9xxx driver. It also adds a shared
helper function to safely NUL-terminate the field before parsing.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 9b93a2440af8f..b2462ee43008e 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
[ ... ]
> @@ -404,10 +422,9 @@ static void twa_aen_queue_event(TW_Device_Extension *tw_dev, TW_Command_Apache_H
>  	tw_dev->error_sequence_id++;
>  
>  	/* Check for embedded error string */
> -	error_str = &(header->err_specific_desc[strlen(header->err_specific_desc)+1]);
> -
> -	header->err_specific_desc[sizeof(header->err_specific_desc) - 1] = '\0';
> -	event->parameter_len = strlen(header->err_specific_desc);
> +	error_str = twa_error_string(header);
> +	event->parameter_len = strnlen(header->err_specific_desc,
> +				       sizeof(header->err_specific_desc));

[Severity: High]
This isn't a bug introduced by this patch, but does the sibling driver 3w-sas
need a similar fix?

Looking at twl_aen_queue_event() in drivers/scsi/3w-sas.c, it seems to use the
same unbounded strlen() before the array is explicitly NUL-terminated:

drivers/scsi/3w-sas.c:twl_aen_queue_event() {
    ...
    error_str = &(header->err_specific_desc[strlen(header->err_specific_desc)+1]);
    header->err_specific_desc[sizeof(header->err_specific_desc) - 1] = '\0';
    event->parameter_len = strlen(header->err_specific_desc);
    memcpy(event->parameter_data, header->err_specific_desc,
           event->parameter_len + 1 + strlen(error_str));
    ...
}

If the firmware response does not contain a NUL terminator, could this lead
to an out-of-bounds read and a subsequent heap buffer overflow in the
memcpy()?

It looks like twl_fill_sense() in drivers/scsi/3w-sas.c also has the same
pattern. Should these be updated as well?

>  	memcpy(event->parameter_data, header->err_specific_desc, event->parameter_len + (error_str[0] == '\0' ? 0 : (1 + strlen(error_str))));
>  	if (event->severity != TW_AEN_SEVERITY_DEBUG)

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during
device removal in this driver?

Looking at twa_remove() in drivers/scsi/3w-9xxx.c, it frees tw_dev without
clearing the global twa_device_extension_list array or preventing existing
ioctl operations:

drivers/scsi/3w-9xxx.c:twa_remove() {
    ...
    scsi_host_put(tw_dev->host);
    pci_disable_device(pdev);
    twa_device_extension_count--;
}

If a user holds an open file descriptor to /dev/twa0 while the device is
unbound, and then calls ioctl, twa_chrdev_ioctl() retrieves the freed
pointer:

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)) {
    ...
}

Would this result in locking freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715084652.47248-1-pengpeng@iscas.ac.cn?part=1