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

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

Pre-existing issues:
- [High] Ethtool operations can trigger a Use-After-Free on the admin queue memory if the device fails a reset or is torn down.
- [High] Temporary DMA buffer is freed on command timeout without canceling the hardware operation, leading to potential DMA-after-free memory corruption.
--

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

gve: add link status/speed ctrl ops
    
Refactor link status check to use a control plane op. Introduce
new op for retrieving the link status in AQ mode, and add an op
for retrieving link speed 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
> --- 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 isn't a bug introduced by this patch, but I wanted to ask about the
lifecycle of the admin queue here.

When the device is suspended or fails a reset, gve_teardown_device()
is called, which invokes gve_adminq_free(). This frees priv->adminq and
destroys the DMA pool, but leaves the priv->adminq pointer dangling.

If ethtool triggers gve_get_link_ksettings() while the device is in this
state, we unconditionally call report_link_speed(). This results in:

gve_get_link_ksettings()
  priv->ctrl_ops->report_link_speed() (gve_adminq_report_link_speed)
    gve_adminq_execute_cmd()
      gve_adminq_issue_cmd()

In gve_adminq_issue_cmd(), the freed priv->adminq buffer is accessed and
written to without checking gve_get_admin_queue_ok().

Can this lead to a use-after-free if ethtool accesses the link settings
during a failed reset or after device teardown?

[Severity: High]
This is also a pre-existing issue, but while looking at
gve_adminq_report_link_speed() being called here, I noticed a potential
DMA-after-free.

In gve_adminq_report_link_speed(), a temporary buffer is allocated via
dma_alloc_coherent() and its bus address is passed to the NIC. If
gve_adminq_execute_cmd() times out (for example, if
gve_adminq_kick_and_wait() returns -ENOTRECOVERABLE), the function
immediately calls dma_free_coherent() on the error path to free the region.

Since the hardware is not reset or instructed to abort the command before
the memory is freed, what happens if the hardware later wakes up and
executes the delayed command?

Will it DMA the 8-byte link speed into the freed memory, potentially
corrupting whatever structure now occupies that address?

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

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