Re: [PATCH v2 3/6] scsi: ufs: core: Validate connected lane counts

[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 check on untrusted `tx_lanes` value read from the device in `ufshcd_disable_tx_lcc` could lead to an excessive loop and kernel hang.
--

--- Patch [3]: [PATCH v2 3/6] scsi: ufs: core: Validate connected lane counts ---
commit a03fd44c15e264d545e15508efe3bce7ec4082a8
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: core: Validate connected lane counts

This commit updates ufshcd_get_max_pwr_mode() to validate that RX and TX
lane counts read from the device do not exceed UFS_MAX_LANES. It ensures
these counts are bounded before they are propagated.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d1b989abc9e2d..bbadddcd661a0 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4729,7 +4729,9 @@ static int ufshcd_get_max_pwr_mode(struct ufs_hba *hba)
>  	ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
>  			&pwr_info->lane_tx);
>  
> -	if (!pwr_info->lane_rx || !pwr_info->lane_tx) {
> +	if (!pwr_info->lane_rx || !pwr_info->lane_tx ||
> +	    pwr_info->lane_rx > UFS_MAX_LANES ||
> +	    pwr_info->lane_tx > UFS_MAX_LANES) {
>  		dev_err(hba->dev, "%s: invalid connected lanes value. rx=%d, tx=%d\n",
>  				__func__,
>  				pwr_info->lane_rx,

[Severity: High]
This isn't a bug introduced by this patch, but since we are adding bounds
checking for PA_CONNECTEDTXDATALANES here, is it possible we need similar
validation in ufshcd_disable_tx_lcc()?

When ufshcd_disable_tx_lcc() reads the connected TX lane count, it uses the
value directly as a loop boundary:

drivers/ufs/core/ufshcd.c:ufshcd_disable_tx_lcc() {
	...
	if (!peer)
		ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
			       &tx_lanes);
	else
		ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
				    &tx_lanes);
	for (i = 0; i < tx_lanes; i++) {
	...
}

If a malicious endpoint or UniPro link error causes the read to return an
abnormally large integer, the code could enter an unvalidated loop.

If the endpoint continuously returns success to the synchronous
ufshcd_dme_peer_set() commands inside the loop, could this force the kernel
to execute an excessive number of slow hardware commands?

This might potentially block the probe or error handler thread and result
in a kernel hang.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717153914.26321-1-liqiang01@kylinos.cn?part=3