[PATCH 0/2] scsi: pm8001: Fix struct layout and FORTIFY_SOURCE crash

[PATCH 0/2] scsi: pm8001: Fix struct layout and FORTIFY_SOURCE crash

[Ronja Meyer]

This patch series:
- Fixes a crash when the driver is built with FORTIFY_SOURCE=y.
- Aligns the struct layout of hw_event_resp to what the HBA believes
  it looks like.
- Simplifies code previously required to work around the incorrect
  struct definition.

Testing:
- Verified I can still read from disks using the pm80xx driver.
- I do not have pm8001 hardware available to verify against.

Signed-off-by: Ronja Meyer <rnj@google.com>
---
Ronja Meyer (2):
      scsi: pm8001: Redefine sas_identify_frame structure
      scsi: pm8001: Match hw_event_resp to HBA data layout

 drivers/scsi/pm8001/pm8001_hwi.c |   6 +--
 drivers/scsi/pm8001/pm8001_hwi.h | 103 +++++++++++++++++++++++++++++++++++++--
 drivers/scsi/pm8001/pm80xx_hwi.c |   6 +--
 drivers/scsi/pm8001/pm80xx_hwi.h |   4 +-
 4 files changed, 108 insertions(+), 11 deletions(-)
---
base-commit: 98f69975d4c0434ca2e6e8cfa1d8d51647a20593
change-id: 20260515-fortify_pm80-b527a10c89d0

Best regards,
-- 
Ronja Meyer <rnj@google.com>

[PATCH 1/2] scsi: pm8001: Redefine sas_identify_frame structure

[Ronja Meyer]

The sas_identify structure defined by pm8001 doesn't have a CRC field.
Add a new sas_identify_frame_local structure without the CRC field.
The equivalent change was already made to the pm80xx driver in:
commit 5990fd57ebea ("scsi: pm80xx: redefine sas_identify_frame structure")

Sending to stable, as this change is required for the fortify-panic fix
later in this chain to apply cleanly.

Cc: stable@vger.kernel.org
Fixes: dbf9bfe61571 ("[SCSI] pm8001: add SAS/SATA HBA driver")
Signed-off-by: Ronja Meyer <rnj@google.com>
---
 drivers/scsi/pm8001/pm8001_hwi.h | 101 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
index f1ce8df082b0..14b162f93eb8 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.h
+++ b/drivers/scsi/pm8001/pm8001_hwi.h
@@ -133,6 +133,103 @@
 
 /* for new SPC controllers MEMBASE III is shared between BIOS and DATA */
 #define GSM_SM_BASE			0x4F0000
+
+#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 */
@@ -153,8 +250,8 @@ struct mpi_msg_hdr{
 struct phy_start_req {
 	__le32	tag;
 	__le32	ase_sh_lm_slr_phyid;
-	struct sas_identify_frame sas_identify;
-	u32	reserved[5];
+	struct	sas_identify_frame_local sas_identify;	/* _local to omit CRC field */
+	u32	reserved[6];
 } __attribute__((packed, aligned(4)));
 
 

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH 2/2] scsi: pm8001: Match hw_event_resp to HBA data layout

[Ronja Meyer]

Correct the hw_event_resp struct definition to match the layout of data
sent by the HBA. Remove pointer arithmetics previously required to work
around incorrect struct definition.

Looking at the struct definition before this patch:
  struct hw_event_resp {
           [...]
           struct	sas_identify_frame sas_identify;
           struct dev_to_host_fis	sata_fis;
   } __attribute__((packed, aligned(4)));

Previously the memcpy() in hw_event_sata_phy_up() crossed reading
from the sas_identify struct over into the sata_fis struct. This was
necessary, because the hw_event_resp struct definition didn't align
properly with what the HBA actually sent. The member sas_identify right
before the member sata_fis was 4 bytes too long, causing the first
4 bytes of the sata_fis to be shifted into the last 4 bytes of
sas_identify. The code worked around this by subtracting 4 bytes from
both the sata_fis pointer, as well as sizeof(sas_identify), when they
were used.

FORTIFY_SOURCE detected this deliberate choice to cross struct member
boundaries as an out-of-bounds read, even though in this case it didn't
lead to a vulnerability. Hence the following fortify-panic was
triggered:

  kernel BUG at lib/string_helpers.c:1044!
  RIP: 0010:__fortify_panic+0x9/0x10
  hw_event_sata_phy_up+0xea/0x120 [pm80xx]
  process_one_iomb+0x634e/0x6360 [pm80xx]
  process_oq+0x391/0x430 [pm80xx]
  pm80xx_chip_isr+0x78/0x100 [pm80xx]
  tasklet_action_common+0x16a/0x2b0
  handle_softirqs+0xcd/0x2a0
  __irq_exit_rcu+0x50/0x100
  common_interrupt+0x89/0xa0

Furthermore hw_event_resp was 64 bytes before this patch, which is
4 bytes too long. Messages exchanged between the pm8001 and the host
kernel can be a maximum of 64 bytes, as defined in iomb_size. The
message structs defined in pm8001_hwi.h must have a size of 60 bytes,
in order to leave space for a 4 byte header that implicitly precedes
each message.

