Re: [PATCH] scsi: efct: validate firmware header size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the firmware write loop exits on signal interruption without canceling the pending asynchronous hardware operation, leading to DMA use-after-free and stack corruption.
- [High] This problem wasn't introduced by this patch, but missing timer synchronization before freeing xport leads to Use-After-Free.
- [High] This problem wasn't introduced by this patch, but there is unprotected traversal and modification of the global efct_devices list.
- [Medium] This problem wasn't introduced by this patch, but there is an information leak via out-of-bounds read due to missing null-termination when logging fw_image->revision.
--

commit 96d5d1eba778004d13f602bb0b32a97487f39774
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: efct: validate firmware header size
    
This commit adds a size check for the firmware blob before casting it to an
efct_hw_grp_hdr structure. This prevents accessing header fields out of bounds
on undersized firmware images.

> diff --git a/drivers/scsi/elx/efct/efct_driver.c b/drivers/scsi/elx/efct/efct_driver.c
> index 07c2f453459e1..b0d8ca58daebe 100644
> --- a/drivers/scsi/elx/efct/efct_driver.c
> +++ b/drivers/scsi/elx/efct/efct_driver.c
> @@ -338,6 +338,10 @@ efct_request_firmware_update(struct efct *efct)
>  		efc_log_debug(efct, "Firmware file(%s) not found.\n", file_name);
>  		return rc;
>  	}
> +	if (fw->size < sizeof(*fw_image)) {
> +		rc = -EINVAL;
> +		goto exit;
> +	}
>  
>  	fw_image = (struct efct_hw_grp_hdr *)fw->data;
>  

[Severity: Medium]
This is a pre-existing issue, but since we are validating the firmware
image here, is there a risk of an out-of-bounds read when logging the
revision?

In efct_request_firmware_update(), fw_image->revision is printed using
the %s format specifier:

	efc_log_info(efct, "Firmware update is initiated. %s -> %s\n",
		     efct->hw.sli.fw_name[0], fw_image->revision);

Since fw_image->revision is a 32-byte fixed array from an external file,
it may not be null-terminated. Will vsnprintf read past the array
boundaries into adjacent memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the firmware write
path handled later in this function suffer from a use-after-free if
interrupted?

When efct_request_firmware_update() calls efct_firmware_write(), the
write loop allocates a DMA buffer and completion struct on the stack,
then waits for completion:

	if (wait_for_completion_interruptible(&result.done) != 0) {
		rc = -ENXIO;
		break;
	}

If interrupted by a signal, the loop breaks, frees the DMA buffer, and
the function returns, destroying its stack frame.

Could the asynchronous hardware callback efct_fw_write_cb() then execute
and write to the freed stack pointer?

	result->status = status;
	result->actual_xfer = actual_write_length;
	result->change_status = change_status;

	complete(&result->done);

Does the hardware operation need to be canceled before returning?

[Severity: High]
This is a pre-existing issue, but does the firmware reset path invoked
at the end of this function safely clean up timers?

When efct_request_firmware_update() triggers efct_fw_reset(), it deletes
the timer unsafely:

	if (timer_pending(&efct->xport->stats_timer))
		timer_delete(&efct->xport->stats_timer);

A similar pattern exists during general device teardown in
efct_xport_detach():

	if (timer_pending(&xport->stats_timer))
		timer_delete(&xport->stats_timer);

Because timer_delete() is used instead of timer_delete_sync(), could the
timer callback efct_xport_stats_timer_cb() be running concurrently on
another CPU?

If the teardown path then proceeds to free the xport object, could the
concurrently running callback access freed memory or re-arm the timer,
causing memory corruption?

[Severity: High]
This problem wasn't introduced by this patch, but is the global
efct_devices list protected against concurrent access during configfs
operations?

Devices are added and removed from the list during PCI probe and
teardown without holding any locks:

drivers/scsi/elx/efct/efct_driver.c:efct_device_alloc() {
	...
	list_add_tail(&efct->list_entry, &efct_devices);
	...
}

drivers/scsi/elx/efct/efct_driver.c:efct_device_free() {
	...
	list_del(&efct->list_entry);
	...
}

Meanwhile, LIO configfs operations traverse the list unprotected:

drivers/scsi/elx/efct/efct_lio.c:efct_find_wwpn() {
	...
	list_for_each_entry(efct, &efct_devices, list_entry) {
		if (wwpn == efct_get_wwpn(&efct->hw))
			return efct;
	}
	...
}

If a device is hot-unplugged or unbound while a configfs operation is
traversing the list, could this result in a data race reading corrupted
list pointers or freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830132051.4441-1-pengpeng@iscas.ac.cn?part=1