Re: [PATCH] scsi: wd719x: initialize all fields of eeprom_93cx6 struct

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The commit fixes an issue in wd719x.c where allocating `struct eeprom_93cx6` on the stack without initialization leaves the newly added `quirk` field undefined, leading to random EEPROM read corruption. However, it misses several other drivers (such as ax88796, rtl8180, rtl8187, and rt2400pci) that also allocate `struct eeprom_93cx6` on the stack and only perform element-wise assignment of some fields, leaving `quirk` uninitialized stack garbage.
- [High] In `wd719x_chip_init()`, if the firmware is reloaded during a host reset, `wd->fw_size` is updated with the size of the newly loaded firmware files, but `wd->fw_virt` is not reallocated if it is already non-NULL. If the new firmware is larger than the original one, the subsequent `memcpy` operations will overflow the previously allocated DMA coherent buffer.
- [High] When `wd719x_queuecommand()` returns `SCSI_MLQUEUE_HOST_BUSY` due to the AMR command register being busy, it fails to clean up the newly allocated SCB and DMA mappings. This leaks DMA handles and leaves the SCB in `wd->active_scbs`. On retry, a new SCB is allocated for the same command. During a host reset, `wd719x_host_reset()` iterates over `active_scbs` and completes commands, which causes a double-completion and use-after-free on the same `scsi_cmnd`.
- [High] In `wd719x_interrupt()`, the `list_for_each_entry` iterator `scb` is evaluated outside the loop to check if a match was found. If the list is empty or no match is found, `scb` points to an invalid structure computed from the list head. Dereferencing `scb->phys` results in an out-of-bounds memory access. If it happens to match `SCB_out`, it leads to processing a bogus SCB.
--

--- Patch [1]: [PATCH] scsi: wd719x: initialize all fields of eeprom_93cx6 struct ---
commit 7b28e726009ab6205dc4b45d7a3c7506d55ae0e3
Author: Stanislaw Gruszka <stf_xl@wp.pl>

scsi: wd719x: initialize all fields of eeprom_93cx6 struct
    
Commit 7738a7ab9d12 ("misc: eeprom: eeprom_93cx6: Add quirk for extra
read clock cycle") added extra quirk field to struct eeprom_93cx6,
which changed how the data is read.
    
Some existing users of eeprom_93cx6, including the wd719x driver, allocate
the structure on the stack without initialization of all the fields.
As a result, the added quirk field has an undefined value, which can
randomly cause reading wrong data from the EEPROM.

> diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
> index e3da1a2fdb666..31e0fe9eb93cb 100644
> --- a/drivers/scsi/wd719x.c
> +++ b/drivers/scsi/wd719x.c
> @@ -731,15 +731,15 @@ static void wd719x_eeprom_reg_write(struct eeprom_93cx6 *eeprom)
>  /* read config from EEPROM so it can be downloaded by the RISC on (re-)init */
>  static void wd719x_read_eeprom(struct wd719x *wd)
>  {
> -	struct eeprom_93cx6 eeprom;
> +	struct eeprom_93cx6 eeprom = {
> +		.data = wd,
> +		.register_read = wd719x_eeprom_reg_read,
> +		.register_write = wd719x_eeprom_reg_write,
> +		.width = PCI_EEPROM_WIDTH_93C46,
> +	};

[Severity: High]
This isn't a bug introduced by this patch, but it looks like there are other
drivers allocating struct eeprom_93cx6 on the stack without initializing the
quirk field. Should this fix also be applied to ax_init_dev() in
drivers/net/ethernet/8390/ax88796.c, rtl8180_eeprom_read() in
drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c, rtl8187_probe() in
drivers/net/wireless/realtek/rtl818x/rtl8187/dev.c, and
rt2400pci_validate_eeprom() in drivers/net/wireless/rt2x00/rt2400pci.c?

For example, in ax_init_dev():

drivers/net/ethernet/8390/ax88796.c:ax_init_dev() {
    ...
    struct eeprom_93cx6 eeprom;
    ...
}

[Severity: High]
This is a pre-existing issue, but does the firmware reallocation logic in this
driver potentially cause a buffer overflow?

If a host reset occurs after the firmware file on disk is replaced with a
larger one, request_firmware() is called again and wd->fw_size is updated,
but the buffer reallocation is skipped:

drivers/scsi/wd719x.c:wd719x_chip_init() {
    ...
    if (!wd->fw_virt)
        wd->fw_virt = pci_alloc_consistent(wd->pdev, wd->fw_size,
                                           &wd->fw_phys);
    ...
}

The subsequent memcpy() operations then write the new larger firmware to the
old buffer, which could overflow the previously allocated DMA coherent buffer:

drivers/scsi/wd719x.c:wd719x_chip_init() {
    ...
    memcpy(wd->fw_virt, fw_wcs->data, fw_wcs->size);
    memcpy(wd->fw_virt + ALIGN(fw_wcs->size, 4), fw_risc->data,
           fw_risc->size);
    ...
}

[Severity: High]
This is a pre-existing issue, but does returning SCSI_MLQUEUE_HOST_BUSY here
leak DMA handles and the SCB?

drivers/scsi/wd719x.c:wd719x_queuecommand() {
    ...
    if (wd719x_readb(wd, WD719X_AMR_COMMAND) != WD719X_CMD_READY) {
        spin_unlock_irqrestore(wd->sh->host_lock, flags);
        return SCSI_MLQUEUE_HOST_BUSY;
    }
    ...
}

If the host is busy, the function returns without cleaning up the newly
allocated SCB and DMA mappings, leaving the SCB in wd->active_scbs.

On retry, a new SCB is allocated for the same command. During a host reset,
iterating over active_scbs and completing commands could cause a
double-completion and use-after-free on the same scsi_cmnd.

[Severity: High]
This is a pre-existing issue, but does this list iterator usage result in an
out-of-bounds access?

drivers/scsi/wd719x.c:wd719x_interrupt() {
    ...
    list_for_each_entry(scb, &wd->active_scbs, list) {
        // ...
    }

    if (SCB_out == scb->phys)
        wd719x_interrupt_SCB(wd, regs, scb);
    ...
}

If the list is empty or no match is found, the scb iterator points to an
invalid structure computed from the list head. Dereferencing scb->phys results
in an out-of-bounds memory access.

If the garbage value matches SCB_out, it could lead to processing a bogus SCB.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724081747.7726-1-stf_xl@wp.pl?part=1