Re: [PATCH] scsi: pm8001: Fix SATA FIS offset in struct hw_event_resp

[sashiko-bot@kernel.org]

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