Re: [PATCH net v3 3/4] gve: Block invalid ring sizes when device options are 0

[Pin-yen Lin]

On Tue, Oct 27, 2026 at 9:42 AM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] In gve_parse_device_option()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:194-197), the new…
>
> Pre-existing issues:
> - [Low] gve_parse_device_option() (gve_adminq.c:177-197) accepts
>   MODIFY_RING options with option_length 9-11, because it rejects only <…
>
> --
>
> gve: Block invalid ring sizes when device options are 0
>
> When the MODIFY_RING device option reports a minimum RX or TX ring size
> of 0, this patch makes gve_parse_device_option() fall back to the
> driver's default minimum ring sizes. The goal is to stop ethtool -G from
> accepting ring sizes small enough to crash gve_tx_alloc_rings_dqo().
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 8b215fca9c1b6..2c848f0c23dc2 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -191,7 +191,9 @@ void gve_parse_device_option(struct gve_priv *priv,
> >               *dev_op_modify_ring = (void *)(option + 1);
> >
> >               /* device has not provided min ring size */
> > -             if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> > +             if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
> > +                 be16_to_cpu((*dev_op_modify_ring)->min_rx_ring_size) == 0 ||
> > +                 be16_to_cpu((*dev_op_modify_ring)->min_tx_ring_size) == 0)
> >                       priv->device_info.default_min_ring_size = true;
>
> [Severity: Low]
> If only one of these two fields is 0, should the other minimum from the
> device be kept?
>
> Both directions share the single default_min_ring_size flag. When it is
> set, gve_enable_supported_features() replaces both minimums:
>
> drivers/net/ethernet/google/gve/gve_adminq.c:gve_enable_supported_features() {
>     ...
>                 if (info->default_min_ring_size) {
>                         /* If device hasn't provided minimums, use default minimums */
>                         info->min_tx_ring_size = GVE_DEFAULT_MIN_TX_RING_SIZE;
>                         info->min_rx_ring_size = GVE_DEFAULT_MIN_RX_RING_SIZE;
>                 } else {
>     ...
> }
>
> Take a device that reports min_rx_ring_size = 1024 and
> min_tx_ring_size = 0. Its RX minimum would become
> GVE_DEFAULT_MIN_RX_RING_SIZE (512), not 1024. Before this patch, the
> non-zero value was used.
>
> gve_set_desc_cnt() copies these values into priv->min_rx_desc_cnt and
> priv->min_tx_desc_cnt. Those are the only lower bounds that
> gve_validate_req_ring_size() checks:
>
> drivers/net/ethernet/google/gve/gve_ethtool.c:gve_validate_req_ring_size() {
>     ...
>         if (new_tx_desc_cnt < priv->min_tx_desc_cnt ||
>             new_tx_desc_cnt > priv->max_tx_desc_cnt ||
>             new_rx_desc_cnt < priv->min_rx_desc_cnt ||
>     ...
> }
>
> Would ethtool -G rx 512 then pass validation and go to the device, even
> though it is below the minimum the device reported?
>
> A per-field fallback would use the default only for the field that is 0.
> Could that fix the zero-minimum crash and still keep the other limit from
> the device? For older devices that report both minimums as 0, as the
> commit message describes, the two approaches behave the same.

The gvnic device either reports both minimums or both as zero, so the
per-field fallback would behave the same in practice.
>
> [Severity: Low]
> This is a pre-existing issue, but can these new reads go past the end of
> the option payload when option_length is 9, 10 or 11?
>
> The length check earlier in this case rejects only lengths below
> GVE_DEVICE_OPTION_NO_MIN_RING_SIZE (8):
>
> drivers/net/ethernet/google/gve/gve_adminq.c:gve_parse_device_option() {
>     ...
>         case GVE_DEV_OPT_ID_MODIFY_RING:
>                 if (option_length < GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
>                     req_feat_mask != GVE_DEV_OPT_REQ_FEAT_MASK_MODIFY_RING) {
>     ...
> }
>
> min_rx_ring_size and min_tx_ring_size sit at payload offsets 8 and 10,
> and sizeof(struct gve_device_option_modify_ring) is 12. With a length of
> 9 to 11, part of each read comes from the next option header or from
> unused descriptor space.
>
> gve_get_next_option() only checks option_length against the
> device-provided total_length. Also, total_length is never checked
> against the GVE_ADMINQ_BUFFER_SIZE allocation.
>
> If those out-of-option bytes are non-zero, the fallback does not fire
> and the bytes are used as the device minimums.
>
> The baseline gve_enable_supported_features() already read the same
> fields in this case, because default_min_ring_size stayed false. So this
> patch does not add any new reachable impact.
>
> Would checking option_length < sizeof(**dev_op_modify_ring), instead of
> comparing with == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE, close this?

This is pre-existing and not changed by this patch. The gvnic device
sends this option with a length of either 8 or 12.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023204706.3112215-1-hramamurthy%40google.com

Regards,
Pin-yen