* Re: [PATCH] scsi: efct: validate firmware header size
2026-08-30 13:20 [PATCH] scsi: efct: validate firmware header size Pengpeng Hou
@ 2026-08-30 13:31 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-30 13:31 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 2+ messages in thread