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

[sashiko-bot@kernel.org]

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