Re: [PATCH] scsi: sd: bound the IO hints descriptor walk to the buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The stack-allocated `buf` in `sd_is_perm_stream()` is not zero-initialized, and its residual length is not checked after `scsi_execute_cmd()`, leading to a use of uninitialized stack memory.
- [Medium] The loop bounds calculation double-counts the mode header length and relies on an unaligned ALIGN_DOWN relative to the buffer, causing the loop to evaluate a truncated descriptor that overhangs the allocated buffer, and generate an out-of-bounds pointer (Undefined Behavior).
--

--- Patch [1]: [PATCH] scsi: sd: bound the IO hints descriptor walk to the buffer ---
commit 536deb268b96e3b10a547fc2087216469f8c3827
Author: Michael Bommarito <michael.bommarito@gmail.com>

scsi: sd: bound the IO hints descriptor walk to the buffer

This commit attempts to fix an out-of-bounds read in sd_read_io_hints() by
clamping the device-reported mode data length to SD_BUF_SIZE. This prevents
the descriptor walk from reading past the buffer when parsing permanent-stream
IO hints.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 599e75f333343..eec383cbc39f1 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3304,6 +3304,7 @@ static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_mode_data data;
>  	int res;
> +	u32 len;
>  
>  	if (sdp->sdev_bflags & BLIST_SKIP_IO_HINTS)
>  		return;
> @@ -3313,9 +3314,17 @@ static void sd_read_io_hints(struct scsi_disk *sdkp, unsigned char *buffer)
>  			      sdkp->max_retries, &data, &sshdr);
>  	if (res < 0)
>  		return;
> +	/*
> +	 * The device-reported mode data length can exceed the buffer that
> +	 * was actually transferred; clamp it so the descriptor walk stays
> +	 * within buffer[SD_BUF_SIZE].
> +	 */
> +	if (data.length > SD_BUF_SIZE - data.header_length)
> +		len = SD_BUF_SIZE;
> +	else
> +		len = data.header_length + data.length;
>  	start = (void *)buffer + data.header_length + 16;
> -	end = (void *)buffer + ALIGN_DOWN(data.header_length + data.length,
> -					  sizeof(*end));
> +	end = (void *)buffer + ALIGN_DOWN(len, sizeof(*end));

[Severity: Medium]
This isn't a bug introduced by this patch, but does this calculation
double-count the mode header length and result in an overhanging pointer?

Looking at scsi_mode_sense(), it populates data.length with the total length
of the mode parameter list, which already includes the header. Computing
len = data.header_length + data.length overestimates the valid data size by
8 bytes.

Additionally, if len is clamped to SD_BUF_SIZE (512) and start is at offset
24, the descriptors (16 bytes each) fall at offsets 24, 40, ..., 504. Because
end is calculated as buffer + ALIGN_DOWN(len, sizeof(*end)), end will be
buffer + 512.

The loop condition desc < end will evaluate to true for the descriptor at
offset 504 (since 504 < 512). This descriptor spans bytes 504-519,
overhanging the 512-byte buffer limit. The loop evaluation of desc++ will
also produce a pointer past the end of the buffer.

>  	/*
>  	 * From "SBC-5 Constrained Streams with Data Lifetimes": Device severs
>  	 * should assign the lowest numbered stream identifiers to permanent
>  	 * streams.
>  	 */
>  	for (desc = start; desc < end; desc++)
>  		if (!desc->st_enble || !sd_is_perm_stream(sdkp, desc - start))

[Severity: High]
This is a pre-existing issue, but does sd_is_perm_stream() use uninitialized
stack memory if the device returns a short transfer?

Looking at sd_is_perm_stream():

drivers/scsi/sd.c:sd_is_perm_stream() {
...
	struct {
		struct scsi_stream_status_header h;
		struct scsi_stream_status s;
	} buf;
...
	res = scsi_execute_cmd(sdev, cdb, REQ_OP_DRV_IN, &buf, sizeof(buf),
			       SD_TIMEOUT, sdkp->max_retries, &exec_args);
...
	if (get_unaligned_be32(&buf.h.len) < sizeof(struct scsi_stream_status))
		return false;
	return buf.s.perm;
}

The stack-allocated buf is not zero-initialized. If a device completes the
command with a GOOD status but transfers fewer bytes than requested (for
example, only the 8-byte header), scsi_execute_cmd() does not check the
residual length here.

If the device-controlled buf.h.len field is large enough to pass the check,
the function will evaluate and return buf.s.perm, leaking uninitialized
kernel stack memory into the disk's permanent stream configuration.

>  			break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711150736.2917641-1-michael.bommarito@gmail.com?part=1