Re: [PATCH v5 3/6] ata: sata_fsl: Fix use-after-free of host_priv on probe() failure

[Niklas Cassel <cassel@kernel.org>]

On Tue, Sep 15, 2026 at 03:50:50PM +0700, Damien Le Moal wrote:
> On 2026/09/15 15:41, Niklas Cassel wrote:
> > sata_fsl_host_stop() releases hcr_base and host_priv:
> > 
> > 	static void sata_fsl_host_stop(struct ata_host *host)
> > 	{
> > 		struct sata_fsl_host_priv *host_priv = host->private_data;
> > 
> > 		iounmap(host_priv->hcr_base);
> > 		kfree(host_priv);
> > 	}
> > 
> > ->host_stop() is called by the driver core through the ata_host_stop()
> > devres action which ata_host_start() registers, so it is called both when
> > the device is unbound and when probe() fails after the host has been
> > started.
> > 
> > The error_exit_with_cleanup label of sata_fsl_probe() releases hcr_base
> > and host_priv as well, and it is reachable from the two
> > device_create_file() calls which are done after the host has been
> > activated. In that case sata_fsl_host_stop() runs on an already freed
> > host_priv, resulting in a use-after-free and a double iounmap()/kfree().
> > 
> > Add a separate error label for the failures happening after the host has
> > been activated, which only detaches the host and leaves hcr_base and
> > host_priv to sata_fsl_host_stop().
> > 
> > Note that ata_host_detach() is dropped from error_exit_with_cleanup, as
> > that label is now only reachable while host is still NULL.
> > 
> > Fixes: 6c8ad7e8cf29 ("sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Looks good.
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> 
> One nit below that is worth fixing I think.
> 
> > +	/*
> > +	 * Once the host has been activated, hcr_base and host_priv are
> > +	 * released by sata_fsl_host_stop(), which is called by the driver core
> > +	 * through the ata_host_stop() devres action registered by
> > +	 * ata_host_start(). Releasing them here as well would result in a
> > +	 * double iounmap() and a use-after-free.
> > +	 */
> > +error_exit_detach:
> 
> The label should really go above the comment block to clearly show this is the
> explanation for what is done in this label block.

You are right, that seems to be the convention.

Will fixup when applying.


Kind regards,
Niklas