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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential DMA use-after-free when an admin queue command times out.
--

commit fd1cf5f7dd6ca7a5afb5900e7042fea37b95574b
Author: Joshua Washington <joshwash@google.com>

gve: add link status/speed ctrl ops

This patch refactors the link status check and link speed retrieval
to use new control plane operations. It adds report_link_status and
report_link_speed to gve_ctrl_ops to handle these tasks in AQ mode.

> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index dd1c44fedc771..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: High]
This is a pre-existing issue, but is there a potential DMA use-after-free
when an admin queue command times out here?

In the implementation of gve_adminq_report_link_speed(), we allocate a DMA
buffer for the hardware to write into:

drivers/net/ethernet/google/gve/gve_adminq.c:gve_adminq_report_link_speed() {
...
	link_speed_region =
		dma_alloc_coherent(&priv->pdev->dev,
				   sizeof(*link_speed_region),
				   &link_speed_region_bus, GFP_KERNEL);
...
	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);
	return err;
}

If gve_adminq_execute_cmd() times out because the hardware is slow to
respond, it returns -ENOTRECOVERABLE. However, the driver unconditionally
frees the DMA buffer via dma_free_coherent(). 

If the hardware is not synchronously reset and eventually completes the
command, could it asynchronously write the 8-byte link speed to the freed
DMA address and corrupt memory? 

Since this path can be reached by an unprivileged user reading
/sys/class/net/ethX/speed, could this cause remote memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903215606.31633-1-hramamurthy@google.com?part=12