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

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

[Pengpeng Hou]

3w-9xxx parses two NUL-separated firmware strings from
err_specific_desc[].  Both the AEN path and the sense-print path call
strlen() on the firmware buffer before forcing a terminator at the
end of the fixed array.  If firmware provides an unterminated first
string, strlen() can walk beyond err_specific_desc[].

Terminate the fixed firmware field before parsing it, use strnlen()
within the field bounds, and only expose the optional second string
when it starts inside the same array.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/scsi/3w-9xxx.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -377,6 +377,7 @@
 	unsigned short aen;
 	char host[16];
 	char *error_str;
+	size_t desc_len, error_len;
 
 	tw_dev->aen_count++;
 
@@ -404,11 +405,21 @@
 	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));
+	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));
 	if (event->severity != TW_AEN_SEVERITY_DEBUG)
 		printk(KERN_WARNING "3w-9xxx:%s AEN: %s (0x%02X:0x%04X): %s:%s.\n",
 		       host,
@@ -993,11 +1004,20 @@
 	unsigned short error;
 	int retval = 1;
 	char *error_str;
+	char *desc;
+	size_t desc_len;
 
 	full_command_packet = tw_dev->command_packet_virt[request_id];
 
 	/* Check for embedded error string */
-	error_str = &(full_command_packet->header.err_specific_desc[strlen(full_command_packet->header.err_specific_desc) + 1]);
+	desc = full_command_packet->header.err_specific_desc;
+	desc[sizeof(full_command_packet->header.err_specific_desc) - 1] = '\0';
+	desc_len = strnlen(desc,
+			   sizeof(full_command_packet->header.err_specific_desc));
+	if (desc_len + 1 < sizeof(full_command_packet->header.err_specific_desc))
+		error_str = &desc[desc_len + 1];
+	else
+		error_str = "";
 
 	/* Don't print error for Logical unit not supported during rollcall */
 	error = le16_to_cpu(full_command_packet->header.status_block.error);

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

[sashiko-bot]

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