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

[Damien Le Moal <dlemoal@kernel.org>]

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.

> +	ata_host_detach(host);
>  
> -	if (host)
> -		ata_host_detach(host);
> +	return retval;
>  
> +error_exit_with_cleanup:
>  	if (hcr_base)
>  		iounmap(hcr_base);
>  	kfree(host_priv);


-- 
Damien Le Moal
Western Digital Research