* [PATCH] scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp
@ 2026-08-31 17:48 Bart Van Assche
2026-08-31 20:07 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Bart Van Assche @ 2026-08-31 17:48 UTC (permalink / raw)
To: Martin K . Petersen
Cc: linux-scsi, Bart Van Assche, Jack Wang, James E.J. Bottomley,
Martin K. Petersen, Nathan Chancellor, Lindar Liu, Kevin Ao,
Tom Peng, Sakthivel K, Hannes Reinecke, Anand Kumar S
In struct hw_event_resp, sas_identify was defined using struct
sas_identify_frame, which is 32 bytes long because it includes a 4-byte
CRC field. However, PM8001 and PM80xx hardware does not send the CRC in
the event response frame, providing only the 28-byte identify payload.
Because sas_identify had a size of 32 bytes, the following sata_fis
member was located at byte offset 44 instead of the actual hardware
offset 40. In hw_event_sata_phy_up(), this discrepancy was worked
around with negative pointer arithmetic: ((u8 *)&pPayload->sata_fis - 4).
This triggers a compiler error under Clang with -Wstringop-overread:
error: 'memcpy' reading 20 bytes from a region of size 0
[-Werror,-Wstringop-overread]
memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
Move struct sas_identify_frame_local (28 bytes) into pm8001_defs.h,
and update struct hw_event_resp in both pm8001_hwi.h and pm80xx_hwi.h
to use struct sas_identify_frame_local followed by sata_fis and a 4-byte
padding word. This correctly aligns sata_fis at byte offset 40 while
maintaining the 64-byte total payload size.
Update hw_event_sata_phy_up() in pm8001_hwi.c and pm80xx_hwi.c to copy
directly from &pPayload->sata_fis.
Compile-tested only. Both the patch and its description have been generated
by Gemini.
Fixes: dbf9bfe61571 ("[SCSI] pm8001: add SAS/SATA HBA driver")
Fixes: f5860992db55 ("[SCSI] pm80xx: Added SPCv/ve specific hardware functionalities and relevant changes in common files")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/pm8001/pm8001_defs.h | 98 +++++++++++++++++++++++++++++
drivers/scsi/pm8001/pm8001_hwi.c | 2 +-
drivers/scsi/pm8001/pm8001_hwi.h | 4 +-
drivers/scsi/pm8001/pm80xx_hwi.c | 2 +-
drivers/scsi/pm8001/pm80xx_hwi.h | 100 +-----------------------------
5 files changed, 106 insertions(+), 100 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_defs.h b/drivers/scsi/pm8001/pm8001_defs.h
index 4e19d61dffbb..a9605ca317f3 100644
--- a/drivers/scsi/pm8001/pm8001_defs.h
+++ b/drivers/scsi/pm8001/pm8001_defs.h
@@ -41,6 +41,104 @@
#ifndef _PM8001_DEFS_H_
#define _PM8001_DEFS_H_
+#include <scsi/sas.h>
+
+#ifdef __LITTLE_ENDIAN_BITFIELD
+struct sas_identify_frame_local {
+ /* Byte 0 */
+ u8 frame_type:4;
+ u8 dev_type:3;
+ u8 _un0:1;
+
+ /* Byte 1 */
+ u8 _un1;
+
+ /* Byte 2 */
+ union {
+ struct {
+ u8 _un20:1;
+ u8 smp_iport:1;
+ u8 stp_iport:1;
+ u8 ssp_iport:1;
+ u8 _un247:4;
+ };
+ u8 initiator_bits;
+ };
+
+ /* Byte 3 */
+ union {
+ struct {
+ u8 _un30:1;
+ u8 smp_tport:1;
+ u8 stp_tport:1;
+ u8 ssp_tport:1;
+ u8 _un347:4;
+ };
+ u8 target_bits;
+ };
+
+ /* Byte 4 - 11 */
+ u8 _un4_11[8];
+
+ /* Byte 12 - 19 */
+ u8 sas_addr[SAS_ADDR_SIZE];
+
+ /* Byte 20 */
+ u8 phy_id;
+
+ u8 _un21_27[7];
+
+} __packed;
+
+#elif defined(__BIG_ENDIAN_BITFIELD)
+struct sas_identify_frame_local {
+ /* Byte 0 */
+ u8 _un0:1;
+ u8 dev_type:3;
+ u8 frame_type:4;
+
+ /* Byte 1 */
+ u8 _un1;
+
+ /* Byte 2 */
+ union {
+ struct {
+ u8 _un247:4;
+ u8 ssp_iport:1;
+ u8 stp_iport:1;
+ u8 smp_iport:1;
+ u8 _un20:1;
+ };
+ u8 initiator_bits;
+ };
+
+ /* Byte 3 */
+ union {
+ struct {
+ u8 _un347:4;
+ u8 ssp_tport:1;
+ u8 stp_tport:1;
+ u8 smp_tport:1;
+ u8 _un30:1;
+ };
+ u8 target_bits;
+ };
+
+ /* Byte 4 - 11 */
+ u8 _un4_11[8];
+
+ /* Byte 12 - 19 */
+ u8 sas_addr[SAS_ADDR_SIZE];
+
+ /* Byte 20 */
+ u8 phy_id;
+
+ u8 _un21_27[7];
+} __packed;
+#else
+#error "Bitfield order not defined!"
+#endif
+
enum chip_flavors {
chip_8001,
chip_8008,
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index fff8d877abb9..55a01e533fd1 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3208,7 +3208,7 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
phy->sas_phy.oob_mode = SATA_OOB_MODE;
sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
- memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
+ memcpy(phy->frame_rcvd, &pPayload->sata_fis,
sizeof(struct dev_to_host_fis));
phy->frame_rcvd_size = sizeof(struct dev_to_host_fis);
phy->identify.target_port_protocols = SAS_PROTOCOL_SATA;
diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
index f1ce8df082b0..a21b1b37a8ea 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.h
+++ b/drivers/scsi/pm8001/pm8001_hwi.h
@@ -42,6 +42,7 @@
#include <linux/types.h>
#include <scsi/libsas.h>
+#include "pm8001_defs.h"
/* for Request Opcode of IOMB */
@@ -229,8 +230,9 @@ struct hw_event_resp {
__le32 lr_evt_status_phyid_portid;
__le32 evt_param;
__le32 npip_portstate;
- struct sas_identify_frame sas_identify;
+ struct sas_identify_frame_local sas_identify;
struct dev_to_host_fis sata_fis;
+ u32 _r_a;
} __attribute__((packed, aligned(4)));
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 2c0fa7ab33d2..659dbd603a03 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -3296,7 +3296,7 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
phy->sas_phy.oob_mode = SATA_OOB_MODE;
sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
- memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
+ memcpy(phy->frame_rcvd, &pPayload->sata_fis,
sizeof(struct dev_to_host_fis));
phy->frame_rcvd_size = sizeof(struct dev_to_host_fis);
phy->identify.target_port_protocols = SAS_PROTOCOL_SATA;
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h b/drivers/scsi/pm8001/pm80xx_hwi.h
index d8a63b7fed6a..d6796ba40df9 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.h
+++ b/drivers/scsi/pm8001/pm80xx_hwi.h
@@ -43,6 +43,7 @@
#include <linux/types.h>
#include <scsi/libsas.h>
+#include "pm8001_defs.h"
/* for Request Opcode of IOMB */
#define OPC_INB_ECHO 1 /* 0x000 */
@@ -236,102 +237,6 @@
/* Port recovery timeout, 10000 ms for PM8006 controller */
#define CHIP_8006_PORT_RECOVERY_TIMEOUT 0x640000
-#ifdef __LITTLE_ENDIAN_BITFIELD
-struct sas_identify_frame_local {
- /* Byte 0 */
- u8 frame_type:4;
- u8 dev_type:3;
- u8 _un0:1;
-
- /* Byte 1 */
- u8 _un1;
-
- /* Byte 2 */
- union {
- struct {
- u8 _un20:1;
- u8 smp_iport:1;
- u8 stp_iport:1;
- u8 ssp_iport:1;
- u8 _un247:4;
- };
- u8 initiator_bits;
- };
-
- /* Byte 3 */
- union {
- struct {
- u8 _un30:1;
- u8 smp_tport:1;
- u8 stp_tport:1;
- u8 ssp_tport:1;
- u8 _un347:4;
- };
- u8 target_bits;
- };
-
- /* Byte 4 - 11 */
- u8 _un4_11[8];
-
- /* Byte 12 - 19 */
- u8 sas_addr[SAS_ADDR_SIZE];
-
- /* Byte 20 */
- u8 phy_id;
-
- u8 _un21_27[7];
-
-} __packed;
-
-#elif defined(__BIG_ENDIAN_BITFIELD)
-struct sas_identify_frame_local {
- /* Byte 0 */
- u8 _un0:1;
- u8 dev_type:3;
- u8 frame_type:4;
-
- /* Byte 1 */
- u8 _un1;
-
- /* Byte 2 */
- union {
- struct {
- u8 _un247:4;
- u8 ssp_iport:1;
- u8 stp_iport:1;
- u8 smp_iport:1;
- u8 _un20:1;
- };
- u8 initiator_bits;
- };
-
- /* Byte 3 */
- union {
- struct {
- u8 _un347:4;
- u8 ssp_tport:1;
- u8 stp_tport:1;
- u8 smp_tport:1;
- u8 _un30:1;
- };
- u8 target_bits;
- };
-
- /* Byte 4 - 11 */
- u8 _un4_11[8];
-
- /* Byte 12 - 19 */
- u8 sas_addr[SAS_ADDR_SIZE];
-
- /* Byte 20 */
- u8 phy_id;
-
- u8 _un21_27[7];
-} __packed;
-#else
-#error "Bitfield order not defined!"
-#endif
-
struct mpi_msg_hdr {
__le32 header; /* Bits [11:0] - Message operation code */
/* Bits [15:12] - Message Category */
@@ -427,8 +332,9 @@ struct hw_event_resp {
__le32 lr_status_evt_portid;
__le32 evt_param;
__le32 phyid_npip_portstate;
- struct sas_identify_frame sas_identify;
+ struct sas_identify_frame_local sas_identify;
struct dev_to_host_fis sata_fis;
+ u32 _r_a;
} __attribute__((packed, aligned(4)));
/*
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp
2026-08-31 17:48 [PATCH] scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp Bart Van Assche
@ 2026-08-31 20:07 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-31 20:07 UTC (permalink / raw)
To: Bart Van Assche; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing bounds checks for unvalidated `port_id` and `phy_id` extracted from hardware payloads in hardware event handlers, leading to potential out-of-bounds array accesses.
--
commit 54581172c25d0fbd24fe1a048aa6e9fe28d5e2e9
Author: Bart Van Assche <bvanassche@acm.org>
scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp
This commit fixes a Clang compiler warning about negative pointer
arithmetic by properly defining the SATA FIS offset in the struct
hw_event_resp. It extracts the 28-byte identify frame into a
separate struct and aligns the sata_fis member to its actual
hardware byte offset.
[ ... ]
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -3296,7 +3296,7 @@ hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
[Severity: High]
This is a pre-existing issue, but does this function lack bounds checking
on the port_id and phy_id extracted from the hardware payload?
Looking earlier in hw_event_sata_phy_up():
u8 port_id = (u8)(lr_status_evt_portid & 0x000000FF);
u8 phy_id = (u8)((phyid_npip_portstate & 0xFF0000) >> 16);
...
struct pm8001_port *port = &pm8001_ha->port[port_id];
struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
Since port_id and phy_id can be up to 255 based on the bitwise masking,
and the port and phy arrays are statically sized to PM8001_MAX_PHYS (16),
could a malfunctioning or untrusted hardware payload cause out-of-bounds
array accesses here?
A similar pattern also appears to exist in pm80xx_mpi_hw_event() when
handling untrusted event payloads.
> phy->sas_phy.oob_mode = SATA_OOB_MODE;
> sas_notify_phy_event(&phy->sas_phy, PHYE_OOB_DONE, GFP_ATOMIC);
> spin_lock_irqsave(&phy->sas_phy.frame_rcvd_lock, flags);
> - memcpy(phy->frame_rcvd, ((u8 *)&pPayload->sata_fis - 4),
> + memcpy(phy->frame_rcvd, &pPayload->sata_fis,
> sizeof(struct dev_to_host_fis));
> phy->frame_rcvd_size = sizeof(struct dev_to_host_fis);
> phy->identify.target_port_protocols = SAS_PROTOCOL_SATA;
--
Sashiko AI review · https://sashiko.dev/#/patchset/31963d53810f42970447e78ef46030ab13d2e91f.1788198434.git.bvanassche@acm.org?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-08-31 20:07 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-31 17:48 [PATCH] scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp Bart Van Assche
2026-08-31 20:07 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.