linux-kernel - Re: [PATCH net] pds_core: remove write-after-free of client

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250428192119.GI3339421@horms.kernel.org>
Date: Mon, 28 Apr 2025 20:21:19 +0100
From: Simon Horman <horms@...nel.org>
To: Shannon Nelson <shannon.nelson@....com>
Cc: andrew+netdev@...n.ch, brett.creeley@....com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net] pds_core: remove write-after-free of client_id

On Fri, Apr 25, 2025 at 01:38:57PM -0700, Shannon Nelson wrote:
> A use-after-free error popped up in stress testing:
> 
> [Mon Apr 21 21:21:33 2025] BUG: KFENCE: use-after-free write in pdsc_auxbus_dev_del+0xef/0x160 [pds_core]
> [Mon Apr 21 21:21:33 2025] Use-after-free write at 0x000000007013ecd1 (in kfence-#47):
> [Mon Apr 21 21:21:33 2025]  pdsc_auxbus_dev_del+0xef/0x160 [pds_core]
> [Mon Apr 21 21:21:33 2025]  pdsc_remove+0xc0/0x1b0 [pds_core]
> [Mon Apr 21 21:21:33 2025]  pci_device_remove+0x24/0x70
> [Mon Apr 21 21:21:33 2025]  device_release_driver_internal+0x11f/0x180
> [Mon Apr 21 21:21:33 2025]  driver_detach+0x45/0x80
> [Mon Apr 21 21:21:33 2025]  bus_remove_driver+0x83/0xe0
> [Mon Apr 21 21:21:33 2025]  pci_unregister_driver+0x1a/0x80
> 
> The actual device uninit usually happens on a separate thread
> scheduled after this code runs, but there is no guarantee of order
> of thread execution, so this could be a problem.  There's no
> actual need to clear the client_id at this point, so simply
> remove the offending code.
> 
> Fixes: 10659034c622 ("pds_core: add the aux client API")
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>

Thinking out loud:

My understanding is that after auxiliary_device_uninit() has
been called then pdsc_auxbus_dev_release(), which frees the memory
used by padev, will be called asynchronously.

And, as per the patch description, this usually happens after
the line deleted by this patch has been executed. But there are no
guarantees about that ordering. And sometimes it does not. *Boom*.

Based on my understanding above this patch looks good to me.

Reviewed-by: Simon Horman <horms@...nel.org>

> ---
>  drivers/net/ethernet/amd/pds_core/auxbus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index c9aac27883a3..92f359f2b449 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -186,7 +186,6 @@ void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf,
>  	pds_client_unregister(pf, padev->client_id);
>  	auxiliary_device_delete(&padev->aux_dev);
>  	auxiliary_device_uninit(&padev->aux_dev);
> -	padev->client_id = 0;
>  	*pd_ptr = NULL;
>  
>  	mutex_unlock(&pf->config_lock);
> -- 
> 2.17.1
> 
>