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

[netdev-bot+sashiko@kernel.org]

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.

[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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com