Re: [PATCH net-next v4 15/15] gve: add link status/speed ctrl ops

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

Pre-existing issues:
- [Critical] DMA Use-After-Free due to missing device reset on Admin Queue command timeout.
--

--- Patch [15]: [PATCH net-next v4 15/15] gve: add link status/speed ctrl ops ---
commit bafe15595d2df2fbd631b0f3f40244e39beaf6e5
Author: Joshua Washington <[email protected]>

gve: add link status/speed ctrl ops

This commit refactors the link status check to use a control plane op and
introduces a new op for retrieving the link status in AQ mode from the device
status register. It also adds an op for retrieving link speed in AQ mode which
calls into gve_adminq_report_link_speed.

> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index 8199738ba979e..a3220c6514382 100644
> --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c

[ ... ]

> @@ -749,12 +749,11 @@ static int gve_get_link_ksettings(struct net_device *netdev,
>  	int err = 0;
>  
>  	if (priv->link_speed == 0)
> -		err = gve_adminq_report_link_speed(priv);
> +		err = priv->ctrl_ops->report_link_speed(priv);

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing the use of
the report_link_speed control operation, I noticed a pre-existing
regression when handling Admin Queue timeouts.

Could the underlying gve_adminq_report_link_speed() function corrupt memory
if the device responds late?

In drivers/net/ethernet/google/gve/gve_adminq.c,
gve_adminq_report_link_speed() unconditionally frees the DMA region:

    err = gve_adminq_execute_cmd(priv, &gvnic_cmd);

    priv->link_speed = be64_to_cpu(*link_speed_region);
    dma_free_coherent(&priv->pdev->dev, sizeof(*link_speed_region),
                      link_speed_region, link_speed_region_bus);

If the hardware is delayed and gve_adminq_execute_cmd() returns a timeout,
the DMA memory is freed without synchronously resetting or quiescing the
device.

If the hardware later completes the command, could it write the link speed
into the now-freed DMA address and corrupt newly allocated memory?

A similar unconditional free after timeout happens in
gve_adminq_execute_extended_cmd():

    err = gve_adminq_execute_cmd(priv, &cmd);

    dma_free_coherent(&priv->pdev->dev, cmd_size, inner_cmd, inner_cmd_bus);

Since an unprivileged user can trigger this path via ethtool, does the
hardware need to be reset before freeing these DMA buffers, or should the
buffers be leaked until a reset is guaranteed?

>  
>  	cmd->base.speed = priv->link_speed;
>  
>  	cmd->base.duplex = DUPLEX_FULL;
> -
>  	return err;
>  }

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