Re: [PATCH] scsi: scsi_dh_alua: validate RTPG descriptors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The RTPG parsing logic trusts the `len` field from the RTPG payload header without verifying the residual transfer length from the SCSI execution. If a target returns a short read, the un-transferred portion of the buffer (which is zeroed by the SCSI midlayer, or potentially left uninitialized by a buggy HBA) is blindly parsed as valid ALUA port group descriptors.
--

commit ede42460fc942e5ee60c7349a556d0e7a6fd87bb
Author: Pengpeng <pengpeng@iscas.ac.cn>

scsi: scsi_dh_alua: validate RTPG descriptors

This commit adds validation to the RTPG descriptor parsing loop in
alua_rtpg() to ensure the fixed eight-byte header is present before
reading descriptor fields.

> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 80ab0ff921d43..f63f32bad12a3 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -681,7 +681,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  	for (k = tpg_desc_tbl_off, desc = buff + tpg_desc_tbl_off;
>  	     k < len;
>  	     k += off, desc += off) {

[Severity: High]
This is a pre-existing issue, but does this loop safely handle short SCSI
transfers?

The RTPG parsing logic appears to trust the len field from the RTPG payload
header without verifying the residual transfer length from the SCSI
execution.

Looking at submit_rtpg(), scsi_execute_cmd() is called without configuring
exec_args.resid to capture the residual count:

drivers/scsi/device_handler/scsi_dh_alua.c:submit_rtpg()
        const struct scsi_exec_args exec_args = {
                .sshdr = sshdr,
        };

Then in alua_rtpg(), the payload length is blindly trusted:

drivers/scsi/device_handler/scsi_dh_alua.c:alua_rtpg()
        len = get_unaligned_be32(&buff[0]) + 4;

        if (len > bufflen) {
                /* Resubmit with the correct length */

If a target returns a short read while specifying a larger length in its
header, could the un-transferred portion of the buffer be parsed as valid
ALUA port group descriptors? 

This might cause the driver to parse zeroes or uninitialized memory,
deterministically forcing the state of ALUA group 0 to 0 
(SCSI_ACCESS_STATE_OPTIMAL) or causing random ALUA state corruption. It 
might also cause the driver to continually reallocate the buffer up to 
the kmalloc limit if the reported len remains larger than bufflen.

> -		u16 group_id = get_unaligned_be16(&desc[2]);
> +		u16 group_id;
> +
> +		if (len - k < 8)
> +			break;
> +
> +		group_id = get_unaligned_be16(&desc[2]);
>  
>  		spin_lock_irqsave(&port_group_lock, flags);
>  		tmp_pg = alua_find_get_pg(pg->device_id_str, pg->device_id_len,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706091440.77131-1-pengpeng@iscas.ac.cn?part=1