Re: [PATCH net v3 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] 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.

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

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