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

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

[shao.mingyin]

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.

Request the residual and abort the scan when the header was not
transferred.

Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 52f3b525d2fd..abbcf529bdc4 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;
+	}
+
 	/*
 	 * Get the length from the first four bytes of lun_data.
 	 */

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

[sashiko-bot]

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

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

[shao.mingyin]

Thanks, you are right about scsi_execute_cmd() zeroing the residual
tail, and my changelog overstated it: with a host that reports a
residual the header is not left uninitialized.  What the check does
catch is a transfer that stops inside the eight byte LUN list header,
where the bytes that did arrive are read as the LUN list length.

For a host that reports no residual, resid stays zero and the check
cannot fire, so v2 will allocate the buffer with kzalloc() and describe
the residual handling accurately.  I'll send it tomorrow.