[PATCH v2 0/2] scsi: 3ware: bound firmware error strings

[PATCH v2 0/2] scsi: 3ware: bound firmware error strings

[Pengpeng Hou]

The 3w-9xxx and 3w-sas drivers parse a fixed firmware-owned field
containing a description and an optional second string.  Both drivers must
terminate the field before finding that optional string and must not form a
pointer one byte past the field when the first string consumes its last
byte.

Changes in v2:
- Group the two same-root SCSI fixes into one ordered series.
- Factor the repeated bounded optional-string handling into one helper per
  driver.
- Preserve the prior standalone submission lineage below.
Link: https://lore.kernel.org/r/20260704010512.71912-1-pengpeng@iscas.ac.cn
Link: https://lore.kernel.org/r/20260704010619.81878-1-pengpeng@iscas.ac.cn

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

[Pengpeng Hou]

The controller response header stores the description and optional firmware
error string in its fixed 98-byte err_specific_desc field.
twa_aen_queue_event() used strlen() to find the second string before
forcing the final byte to NUL, while twa_fill_sense() used the same
unbounded layout directly.

If a response does not contain a terminator, strlen() can scan past
the field.
If the first string reaches the last field byte, adding one also forms a
pointer past the array.

Terminate the field before parsing it and use a shared helper that treats a
full first string as having no optional second string. This keeps both AEN
and sense reporting paths within the response field.

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

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index 9b93a2440af8..b2462ee43008 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -149,6 +149,24 @@ static int twa_scsiop_execute_scsi(TW_Device_Extension *tw_dev, int request_id,
 static void twa_scsiop_execute_scsi_complete(TW_Device_Extension *tw_dev, int request_id);
 static char *twa_string_lookup(twa_message_type *table, unsigned int aen_code);
 
+/*
+ * The firmware field contains two NUL-terminated strings in one fixed-size
+ * array.  Make the last byte a terminator before finding the optional second
+ * string, and never form a pointer one byte past the array.
+ */
+static const char *twa_error_string(TW_Command_Apache_Header *header)
+{
+	size_t description_len;
+
+	header->err_specific_desc[sizeof(header->err_specific_desc) - 1] = '\0';
+	description_len = strnlen(header->err_specific_desc,
+				  sizeof(header->err_specific_desc));
+	if (description_len == sizeof(header->err_specific_desc) - 1)
+		return "";
+
+	return &header->err_specific_desc[description_len + 1];
+}
+
 /* Functions */
 
 /* Show some statistics about the card */
@@ -376,7 +394,7 @@ static void twa_aen_queue_event(TW_Device_Extension *tw_dev, TW_Command_Apache_H
 	TW_Event *event;
 	unsigned short aen;
 	char host[16];
-	char *error_str;
+	const char *error_str;
 
 	tw_dev->aen_count++;
 
@@ -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));
 	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)
 		printk(KERN_WARNING "3w-9xxx:%s AEN: %s (0x%02X:0x%04X): %s:%s.\n",
@@ -992,12 +1009,12 @@ static int twa_fill_sense(TW_Device_Extension *tw_dev, int request_id, int copy_
 	TW_Command_Full *full_command_packet;
 	unsigned short error;
 	int retval = 1;
-	char *error_str;
+	const char *error_str;
 
 	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]);
+	error_str = twa_error_string(&full_command_packet->header);
 
 	/* Don't print error for Logical unit not supported during rollcall */
 	error = le16_to_cpu(full_command_packet->header.status_block.error);
-- 
2.43.0

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

[Pengpeng Hou]

The controller response header stores the description and optional firmware
error string in its fixed 98-byte err_specific_desc field.
twl_aen_queue_event() and twl_fill_sense() use strlen().  They derive an
optional second string without first proving a terminator lies in the
firmware-owned field.

Without a terminator, strlen() can scan past the field.
A first string reaching the last field byte also makes adding one form a
pointer past the array.

Terminate the field before parsing it and use a shared helper that treats a
full first string as having no optional second string.  This keeps both AEN
and sense reporting paths within the response field.

Fixes: f619106bdd9d ("[SCSI] 3w-sas: Add new driver for LSI 3ware 9750")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/scsi/3w-sas.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c
index 52dc1aa639f7..5fc46a90e026 100644
--- a/drivers/scsi/3w-sas.c
+++ b/drivers/scsi/3w-sas.c
@@ -92,6 +92,24 @@ MODULE_PARM_DESC(use_msi, "Use Message Signaled Interrupts. Default: 0");
 /* Function prototypes */
 static int twl_reset_device_extension(TW_Device_Extension *tw_dev, int ioctl_reset);
 
+/*
+ * The firmware field contains two NUL-terminated strings in one fixed-size
+ * array.  Make the last byte a terminator before finding the optional second
+ * string, and never form a pointer one byte past the array.
+ */
+static const char *twl_error_string(TW_Command_Apache_Header *header)
+{
+	size_t description_len;
+
+	header->err_specific_desc[sizeof(header->err_specific_desc) - 1] = '\0';
+	description_len = strnlen(header->err_specific_desc,
+				  sizeof(header->err_specific_desc));
+	if (description_len == sizeof(header->err_specific_desc) - 1)
+		return "";
+
+	return &header->err_specific_desc[description_len + 1];
+}
+
 /* Functions */
 
 /* This function returns AENs through sysfs */
@@ -226,7 +244,7 @@ static void twl_aen_queue_event(TW_Device_Extension *tw_dev, TW_Command_Apache_H
 	TW_Event *event;
 	unsigned short aen;
 	char host[16];
-	char *error_str;
+	const char *error_str;
 
 	tw_dev->aen_count++;
 
@@ -250,10 +268,9 @@ 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);
+	error_str = twl_error_string(header);
+	event->parameter_len = strnlen(header->err_specific_desc,
+				       sizeof(header->err_specific_desc));
 	memcpy(event->parameter_data, header->err_specific_desc, event->parameter_len + 1 + strlen(error_str));
 	if (event->severity != TW_AEN_SEVERITY_DEBUG)
 		printk(KERN_WARNING "3w-sas:%s AEN: %s (0x%02X:0x%04X): %s:%s.\n",
@@ -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];
 
 	/* 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);
-- 
2.43.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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