Re: [PATCH] scsi: core: reject short REPORT LUNS responses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: core: incomplete short-transfer validation in scsi_report_lun_scan()
--

commit d5b77af2d901c3158f427e5d31d0271faf51bd28
Author: shao.mingyin@zte.com.cn <shao.mingyin@zte.com.cn>

scsi: core: reject short REPORT LUNS responses

This commit requests the transfer residual from scsi_execute_cmd() in
scsi_report_lun_scan() and aborts the REPORT LUNS scan when fewer than
sizeof(struct scsi_lun) bytes were transferred.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 52f3b525d2fda..abbcf529bdc4c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1475,10 +1475,11 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  		.total_allowed = 3,
>  		.failure_definitions = failure_defs,
>  	};
> +	int ret = 0, resid = 0;
>  	const struct scsi_exec_args exec_args = {
> +		.resid = &resid,
>  		.failures = &failures,
>  	};
> -	int ret = 0;
>  
>  	/*
>  	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
> @@ -1567,6 +1568,12 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  		goto out_err;
>  	}
>  
> +	if (resid > length - sizeof(struct scsi_lun)) {
> +		/* Short transfer: the LUN list header was not filled in. */
> +		ret = 1;
> +		goto out_err;
> +	}

[Severity: Medium]
What happens if a target transfers the 8-byte LUN list header reporting a
non-zero LUN list length, but truncates the transfer before sending all of
the reported LUN descriptors?

In drivers/scsi/scsi_scan.c:scsi_report_lun_scan(), after this check passes,
length and num_luns are computed solely from the header value without
verifying that the transferred byte count (length - resid) covers the full
reported list:

	if (get_unaligned_be32(lun_data->scsi_lun) +
	    sizeof(struct scsi_lun) > length) {
		length = get_unaligned_be32(lun_data->scsi_lun) +
			 sizeof(struct scsi_lun);
		kfree(lun_data);
		goto retry;
	}
	length = get_unaligned_be32(lun_data->scsi_lun);

	num_luns = (length / sizeof(struct scsi_lun));

Because drivers/scsi/scsi_lib.c:scsi_execute_cmd() zeroes the residual tail
of the buffer:

	if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen))
		memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len);

	if (args->resid)
		*args->resid = scmd->resid_len;

wouldn't a truncated payload cause the scan loop to decode a partially
zeroed descriptor at the truncation boundary into a corrupted LUN number and
repeatedly probe LUN 0 for all remaining untransferred entries, instead of
rejecting the short transfer or clamping to the transferred bytes?

Also, the commit message states:

> scsi_report_lun_scan() allocates the REPORT LUNS buffer with kmalloc()
> and does not ask scsi_execute_cmd() for the residual count.  A device that
> completes the command successfully after transferring fewer than the eight
> bytes of the LUN list header leaves the header uninitialized; the bogus
> length read from it can trigger a multi-gigabyte kmalloc() retry or probe
> LUNs that do not exist.

Since scsi_execute_cmd() already zeroes scmd->resid_len bytes at the end of
the buffer whenever 0 < scmd->resid_len <= bufflen regardless of whether
args->resid is set, a reported residual does not leave lun_data
uninitialized. Conversely, if a host adapter driver does not report a
residual on a short transfer (leaving scmd->resid_len == 0), resid is 0 and
lun_data allocated with kmalloc() remains uninitialized. Should lun_data be
allocated with kzalloc() and the commit message updated to reflect how
scsi_execute_cmd() handles residuals?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2026110318534911091Mvzh5diLU9SKNWyne2p@zte.com.cn?part=1