Re: [PATCH v5 4/6] scsi: core: Add lock context annotations

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incomplete implementation of lock context annotations.

Pre-existing issues:
- [High] Integer overflow in `scsi_report_lun_scan` leading to infinite loop or out-of-bounds read.
--

--- Patch [4]: [PATCH v5 4/6] scsi: core: Add lock context annotations ---
commit f676281064957e5d166348bcd6e0996c11afa242
Author: Bart Van Assche <bvanassche@acm.org>

scsi: core: Add lock context annotations

This commit adds lock context annotations to document which functions expect
shost->scan_mutex to be held.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index d7e0e93931947..3b82e80e807a7 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
[ ... ]
> @@ -1444,6 +1446,7 @@ static void scsi_sequential_lun_scan(struct Scsi_Host *shost,
>  static int scsi_report_lun_scan(struct Scsi_Host *shost,
>  			struct scsi_target *starget, blist_flags_t bflags,
>  			enum scsi_scan_mode rescan)
> +	__must_hold(&shost->scan_mutex)
>  {

[Severity: High]
This is a pre-existing issue, but while looking at scsi_report_lun_scan()
there appears to be an integer overflow condition when handling untrusted
device input.

If a malicious device returns 0xFFFFFFFF as the length in a REPORT LUNS
response, the addition with sizeof(struct scsi_lun) can cause an overflow:

	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;
	}

On 64-bit platforms, size_t promotion causes the addition to evaluate to
0x100000007, which is greater than length. When this is assigned back to the
32-bit length variable, it truncates to 7. This causes the code to reallocate
a 7-byte buffer and retry indefinitely, leading to a deadlock on scan_mutex.

On 32-bit platforms, the addition wraps to 7, skipping the reallocation
entirely. The length variable stays 0xFFFFFFFF, which results in a massive
num_luns count and eventually an out-of-bounds read past the buffer.

Could we add a bounds check to ensure the reported length doesn't overflow
before we attempt to reallocate?

[ ... ]

[Severity: Low]
Are there other functions that should be included in this update? Looking
through the SCSI core code, there are a few other static and internal helpers
that rely on scan_mutex being held, such as scsi_sysfs_add_devices() in
scsi_scan.c:

static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
{
    ...
}

and __scsi_remove_device() in scsi_sysfs.c:

void __scsi_remove_device(struct scsi_device *sdev)
{
    ...
}

Would it be useful to add the __must_hold annotation to these functions as
well so static analysis tools can catch missing locks across the subsystem?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785965531.git.bvanassche@acm.org?part=4