Re: [PATCH net-next v4 01/15] gve: don't pass in unused parameter to gve_adminq_free

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

Pre-existing issues:
- [High] Workqueue is destroyed after freeing resources in gve_remove, leading to resource leaks and UAF if a reset is pending.
- [High] Hardware device resources are not deconfigured before DMA memory is freed in the gve_setup_device_resources error path.
--

--- Patch [1]: [PATCH net-next v4 01/15] gve: don't pass in unused parameter to gve_adminq_free ---
commit 9a4a2e123b2090a5f84279c50fa3476f0a484a87
Author: Harshitha Ramamurthy <[email protected]>

gve: don't pass in unused parameter to gve_adminq_free
    
Clean up gve_adminq_free to not take in an unused parameter.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index e4d78ae52dafe..30bf6df4ebc58 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -2506,14 +2506,14 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
>  	bitmap_free(priv->xsk_pools);
>  	priv->xsk_pools = NULL;
>  err:
> -	gve_adminq_free(&priv->pdev->dev, priv);
> +	gve_adminq_free(priv);
>  	return err;
>  }

[Severity: High]
This is a pre-existing issue, but does the error path in
gve_setup_device_resources() leave hardware configured with freed DMA
addresses?

If an initialization step fails after configuring the hardware, the error path
frees the DMA memory without deconfiguring or resetting the hardware:

drivers/net/ethernet/google/gve/gve_main.c:gve_setup_device_resources() {
    ...
    err = gve_adminq_configure_device_resources(...);
    ...
abort_with_ntfy_blocks:
	gve_free_notify_blocks(priv);
abort_with_counter:
	gve_free_counter_array(priv);
    ...
}

Can this allow the hardware to asynchronously write to the freed DMA addresses
until it is finally reset later by the gve_adminq_free() call in
gve_init_priv(), causing a use-after-free?

>  static void gve_teardown_priv_resources(struct gve_priv *priv)
>  {
>  	gve_teardown_device_resources(priv);
> -	gve_adminq_free(&priv->pdev->dev, priv);
> +	gve_adminq_free(priv);
>  	bitmap_free(priv->xsk_pools);
>  	priv->xsk_pools = NULL;
>  }

[Severity: High]
This is a pre-existing issue, but can the sequence in gve_remove() lead to a
resource leak and use-after-free if a reset is pending?

In gve_remove(), teardown is performed before the workqueue is destroyed, and
the service task is not cancelled:

drivers/net/ethernet/google/gve/gve_main.c:gve_remove() {
    ...
	gve_teardown_priv_resources(priv);
	destroy_workqueue(priv->gve_wq);
	free_netdev(netdev);
    ...
}

If a device reset is requested or handled by gve_service_task() concurrently
with module removal:

1. unregister_netdev() drops the rtnl_lock.
2. The pending gve_service_task() acquires the lock and allocates new device
   resources during reset recovery.
3. Concurrently, gve_remove() calls gve_teardown_priv_resources() without
   rtnl_lock, racing with the reset teardown/recovery.
4. destroy_workqueue() waits for the service task to complete.
5. gve_remove() calls free_netdev().

Are the newly allocated resources from the reset recovery leaked, leaving the
hardware active to DMA into freed structures?

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