scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp
Subsystems (2)
PATCH 1/1 scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp
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.
--- 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(-)
drivers/scsi/pm8001/pm8001_defs.h +98 -0
@@ -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,
drivers/scsi/pm8001/pm8001_hwi.c +1 -1
@@ -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 ;
drivers/scsi/pm8001/pm8001_hwi.h +3 -1
@@ -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)));
drivers/scsi/pm8001/pm80xx_hwi.c +1 -1
@@ -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 ;
drivers/scsi/pm8001/pm80xx_hwi.h +3 -97
119 lines not on this page · raw message
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
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 <[email protected]>
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.
[ ... ]
drivers/scsi/pm8001/pm80xx_hwi.c hw_event_sata_phy_up(struct pm8001_hba_info *pm8001_ha, void *piomb)
Quoted 4 lines
drivers/scsi/pm8001/pm80xx_hwi.c +0
-0@@ -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.
Quoted 8 lines
drivers/scsi/pm8001/pm80xx_hwi.c +1
-1phy->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 ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/31963d53810f42970447e78ef46030ab13d2e91f.1788198434.git.bvanassche@acm.org?part=1
Bart,
Quoted 5 lines
In struct
hw_event_resp,sas_identifywas defined using structsas_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.
Applied to 7.4/scsi-staging, thanks!
Signature · 1 line
Martin K. Petersen