Re: [PATCH 3/3] scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The asynchronous work `hisi_sas_spinup_notify_work` accesses MMIO registers without acquiring a runtime PM reference, potentially accessing unclocked hardware and causing bus faults.

Pre-existing issues:
- [Critical] Integer overflow in `min_t` causes a massive out-of-bounds `memcpy` when processing malicious SAS sense data.
- [High] Read-modify-write race on the `SL_CONTROL` register between process context and hardirq context can lead to lost hardware state updates.
- [High] Race condition in suspend path allows hardware access after device is disabled due to improper `flush_workqueue()` ordering.
--

commit 02edb194a91901ddad40e4d837a58a4131b5ffa4
Author: Xingui Yang <yangxingui@huawei.com>

scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

This commit fixes a spinup failure for SAS SSP devices powered up with RNOT=1.
It parses sense data in the driver's slot completion path and queues a deferred
work item to notify the phy to spin up the device.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..1cb578e6ae593 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -996,10 +996,22 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
>  	pm_runtime_put_sync(dev);
>  }
>  
> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
> +{
> +	struct hisi_sas_phy *phy =
> +		container_of(work, typeof(*phy), works[HISI_PHYE_SPINUP_NOTIFY]);
> +	struct hisi_hba *hisi_hba = phy->hisi_hba;
> +	int phy_no = phy->sas_phy.id;
> +
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);

[Severity: High]
Could this MMIO access happen while the device is in a suspended state?

When hisi_sas_spinup_notify() queues the HISI_PHYE_SPINUP_NOTIFY work, it
does not take a runtime PM reference on the HBA. Once the completed SAS task
finishes, the block layer can release its runtime PM reference on the device.

If the system is under load and the work execution is delayed beyond the
autosuspend timeout, the host controller might enter runtime suspend. When
hisi_sas_spinup_notify_work() eventually executes, it calls
hisi_hba->hw->sl_notify_ssp(), which directly reads and writes hardware
registers.

Would it be safer to wrap this hardware access with pm_runtime_get_sync()
and pm_runtime_put_sync() to prevent synchronous bus faults or invalid
reads on unclocked hardware?

[Severity: High]
This is a pre-existing issue, but does sl_notify_ssp() contain a
read-modify-write race with hardirq contexts?

For example, in sl_notify_ssp_v3_hw() (called here via workqueue in process
context), the code performs a lockless read-modify-write on the SL_CONTROL
register:

sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
sl_control |= SL_CONTROL_NOTIFY_EN_MSK;
hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);

However, phy_down_v3_hw() (called from the int_phy_up_down_bcast_v3_hw
hardirq handler) also performs a lockless read-modify-write on the same
SL_CONTROL register. Since sl_notify_ssp_v3_hw() doesn't disable interrupts
or hold a spinlock, can the interrupt fire exactly between its read and
write, causing the interrupt's modification to be completely overwritten
and lost?

> +	dev_info(hisi_hba->dev, "spinup notify primitive on phy%d\n", phy_no);
> +}
> +
>  static const work_func_t hisi_sas_phye_fns[HISI_PHYES_NUM] = {
>  	[HISI_PHYE_PHY_UP] = hisi_sas_phyup_work,
>  	[HISI_PHYE_LINK_RESET] = hisi_sas_linkreset_work,
>  	[HISI_PHYE_PHY_UP_PM] = hisi_sas_phyup_pm_work,
> +	[HISI_PHYE_SPINUP_NOTIFY] = hisi_sas_spinup_notify_work,

[Severity: High]
This is a pre-existing issue, but could there be a race condition in the
suspend path that executes newly queued work on disabled hardware?

In _suspend_v3_hw(), the workqueue is flushed before threaded IRQs are
silenced:

flush_workqueue(hisi_hba->wq);
interrupt_disable_v3_hw(hisi_hba);

Because the driver uses threaded IRQs, an interrupt could have already woken
the IRQ thread before interrupt_disable_v3_hw() runs. The IRQ thread (calling
slot_complete_v3_hw()) can queue new work items like HISI_PHYE_SPINUP_NOTIFY
to hisi_hba->wq after the flush_workqueue() call has returned.

Will these newly queued works then execute and access hardware registers
concurrently with or after disable_host_v3_hw(), leading to illegal
accesses on disabled or suspended hardware?

[ ... ]

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index fa94d7110714f..9d89db6e8716e 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1272,6 +1272,9 @@ static void slot_complete_v1_hw(struct hisi_hba *hisi_hba,
>  				&status_buffer->iu[0];
>  
>  		sas_ssp_task_response(dev, task, iu);

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a potential integer
overflow leading to a buffer overflow when sas_ssp_task_response() processes
malicious SAS sense data?

In sas_ssp_task_response() (called here), iu->sense_data_len is converted
to CPU byte order and bounded using min_t():

tstat->buf_valid_size =
    min_t(int, SAS_STATUS_BUF_SIZE,
          be32_to_cpu(iu->sense_data_len));

Because both arguments are cast to int, if a malicious or malformed device
provides a sense_data_len like 0xFFFFFFFF, the cast results in -1. Since
-1 < 96, min_t() returns -1.

When this -1 is passed to the subsequent memcpy(tstat->buf, iu->sense_data,
tstat->buf_valid_size), it is interpreted as a massive size_t value. 
Can this cause an immediate out-of-bounds read from the DMA buffer and a 
massive out-of-bounds write to kernel memory?

> +		if (ts->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> +		    !dev_parent_is_expander(device))
> +			hisi_sas_spinup_notify(hisi_hba, task);
>  		break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829032858.1661019-1-yangxingui@huawei.com?part=3