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

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

[shao.mingyin]

scsi_report_lun_scan() reads the LUN list length from a buffer that was
allocated with kmalloc() and that the device may not have filled in
completely.  scsi_execute_cmd() zeroes the part of the buffer that the
host reported as not transferred, but only when the host reports a
residual at all, and it cannot tell whether a transfer that stopped in
the middle of the eight byte LUN list header left a usable length
field.

A host that completes the command without reporting a residual leaves
the kmalloc()ed contents in place, so stale heap data is parsed as a
length.  A host that reports the residual but stopped inside the header
leaves the bytes it did transfer to be read as that length, and neither
the retry allocation nor the number of descriptors to scan is then
bounded by what was actually transferred.

Allocate the buffer with kzalloc() so that a host which reports no
residual and transfers nothing cannot hand back stale heap data, and
ask for the residual so that a transfer which stopped inside the header
is rejected before its length is trusted.

The check requires the full eight byte LUN list header (the four byte
length plus the four reserved bytes), not just the length field, so a
transfer that stopped anywhere inside the header is rejected.  It also
rejects a length that cannot hold the header at all: the retry path
derives the length from the device-reported value with an addition that
can wrap below sizeof(struct scsi_lun), and the residual comparison
would then underflow.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
v2:
  - allocate the buffer with kzalloc() instead of kmalloc().
  - reword the commit message: scsi_execute_cmd() zeroes the residual
    tail, so the buffer is not simply left uninitialized.
  - keep the residual check, which is what catches a transfer that
    stopped inside the LUN list header.
  - also reject a length that cannot hold the LUN header, so the
    residual comparison cannot underflow when the device-reported
    length wraps on the retry path.
  - spell out that the check requires the full eight byte header.

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 52f3b525d2fd..e6aac7310537 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.
@@ -1516,7 +1517,7 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
 	 */
 	length = (511 + 1) * sizeof(struct scsi_lun);
 retry:
-	lun_data = kmalloc(length, GFP_KERNEL);
+	lun_data = kzalloc(length, GFP_KERNEL);
 	if (!lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
 		goto out;
@@ -1567,6 +1568,19 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
 		goto out_err;
 	}
 
+	/*
+	 * Reject a transfer that did not fill in the whole LUN list header.
+	 * Also reject a length that cannot even hold the header: the retry
+	 * path computes it from the device-reported value with an addition
+	 * that can wrap below sizeof(struct scsi_lun), in which case the
+	 * residual comparison below would underflow.
+	 */
+	if (length < sizeof(struct scsi_lun) ||
+	    resid > length - sizeof(struct scsi_lun)) {
+		ret = 1;
+		goto out_err;
+	}
+
 	/*
 	 * Get the length from the first four bytes of lun_data.
 	 */

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: core: length wrap check in scsi_report_lun_scan() fails on 32-bit
- [Medium] scsi: core: scsi_report_lun_scan() ignores resid for LUN list payload
--

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

scsi: core: reject short REPORT LUNS responses

This commit allocates the REPORT LUNS buffer in scsi_report_lun_scan() with
kzalloc() instead of kmalloc() and checks the transfer residual and buffer
length so that responses shorter than the eight-byte LUN list header are
rejected.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 52f3b525d2fda..e6aac73105375 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.
> @@ -1516,7 +1517,7 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  	 */
>  	length = (511 + 1) * sizeof(struct scsi_lun);
>  retry:
> -	lun_data = kmalloc(length, GFP_KERNEL);
> +	lun_data = kzalloc(length, GFP_KERNEL);
>  	if (!lun_data) {
>  		printk(ALLOC_FAILURE_MSG, __func__);
>  		goto out;
> @@ -1567,6 +1568,19 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  		goto out_err;
>  	}
>  
> +	/*
> +	 * Reject a transfer that did not fill in the whole LUN list header.
> +	 * Also reject a length that cannot even hold the header: the retry
> +	 * path computes it from the device-reported value with an addition
> +	 * that can wrap below sizeof(struct scsi_lun), in which case the
> +	 * residual comparison below would underflow.
> +	 */
> +	if (length < sizeof(struct scsi_lun) ||
> +	    resid > length - sizeof(struct scsi_lun)) {
> +		ret = 1;
> +		goto out_err;
> +	}

[Severity: High]
Does the length < sizeof(struct scsi_lun) check in scsi_report_lun_scan()
in drivers/scsi/scsi_scan.c work on 32-bit architectures, or happen early
enough on 64-bit architectures, when the device-reported LUN list length is
in the range 0xfffffff8..0xffffffff?

Immediately after this check, scsi_report_lun_scan() inspects the reported
length and decides whether to retry:

	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));
	...
	for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {

On 32-bit architectures, get_unaligned_be32(lun_data->scsi_lun) (u32),
sizeof(struct scsi_lun) (size_t, 8), and length (unsigned int) are all
32-bit unsigned values. If a target responds to the initial 4096-byte
REPORT LUNS command with a LUN list length in 0xfffffff8..0xffffffff, the
addition in the if condition wraps modulo 2^32 to 0..7. Since 0..7 > 4096
is false, the retry branch is not taken and the length < sizeof(struct
scsi_lun) check is never reached. Execution falls through with length set
to 0xfffffff8..0xffffffff and num_luns set to 0x1fffffff, causing the scan
loop to read far past the end of the 4096-byte lun_data buffer.

On 64-bit architectures, sizeof(struct scsi_lun) is a 64-bit size_t, so the
if condition evaluates to true without wrapping, but assigning the sum to
the 32-bit unsigned int length truncates it to 0..7 before jumping to
retry. Because the length < sizeof(struct scsi_lun) check is placed after
kzalloc() and scsi_execute_cmd(), the retry pass calls kzalloc(0..7,
GFP_KERNEL) (returning ZERO_SIZE_PTR when length is 0) and sends a bogus
0..7 byte REPORT LUNS command to the target before rejecting length.

Would it be better to check for overflow when adding sizeof(struct
scsi_lun) to get_unaligned_be32(lun_data->scsi_lun) (for example via
check_add_overflow()) before comparing against length and jumping to retry?

[Severity: Medium]
As reported by Sashiko on v1 [1], what happens in scsi_report_lun_scan() in
drivers/scsi/scsi_scan.c when a target transfers the full 8-byte LUN list
header (so resid <= length - sizeof(struct scsi_lun)), but stops
transferring before sending all of the LUN descriptors reported in that
header?

After the header residual check and retry check pass, scsi_report_lun_scan()
computes num_luns directly from the header value without checking or
bounding against the number of payload bytes actually transferred (length -
resid - sizeof(struct scsi_lun)):

	length = get_unaligned_be32(lun_data->scsi_lun);

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

If the transfer stopped mid-descriptor or before the end of the reported
list, the untransferred tail is zeroed by kzalloc() and scsi_execute_cmd().
Doesn't the scan loop then decode a partially transferred descriptor with
zeroed trailing bytes into a corrupted LUN number via scsilun_to_int(), and
decode all remaining untransferred descriptors as LUN 0?

[1] https://lore.kernel.org/r/sashiko-outbox-195103@kernel.org

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