Re: [PATCH net] pds_core: remove write-after-free of client_id

[Simon Horman <horms@kernel.org>]

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@amd.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@kernel.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
> 
>