Re: [PATCH net] gve: bound the device reported rule count in the flow rules query

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message gets the pre-patch impact of the
  GVE_FLOW_RULE_QUERY_IDS path wrong.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  gve_adminq_process_rss_query() in…

--

gve: bound the device reported rule count in the flow rules query

This patch makes gve_adminq_process_flow_rules_query() reject a
device-reported num_queried_rules that does not fit in the admin queue
buffer after the descriptor. The check runs before the count is turned
into a memcpy() length. It is applied to both the
GVE_FLOW_RULE_QUERY_RULES and GVE_FLOW_RULE_QUERY_IDS opcodes.

> For GVE_FLOW_RULE_QUERY_IDS the destination is large
> enough, but the descriptor and the rule array share one
> GVE_ADMINQ_BUFFER_SIZE block and the array starts after the descriptor, so
> counts of 1021 to 1024 read past that 4096 byte block.

[Severity: Medium]
Is this description of the pre-patch QUERY_IDS behavior complete? It
seems to hold only for counts up to 1024.

rule_ids_cache is allocated in gve_alloc_flow_rule_caches() as:

    kvcalloc(GVE_FLOW_RULE_IDS_CACHE_SIZE, sizeof(*flow_rules_cache->rule_ids_cache),
             GFP_KERNEL);

GVE_FLOW_RULE_IDS_CACHE_SIZE is GVE_ADMINQ_BUFFER_SIZE divided by the size
of a __be32 location. That makes the cache 1024 entries, or 4096 bytes.

Before this patch, a reply with num_queried_rules = 1025 and
total_length = 4116 passes the total_length check. The memcpy() in
gve_adminq_process_flow_rules_query() then writes 4100 bytes into the
4096-byte rule_ids_cache, and larger counts write further. So the IDS path
looks like it also had a heap write overflow, not only a 4 to 16 byte
over-read.

There is also a wrap case. For num_queried_rules >= 2^30 the u32 multiply
num_queried_rules * 4 wraps, so the memcpy() is small. However,
rule_ids_cache_num is still set to the huge count. gve_get_flow_rule_ids()
then walks past the 1024 entries:

    for (i = 0; i < *cache_num; i++) {
        if (j >= cmd->rule_cnt)
            return -EMSGSIZE;

        rule_locs[j++] = be32_to_cpu(rule_ids_cache[i]);

Those values reach userspace through ETHTOOL_GRXCLSRLALL.

The RULES path can wrap the same way, since 2^29 * 88 is 0 mod 2^32. The
resulting rules_cache_num is then walked through ETHTOOL_GRXCLSRULE. That
command is in the same no-CAP_NET_ADMIN list in net/ethtool/ioctl.c, but
the commit message only mentions ETHTOOL_GRXCLSRLALL.

The new bounds do cover all of these cases. Could the commit message also
mention the write overflow for more than 1024 ids and the multiply wrap on
both paths?

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 8b215fca9c1b6..02c1f398f6e39 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -1542,14 +1542,20 @@ static int gve_adminq_process_flow_rules_query(struct gve_priv *priv, u16 query_
>  {
>  	struct gve_flow_rules_cache *flow_rules_cache = &priv->flow_rules_cache;
>  	u32 num_queried_rules, total_memory_len, rule_info_len;
> +	u32 rule_info_max_len;
>  	void *rule_info;
>  
>  	total_memory_len = be32_to_cpu(descriptor->total_length);
>  	num_queried_rules = be32_to_cpu(descriptor->num_queried_rules);
>  	rule_info = (void *)(descriptor + 1);
> +	rule_info_max_len = GVE_ADMINQ_BUFFER_SIZE - sizeof(*descriptor);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
does gve_adminq_process_rss_query() in gve_adminq.c need the same kind of
bound?

gve_adminq_query_rss_config() also allocates its descriptor from
priv->adminq_pool, whose blocks are GVE_ADMINQ_BUFFER_SIZE bytes. The only
length check compares device-supplied values with each other:

    hash_lut_length = priv->rss_lut_size * sizeof(*rxfh->indir);

    if (sizeof(*descriptor) + priv->rss_key_size + hash_lut_length != total_memory_length) {

gve_enable_supported_features() takes the key and LUT sizes from the
device with no upper bound:

    info->rss_key_size =
        be16_to_cpu(dev_op_rss_config->hash_key_size);
    info->rss_lut_size =
        be16_to_cpu(dev_op_rss_config->hash_lut_size);
    info->cache_rss_config = false;

Nothing then caps the total at GVE_ADMINQ_BUFFER_SIZE - sizeof(*descriptor)
before these reads:

    memcpy(rxfh->key, rss_info_addr, priv->rss_key_size);
    ...
    for (i = 0; i < priv->rss_lut_size; i++)
        rxfh->indir[i] = be32_to_cpu(lut[i]);

hash_lut_length is also a u16. With rss_lut_size = 16384 it truncates to
0, so the check passes against a small total_length while the loop still
reads 64 KiB of LUT.

The ethtool destination buffers are sized from gve_get_rxfh_key_size() and
gve_get_rxfh_indir_size(). Those return the same device values, so they do
not limit the read either.

Because cache_rss_config is false, gve_get_rxfh() always calls
gve_adminq_query_rss_config(). ETHTOOL_GRSSH also does not require
CAP_NET_ADMIN.

Can this copy kernel memory from beyond the 4096-byte DMA block to an
unprivileged user?

>  
>  	switch (query_opcode) {
>  	case GVE_FLOW_RULE_QUERY_RULES:
> +		if (num_queried_rules >
> +		    rule_info_max_len / sizeof(*flow_rules_cache->rules_cache))
> +			return -EINVAL;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104061329.1881388-1-yhlee%40isslab.korea.ac.kr