Luckily the code interacting with hw_event_resp doesn't ever seem to
read or write the last 4 bytes of the struct and doesn't seem to use
the incorrect size of the struct in a copy operation. Hence it doesn't
overflow in practice. Further the pm80xx driver was unaffected by this
bug. While the pm80xx struct was also 64 bytes, the message size on
pm80xx is 128 bytes. Hence it is able to fit the 68 byte header and
message without overflowing.

This is not security critical AFAICT.

Cc: stable@vger.kernel.org
Fixes: dbf9bfe61571 ("[SCSI] pm8001: add SAS/SATA HBA driver")
Co-developed-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Ronja Meyer <rnj@google.com>
---
 drivers/scsi/pm8001/pm8001_hwi.c | 6 +++---
 drivers/scsi/pm8001/pm8001_hwi.h | 2 +-
 drivers/scsi/pm8001/pm80xx_hwi.c | 6 +++---
 drivers/scsi/pm8001/pm80xx_hwi.h | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index fff8d877abb9..e90f2d98d8ed 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -3164,8 +3164,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	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, &pPayload->sas_identify,
-		sizeof(struct sas_identify_frame)-4);
-	phy->frame_rcvd_size = sizeof(struct sas_identify_frame) - 4;
+		sizeof(struct sas_identify_frame_local));
+	phy->frame_rcvd_size = sizeof(struct sas_identify_frame_local);
 	pm8001_get_attached_sas_addr(phy, phy->sas_phy.attached_sas_addr);
 	spin_unlock_irqrestore(&phy->sas_phy.frame_rcvd_lock, flags);
 	if (pm8001_ha->flags == PM8001F_RUN_TIME)
@@ -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 14b162f93eb8..2b5483989886 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.h
+++ b/drivers/scsi/pm8001/pm8001_hwi.h
@@ -326,7 +326,7 @@ 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;	/* _local to omit CRC field */
 	struct dev_to_host_fis	sata_fis;
 } __attribute__((packed, aligned(4)));
 
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 954f307352e6..03293e9b84e6 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -3241,8 +3241,8 @@ hw_event_sas_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	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, &pPayload->sas_identify,
-		sizeof(struct sas_identify_frame)-4);
-	phy->frame_rcvd_size = sizeof(struct sas_identify_frame) - 4;
+		sizeof(struct sas_identify_frame_local));
+	phy->frame_rcvd_size = sizeof(struct sas_identify_frame_local);
 	pm8001_get_attached_sas_addr(phy, phy->sas_phy.attached_sas_addr);
 	spin_unlock_irqrestore(&phy->sas_phy.frame_rcvd_lock, flags);
 	if (pm8001_ha->flags == PM8001F_RUN_TIME)
@@ -3289,7 +3289,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..5ccc010a2368 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.h
+++ b/drivers/scsi/pm8001/pm80xx_hwi.h
@@ -351,7 +351,7 @@ struct mpi_msg_hdr {
 struct phy_start_req {
 	__le32	tag;
 	__le32	ase_sh_lm_slr_phyid;
-	struct sas_identify_frame_local sas_identify; /* 28 Bytes */
+	struct	sas_identify_frame_local sas_identify;	/* _local to omit CRC field */
 	__le32 spasti;
 	u32	reserved[21];
 } __attribute__((packed, aligned(4)));
@@ -427,7 +427,7 @@ 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;	/* _local to omit CRC field */
 	struct dev_to_host_fis	sata_fis;
 } __attribute__((packed, aligned(4)));
 

-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH 1/2] scsi: pm8001: Redefine sas_identify_frame structure

[James Bottomley]

On Fri, 2026-05-15 at 12:15 +0000, Ronja Meyer wrote:
> The sas_identify structure defined by pm8001 doesn't have a CRC
> field.
> Add a new sas_identify_frame_local structure without the CRC field.
> The equivalent change was already made to the pm80xx driver in:
> commit 5990fd57ebea ("scsi: pm80xx: redefine sas_identify_frame
> structure")
> 
> Sending to stable, as this change is required for the fortify-panic
> fix
> later in this chain to apply cleanly.
> 
> Cc: stable@vger.kernel.org
> Fixes: dbf9bfe61571 ("[SCSI] pm8001: add SAS/SATA HBA driver")
> Signed-off-by: Ronja Meyer <rnj@google.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.h | 101
> ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h
> b/drivers/scsi/pm8001/pm8001_hwi.h
> index f1ce8df082b0..14b162f93eb8 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.h
> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
> @@ -133,6 +133,103 @@
>  
>  /* for new SPC controllers MEMBASE III is shared between BIOS and
> DATA */
>  #define GSM_SM_BASE			0x4F0000
> +
> +#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

This is basically a duplicate of what's in pm80xx_hwi.h, which looks a
bit ungainly ... couldn't they be unified?  Additionally, it does seem
we could add a #define to the definition in scsi/sas.h to omit the crc
field and allow you to use its definition for both drivers rather than
having to duplicate it like this.

Regards,

James