Re: [PATCH AUTOSEL 6.18-5.10] scsi: pm8001: Reject non-fatal dump when controller is crashed

[sashiko-bot@kernel.org]

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

New issues:
- [High] Returning -EINVAL from pm80xx_get_non_fatal_dump() is improperly handled by its caller, which truncates the negative error code to a large positive u32 integer, leading to severe sysfs corruption and kernel memory leak.

Pre-existing issues:
- [High] Preexisting buffer overflow in pm80xx_get_non_fatal_dump due to incorrect snprintf size parameter and trusting unvalidated hardware register values for loop bounds.
- [High] The sysfs callbacks for fatal and non-fatal dump collection access and modify shared driver state and hardware registers without holding any locks.
--

commit 88fed450e7290e833f0fc22f3e8782d89c4fea56
Author: Kumar Meiyappan <kumar.meiyappan@microchip.com>

scsi: pm8001: Reject non-fatal dump when controller is crashed

pm80xx_get_non_fatal_dump() can be called even after the controller has
entered a fatal error state. In that case the forensic memory contents
are not safe to access for a non-fatal dump request, and attempting to
do so can trigger a call trace.

Check controller_fatal_error before reading the non-fatal dump buffer
and return -EINVAL when the controller is already in a crashed state.

Link: https://patch.msgid.link/20260416154650.415624-1-sagar.biradar@microchip.com

> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 31960b72c1e92..1ce8226fcd292 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -401,6 +401,13 @@ ssize_t pm80xx_get_non_fatal_dump(struct device *cdev,
>  	char *buf_copy = buf;
>  
>  	temp = (u32 *)pm8001_ha->memoryMap.region[FORENSIC_MEM].virt_ptr;
> +
> +	if (pm8001_ha->controller_fatal_error) {
> +		pm8001_dbg(pm8001_ha, FAIL,
> +			   "non-fatal dump not available in fatal error state\n");
> +		return -EINVAL;

[Severity: High]
Does returning -EINVAL here cause a massive sysfs memory leak?

The caller of pm80xx_get_non_fatal_dump() in drivers/scsi/pm8001/pm8001_ctl.c
is non_fatal_log_show(), which stores the return value in a u32:

drivers/scsi/pm8001/pm8001_ctl.c:non_fatal_log_show() {
	u32 count;

	count = pm80xx_get_non_fatal_dump(cdev, attr, buf);
	return count;
}

When pm80xx_get_non_fatal_dump() returns -EINVAL, count becomes a large
positive integer (4294967274). This is returned to the sysfs core as a ssize_t.
Because the sysfs core sees a huge length, it caps it to PAGE_SIZE and copies
uninitialized kernel memory to userspace.

> +	}
> +
>  	if (++pm8001_ha->non_fatal_count == 1) {

[Severity: High]
This isn't a bug introduced by this patch, but does this sysfs read callback 
access shared driver state and hardware registers concurrently without holding
any locks?

Since sysfs read callbacks are not serialized, multiple threads reading the
non_fatal_log sysfs file concurrently can increment pm8001_ha->non_fatal_count
and execute multi-step MMIO sequences simultaneously.

This also seems to affect pm80xx_get_fatal_dump() which modifies hardware
registers without serialization:

drivers/scsi/pm8001/pm80xx_hwi.c:pm80xx_get_fatal_dump() {
	...
	pm8001_cw32(pm8001_ha, 0, MEMBASE_II_SHIFT_REGISTER,
			pm8001_ha->fatal_forensic_shift_offset);
	...
}

Could concurrent reads overwrite each other's hardware doorbell requests and
corrupt the forensic DMA setup?


[Severity: High]
This is a pre-existing issue, but is it possible for snprintf to write
out-of-bounds later in this function?

drivers/scsi/pm8001/pm80xx_hwi.c:pm80xx_get_non_fatal_dump() {
	...
	accum_len = pm8001_mr32(nonfatal_table_address,
			MPI_FATAL_EDUMP_TABLE_ACCUM_LEN);
	output_length = accum_len -
		pm8001_ha->forensic_preserved_accumulated_transfer;

	for (index = 0; index < output_length/4; index++)
		buf_copy += snprintf(buf_copy, PAGE_SIZE,
				"%08x ", *(temp+index));
	...
}

The loop dictates the number of iterations using accum_len, which is read
directly from the hardware register MPI_FATAL_EDUMP_TABLE_ACCUM_LEN.

If the hardware provides a large value, snprintf is statically passed PAGE_SIZE
on every iteration instead of the actual remaining buffer space
(PAGE_SIZE - (buf_copy - buf)). This allows snprintf to write past the end of
the single-page sysfs buffer, leading to a kernel heap buffer overflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-493-sashal@kernel.org?part=1