Re: [PATCHv2 4/8] ata: sata_dwc_460ex: drop redundant struct copy of port_info

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

Pre-existing issues:
- [High] Returning success (0) on `ata_host_activate()` failure causes a kernel panic on device removal.
- [High] Missing cleanup of DMA resources on probe error paths leads to Use-After-Free.
--

commit 1231cd1ac0d6ac3ad6a4b61a9f9cf20df21198d9
Author: Rosen Penev <[email protected]>
Subject: ata: sata_dwc_460ex: drop redundant struct copy of port_info

This patch optimizes sata_dwc_probe() by dropping an unnecessary local
copy of sata_dwc_port_info[0]. It directly points to the static data
instead, saving stack space and correcting the const qualification.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..f262b00384ac4 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1134,8 +1134,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	int err = 0;
>  	int irq;
>  	struct ata_host *host;
> -	struct ata_port_info pi = sata_dwc_port_info[0];
> -	const struct ata_port_info *ppi[] = { &pi, NULL };
> +	const struct ata_port_info *pi = &sata_dwc_port_info[0];
> +	const struct ata_port_info *ppi[] = { pi, NULL };
>  	struct resource *res;
>  
>  	/* Allocate DWC SATA device */

[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() correctly handle
ata_host_activate() failures?

Looking further down in drivers/ata/sata_dwc_460ex.c:sata_dwc_probe(), the
driver logs an error but returns 0 if activation fails:

	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
	if (err)
		dev_err(dev, "failed to activate host");

	return 0;

If ata_host_activate() fails (for example, if devm_request_irq() or
scsi_host_alloc() fails), returning 0 falsely reports successful
initialization to the driver core.

When the device is later unbound or removed, sata_dwc_remove() is invoked
and calls ata_host_detach() which calls ata_port_detach(). Since the
scsi_host was never properly allocated or was freed, scsi_remove_host()
will dereference a null pointer and cause a kernel panic.

Could the return statement be updated to return the error code instead?


[Severity: High]
This is a pre-existing issue, but does sata_dwc_probe() leak dma resources
on error paths?

If CONFIG_SATA_DWC_OLD_DMA is enabled and no "dmas" property is present,
sata_dwc_dma_init_old() is called:

	err = sata_dwc_dma_init_old(ofdev, hsdev);
	if (err)
		return err;

This allocates a dma controller via devm_kzalloc, requests an interrupt, and
registers it with the dma engine subsystem.

If a subsequent probe step fails, such as the phy retrieval:

	if (IS_ERR(hsdev->phy))
		return PTR_ERR(hsdev->phy);

or later in the main error path:

error_out:
	phy_exit(hsdev->phy);
	return err;

The function returns without calling sata_dwc_dma_exit_old(). The driver core
will free the devm-managed memory, including the dma controller structures.
However, because explicit teardown was bypassed, the dma engine subsystem and
the interrupt layer will retain pointers to this freed memory. This can lead
to a use-after-free if the shared interrupt fires or the dma subsystem
accesses the controller.

Should the error paths be updated to call sata_dwc_dma_exit_old() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1