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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The identical out-of-bounds read and copy vulnerabilities fixed by this patch in the 3w-9xxx driver also exist in the 3w-sas driver, but were missed. The `twl_aen_queue_event` function calls `strlen(header->err_specific_desc)` without bounds checking, which can read past the fixed 98-byte firmware array. Furthermore, it uses the resulting unbounded pointer `error_str` to calculate the copy length for a `memcpy()` into the 98-byte `event->parameter_data` array, leading to a heap buffer overflow. A similar unbounded `strlen()` is also present in `twl_fill_sense`.
--

commit 18b063874ec862a643c52abdfc2ae2cc3b0824b7
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: 3w-9xxx: bound firmware error string parsing

This commit bounds firmware error string parsing in the 3w-9xxx driver.
It replaces unbounded strlen() calls with strnlen() and enforces null
termination to prevent potential out-of-bounds reads and writes.

> diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
> index 9b93a2440af8f..d6e183517797e 100644
> --- a/drivers/scsi/3w-9xxx.c
> +++ b/drivers/scsi/3w-9xxx.c
[ ... ]
> @@ -404,11 +405,21 @@ 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);
> -	memcpy(event->parameter_data, header->err_specific_desc, event->parameter_len + (error_str[0] == '\0' ? 0 : (1 + strlen(error_str))));
> +	desc_len = strnlen(header->err_specific_desc,
> +			   sizeof(header->err_specific_desc));

[Severity: High]
This is a pre-existing issue, but the identical out-of-bounds read and copy
vulnerabilities fixed here also exist in the 3w-sas driver.

In drivers/scsi/3w-sas.c:twl_aen_queue_event(), the code calls
strlen(header->err_specific_desc) without bounds checking, which can read
past the fixed firmware array if the SCSI controller firmware sends an AEN
lacking a null terminator:

    error_str = &(header->err_specific_desc[strlen(header->err_specific_desc)+1]);
    ...
    event->parameter_len = strlen(header->err_specific_desc);
    memcpy(event->parameter_data, header->err_specific_desc, event->parameter_len + 1 + strlen(error_str));

This uses the unbounded pointer error_str to calculate the copy length for a
memcpy() into the event->parameter_data array, leading to a heap buffer 
overflow.

A similar unbounded strlen() is also present in drivers/scsi/3w-sas.c in
twl_fill_sense():

    error_str = &(header->err_specific_desc[strlen(header->err_specific_desc) + 1]);

Would it make sense to extend these bounds checking fixes to the 3w-sas
driver as well?

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