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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver extracts `request_id` from hardware-controlled data and uses it to directly index multiple fixed-size arrays without any bounds checking, leading to potential out-of-bounds memory reads and writes.
--

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

scsi: 3w-sas: bound firmware error strings

This patch prevents out-of-bounds reads when parsing firmware error strings by
properly terminating the description field. It introduces the helper function
twl_error_string() to safely calculate lengths without scanning past the
98-byte field.

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

[ ... ]

> @@ -861,13 +878,13 @@ static int twl_fill_sense(TW_Device_Extension *tw_dev, int i, int request_id, in
>  	TW_Command_Apache_Header *header;
>  	TW_Command_Full *full_command_packet;
>  	unsigned short error;
> -	char *error_str;
> +	const char *error_str;
>  
>  	header = tw_dev->sense_buffer_virt[i];
>  	full_command_packet = tw_dev->command_packet_virt[request_id];

[Severity: High]
This is a pre-existing issue, but I noticed request_id is used here as an
unvalidated index. Is it possible for a compromised or malfunctioning device
to supply an out-of-bounds request_id?

Looking at twl_interrupt(), request_id is extracted directly from the hardware
without bounds checking:

    request_id = le16_to_cpu(
            tw_dev->sense_buffer_virt[i]->header_desc.request_id);
    if (tw_dev->srb[request_id] != NULL)
        error = twl_fill_sense(tw_dev, i, request_id, 1, 1);

Or alternatively from the MMIO response:

    request_id = TW_RESID_OUT(response);
    full_command_packet = tw_dev->command_packet_virt[request_id];

    if (tw_dev->state[request_id] != TW_S_POSTED) {

If a device returns a request_id greater than or equal to TW_Q_LENGTH (256),
wouldn't this result in out-of-bounds memory accesses against arrays like
tw_dev->srb, tw_dev->command_packet_virt, and tw_dev->state?

>  	/* Get embedded firmware error string */
> -	error_str = &(header->err_specific_desc[strlen(header->err_specific_desc) + 1]);
> +	error_str = twl_error_string(header);
>  
>  	/* Don't print error for Logical unit not supported during rollcall */
>  	error = le16_to_cpu(header->status_block.error);

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