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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] An integer overflow in the calculation of the REPORT LUNS response length allows a malicious or malformed device to cause an infinite loop or kernel crash.
--

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

scsi: core: Add lock context annotations

This commit documents which functions expect that shost->scan_mutex is 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)
>  {
>  	unsigned char scsi_cmd[MAX_COMMAND_SIZE];
>  	unsigned int length;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at this function,
I noticed a potential integer overflow when calculating the response length:

drivers/scsi/scsi_scan.c:scsi_report_lun_scan() {
    ...
	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;
	}
    ...
}

If a malicious or malformed device returns a large value like 0xFFFFFFFF
in the first 4 bytes of its response, can the addition of
sizeof(struct scsi_lun) overflow?

On 64-bit systems, would this addition yield a 64-bit value that bypasses
the greater-than length check, but then truncate when assigned back to the
32-bit unsigned int length variable?  Could this cause an infinite retry
loop or an invalid pointer dereference if it truncates to zero?

On 32-bit systems, could the addition directly overflow to a small value,
fail the greater-than length check, and proceed to parse out-of-bounds luns
from the initially allocated buffer?

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