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

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <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] The MODIFY_RING option is accepted whenever option_length >= 8.
>
> --
>
> gve: Block invalid ring sizes when device options are 0
>
> Older devices report the minimum ring sizes in the MODIFY_RING device
> option as 0. This let userspace shrink the rings far enough to hit a NULL
> pointer dereference in gve_tx_alloc_rings_dqo(). With this patch,
> gve_parse_device_option() sets default_min_ring_size when either reported
> minimum is 0, so the driver's default minimums are used instead.
>
> > 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]
> Should the fallback be done per direction here? One shared flag is set
> when either min_rx_ring_size or min_tx_ring_size is 0. After that,
> gve_enable_supported_features() overwrites 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 {
>     ...
> }
>
> Say a device reports min_tx=1024 and min_rx=0. gve_set_desc_cnt() then
> sets the TX minimum to 256, and gve_validate_req_ring_size() would accept
> a TX ring size below the device's advertised minimum.
>
> Now say the non-zero minimum is below the default, for example min_tx=64.
> In that case, sizes the device supports are now rejected. Before this
> patch, the else branch kept the non-zero value.
>
> struct gve_device_option_modify_ring has a separate field for each
> minimum. Would a per-field fallback be closer to the intent? For example:
>
>         info->min_tx_ring_size =
>                 be16_to_cpu(dev_op_modify_ring->min_tx_ring_size) ?:
>                 GVE_DEFAULT_MIN_TX_RING_SIZE;
>
> Both values would then only be overridden for the legacy 8-byte option,
> which has neither field.
>
> It's not clear that any firmware reports only one minimum as 0. The
> commit message says older devices report both minimums as 0.

As my reply in v3, the gvnic device either reports both minimums or
both as zero.
>
> [Severity: Low]
> This is a pre-existing issue, but can these reads go past the end of the
> option payload? The length check at the top of this case only requires
> the legacy 8-byte size:
>
>         case GVE_DEV_OPT_ID_MODIFY_RING:
>                 if (option_length < GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
>
> min_rx_ring_size sits at payload offset 8 and min_tx_ring_size at offset
> 10. An option_length of 9, 10 or 11 passes this check. The new condition
> then reads one or both fields partly or wholly beyond the declared option
> length. That is either the next option's header, or memory past the
> descriptor's total_length if this is the last option.
> gve_get_next_option() only checks the option's declared length.
>
> The else branch of gve_enable_supported_features() already over-reads in
> the same way. The new check adds another read at parse time, and that
> read runs even when GVE_SUP_MODIFY_RING_MASK isn't set.
>
> Would it help to require option_length >= sizeof(**dev_op_modify_ring)
> before reading either minimum? Triggering this needs a malformed device
> response.

As my reply in v3, the gvnic device will always return either 8 or 12
for the option_length.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen