Re: [PATCH v2 3/4] scsi: ipr: use kmalloc() to allocate IPR dump buffer memory

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The sysfs bin attribute read handler `ipr_read_dump` can enter an infinite loop in the kernel when reading from the `ioa_data` section at a page-aligned logical offset.
- [High] The sysfs dump read handler (`ipr_read_dump`) uses an untrusted `num_entries_used` value from the adapter firmware to calculate `sdt_end`, allowing an out-of-bounds read past the end of the `ipr_dump` structure.
--

commit c5fdd71b8661a08dfced04b150e6d5fc217eacda
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

scsi: ipr: use kmalloc() to allocate IPR dump buffer memory

This commit replaces the use of __get_free_page() with kmalloc() for
allocating IPR dump buffers. It also relaxes the allocation flag from
GFP_ATOMIC to GFP_NOIO since the allocations happen in a workqueue context.

Link: https://lore.kernel.org/all/635405e4-9423-4a25-a6e7-e03c8ea0bcbe@redhat.com

> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index d207e5e81afe0..19153dd247363 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
[ ... ]
> @@ -3226,7 +3226,7 @@ static void ipr_release_dump(struct kref *kref)
>  	spin_unlock_irqrestore(ioa_cfg->host->host_lock, lock_flags);
>  
>  	for (i = 0; i < dump->ioa_dump.next_page_index; i++)
> -		free_page((unsigned long) dump->ioa_dump.ioa_data[i]);
> +		kfree(dump->ioa_dump.ioa_data[i]);
>  
>  	vfree(dump->ioa_dump.ioa_data);
>  	kfree(dump);

[Severity: High]
This isn't a bug introduced by this patch, but the sysfs dump read
handler ipr_read_dump() in drivers/scsi/ipr.c uses an untrusted 
num_entries_used value from the adapter firmware to calculate sdt_end:

drivers/scsi/ipr.c:ipr_read_dump() {
    ...
	sdt_end = offsetof(struct ipr_ioa_dump, sdt.entry) +
			  (be32_to_cpu(dump->ioa_dump.sdt.hdr.num_entries_used) *
			   sizeof(struct ipr_sdt_entry));
    ...
	src = (u8 *)&dump->ioa_dump + off;
	memcpy(buf, src, len);
    ...
}

While ipr_get_ioa_dump() caps the local variable num_entries, it leaves
the raw dump->ioa_dump.sdt.hdr.num_entries_used in the struct unchanged.
When ipr_read_dump() calculates sdt_end using this raw uncapped value,
it allows an out-of-bounds read past the end of the ipr_dump structure.

Could this lead to a kernel heap exposure when the adjacent kernel memory
is copied to user-space?

[Severity: High]
This is a pre-existing issue, but the sysfs bin attribute read handler
ipr_read_dump() in drivers/scsi/ipr.c can enter an infinite loop when
reading from the ioa_data section at a page-aligned logical offset:

drivers/scsi/ipr.c:ipr_read_dump() {
    ...
	if ((off & PAGE_MASK) != ((off + count) & PAGE_MASK))
		len = PAGE_ALIGN(off) - off;
    ...
}

If a user reads the sysfs dump file such that off starts at a page
boundary and count extends to or past the next page boundary (e.g.,
off = 0 and count = 4096), PAGE_ALIGN(off) - off evaluates to 0.

This results in a zero-length memcpy and no advancement of off or count.
Since there is no cond_resched(), could this lock up a CPU core in an
infinite loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704-b4-scsi-v2-0-7d2d21a810de@kernel.org?part=3