Re: [PATCH] scsi: 3w-sas: bound firmware error string parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Security Fix / Missing Bug Fix in Sibling Driver
- [High] Use-After-Free (UAF) in character device ioctl
- [Medium] Teardown Asymmetry breaks all remaining controllers on single device removal
- [Medium] Incorrect endianness conversion for hardware error code in printk statement
- [Low] Missing endianness annotations on hardware structures hides bugs
--

commit 027577f5ce5e60299d84e8b1953276ed554014b3
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: 3w-sas: bound firmware error string parsing

3w-sas parses two NUL-separated firmware strings from the fixed
err_specific_desc[] field.  The AEN and sense paths locate the second
string with strlen(first) + 1 before guaranteeing that the first
string is terminated inside the array.

Force a terminator in the fixed field before parsing, use strnlen()
for the first and optional second strings, and copy only the bytes
known to reside in err_specific_desc[].

> diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
> index 52dc1aa639f7e..0a5e23819a826 100644
> --- a/drivers/scsi/3w-sas.c
> +++ b/drivers/scsi/3w-sas.c

[ ... ]

> @@ -250,11 +251,21 @@ static void twl_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);
> -	memcpy(event->parameter_data, header->err_specific_desc, event->parameter_len + 1 + strlen(error_str));
> +	desc_len = strnlen(header->err_specific_desc,
> +			   sizeof(header->err_specific_desc));
> +	error_str = "";
> +	error_len = 0;
> +	if (desc_len + 1 < sizeof(header->err_specific_desc)) {
> +		error_str = &header->err_specific_desc[desc_len + 1];
> +		error_len = strnlen(error_str,
> +				    sizeof(header->err_specific_desc) -
> +				    desc_len - 1);
> +	}
> +
> +	event->parameter_len = desc_len;
> +	memcpy(event->parameter_data, header->err_specific_desc,
> +	       desc_len + (error_len ? 1 + error_len : 0));

[Severity: High]
This is a pre-existing issue, but the exact same vulnerability patched by 
this commit in 3w-sas.c appears to be present in the sibling 3w-9xxx.c
driver.

Does twa_aen_queue_event() in drivers/scsi/3w-9xxx.c also need this string
termination fix to prevent a heap buffer overflow when writing to the
event->parameter_data array?

>  	if (event->severity != TW_AEN_SEVERITY_DEBUG)
>  		printk(KERN_WARNING "3w-sas:%s AEN: %s (0x%02X:0x%04X): %s:%s.\n",

[ ... ]

> @@ -862,12 +873,19 @@ static int twl_fill_sense(TW_Device_Extension *tw_dev, int i, int request_id, in
>  	TW_Command_Full *full_command_packet;
>  	unsigned short error;
>  	char *error_str;
> +	size_t desc_len;
>  
>  	header = tw_dev->sense_buffer_virt[i];
>  	full_command_packet = tw_dev->command_packet_virt[request_id];
>  
>  	/* Get embedded firmware 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';
> +	desc_len = strnlen(header->err_specific_desc,
> +			   sizeof(header->err_specific_desc));
> +	if (desc_len + 1 < sizeof(header->err_specific_desc))
> +		error_str = &header->err_specific_desc[desc_len + 1];
> +	else
> +		error_str = "";
>  
>  	/* Don't print error for Logical unit not supported during rollcall */
>  	error = le16_to_cpu(header->status_block.error);

[Severity: Medium]
This isn't a bug introduced by this patch, but does the printk statement
immediately following this line log the byte-swapped hardware error code on
big-endian architectures? 

The code correctly converts the hardware error code to local endianness
using le16_to_cpu(header->status_block.error), but then passes the
unconverted header->status_block.error to %04X instead of using the local
error variable.


While reviewing these changes, I noticed a few other pre-existing issues
in this driver that might be worth looking into:

[Severity: High]
This is a pre-existing issue, but is it possible for a use-after-free to
occur if a user-space process opens /dev/twl and the PCI device is
hot-removed?

In twl_remove(), scsi_host_put() frees the device structure without clearing
the pointer from twl_device_extension_list:

drivers/scsi/3w-sas.c:twl_remove() {
    ...
    scsi_host_put(tw_dev->host);
    pci_disable_device(pdev);
    twl_device_extension_count--;
    ...
}

Could a subsequent ioctl call to twl_chrdev_ioctl() dereference the freed
pointer since it unconditionally fetches it from the global array?

drivers/scsi/3w-sas.c:twl_chrdev_ioctl() {
    ...
    TW_Device_Extension *tw_dev = twl_device_extension_list[iminor(inode)];
    mutex_lock(&twl_chrdev_mutex);
    if (mutex_lock_interruptible(&tw_dev->ioctl_lock)) {
    ...
}

[Severity: Medium]
This is a pre-existing issue, but does twl_remove() unconditionally destroy
the management ioctl interface for all active controllers?

If one controller is unbound on a multi-controller system,
unregister_chrdev() is called and twl_major is set to -1, which might break
communication with the remaining controllers:

drivers/scsi/3w-sas.c:twl_remove() {
    ...
    if (twl_major >= 0) {
        unregister_chrdev(twl_major, "twl");
        twl_major = -1;
    }
    ...
}

[Severity: Low]
This is a pre-existing issue, but should the hardware-defined structures
like TW_Command_Apache_Header use __le16 instead of unsigned short for
fields like error?

drivers/scsi/3w-sas.h:TAG_TW_Command_Apache_Header {
    ...
    unsigned short error;
    ...
}

Using __le16 might help static analysis tools catch endianness regressions
like the printk bug mentioned above.

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