Re: [PATCH net] gve: bound the device reported RSS sizes in the RSS query

[Harshitha Ramamurthy]

On Tue, Nov 3, 2026 at 11:01 PM Yehyeong Lee <yhlee@xxxxxxxxxxxxxxxxxx> wrote:
>
> gve_adminq_process_rss_query() copies priv->rss_key_size bytes and
> priv->rss_lut_size words out of the admin queue buffer.  Both sizes come
> from the device: gve_parse_device_option() takes them from the RSS device
> option with no validation, and gve_adminq_describe_device() stores them.
> They are 16 bit fields, so either can be up to 65535.
>
> The source is one GVE_ADMINQ_BUFFER_SIZE block and the payload starts
> after the descriptor, so it holds at most 4088 bytes.  The only length
> test compares the sum with descriptor->total_length, which the same device
> wrote, and it is computed into a u16:
>
>         u16 hash_lut_length;
>         ...
>         hash_lut_length = priv->rss_lut_size * sizeof(*rxfh->indir);
>
>         if (sizeof(*descriptor) + priv->rss_key_size + hash_lut_length !=
>             total_memory_length) {
>
> A lut size of 16384 makes that product 65536, which truncates to zero, so
> the device only has to report a total_length of 48 to pass.  The loop then
> reads 16384 words, 65536 bytes, out of a 4096 byte block, and the walk
> lands in rxfh->indir, which the ethtool core sized from the same device
> value.  ETHTOOL_GRSSH and ETHTOOL_GRXFHINDIR hand that to the caller and
> neither needs CAP_NET_ADMIN, net/ethtool/ioctl.c exempts them.
>
> Reject sizes that do not fit the buffer, and compute the lut length in a
> type that cannot truncate.
>
> In the driver the block is dma_pool memory, so KASAN does not annotate it
> and the contents of the neighbouring blocks leak with no report at all;
> the report below comes from a KUnit harness that hands the handler a
> kmalloc block of the same size, injects the device reply, and is not part
> of this patch.  Unpatched, a lut of 16384 and a lut of 1022 both read past
> the block; with the patch both are rejected with -EINVAL, and the sizes
> the driver itself defaults to, 40 and 128, still work.
>
>   BUG: KASAN: slab-out-of-bounds in gve_adminq_process_rss_query+0x3da/0x4a0
>   Read of size 4 at addr ffff888002845000 by task kunit_try_catch/54
>
>   CPU: 0 UID: 0 PID: 54 Comm: kunit_try_catch Tainted: G                 N  7.3.0-dirty #11 PREEMPT(lazy)
>   Tainted: [N]=TEST
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x3f/0x50
>    print_report+0xcd/0x620
>    ? gve_adminq_process_rss_query+0x3da/0x4a0
>    kasan_report+0xde/0x110
>    ? gve_adminq_process_rss_query+0x3da/0x4a0
>    gve_adminq_process_rss_query+0x3da/0x4a0
>    gve_rss_run.constprop.0+0xa7/0x1a0
>    ? __pfx_gve_rss_run.constprop.0+0x10/0x10
>    ? __pfx_read_tsc+0x10/0x10
>    ? ktime_get_ts64+0x192/0x250
>    kunit_try_run_case+0x1ca/0x4b0
>    ? __pfx_kunit_try_run_case+0x10/0x10
>    ? kthread_affine_node+0x247/0x360
>    ? __pfx_kthread_affine_node+0x10/0x10
>    ? __pfx_kunit_try_run_case+0x10/0x10
>    ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
>    kunit_generic_run_threadfn_adapter+0x7b/0xe0
>    kthread+0x36a/0x4f0
>    ? __pfx_kthread+0x10/0x10
>    ret_from_fork+0x34d/0x620
>    ? __pfx_ret_from_fork+0x10/0x10
>    ? __switch_to+0x22/0xde0
>    ? __pfx_kthread+0x10/0x10
>    ret_from_fork_asm+0x19/0x30
>    </TASK>
>
>   Allocated by task 54:
>    kasan_save_stack+0x33/0x60
>    kasan_save_track+0x14/0x30
>    __kasan_kmalloc+0x8f/0xa0
>    __kmalloc_noprof+0x19b/0x3e0
>    kunit_kmalloc_array+0x1b/0x50
>    gve_rss_ctx_init+0x40a/0x910
>    gve_rss_run.constprop.0+0x93/0x1a0
>    kunit_try_run_case+0x1ca/0x4b0
>    kunit_generic_run_threadfn_adapter+0x7b/0xe0
>    kthread+0x36a/0x4f0
>    ret_from_fork+0x34d/0x620
>    ret_from_fork_asm+0x19/0x30
>
>   The buggy address belongs to the object at ffff888002844000
>    which belongs to the cache kmalloc-4k of size 4096
>   The buggy address is located 0 bytes to the right of
>    allocated 4096-byte region [ffff888002844000, ffff888002845000)
>
>   The buggy address belongs to the physical page:
>   page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x2840
>   head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
>   flags: 0x4000000000000040(head|zone=1)
>   page_type: f5(slab)
>   raw: 4000000000000040 ffff888001042140 dead000000000122 0000000000000000
>   raw: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
>   head: 4000000000000040 ffff888001042140 dead000000000122 0000000000000000
>   head: 0000000000000000 0000000000040004 00000000f5000000 0000000000000000
>   head: 4000000000000003 ffffea00000a1001 00000000ffffffff 00000000ffffffff
>   head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
>   page dumped because: kasan: bad access detected
>
>   Memory state around the buggy address:
>    ffff888002844f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    ffff888002844f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   >ffff888002845000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>                      ^
>    ffff888002845080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>    ffff888002845100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> Fixes: fa46c456fa6e ("gve: Add RSS adminq commands and ethtool support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Yehyeong Lee <yhlee@xxxxxxxxxxxxxxxxxx>

Similarly to https://lore.kernel.org/netdev/CAEAWyHdJNjhV4biOFR+cw0MWNMZHUT638+AT9Q83s9G7wyBsgw@xxxxxxxxxxxxxx/,
the driver already provides an 'available_length' field as part of
gve_adminq_query_rss_config(). So this should be dropped too.

> ---
>  drivers/net/ethernet/google/gve/gve_adminq.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 8b215fca9c1b63..1ba658cb2942be 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -1615,7 +1615,7 @@ static int gve_adminq_process_rss_query(struct gve_priv *priv,
>                                         struct ethtool_rxfh_param *rxfh)
>  {
>         u32 total_memory_length;
> -       u16 hash_lut_length;
> +       u32 hash_lut_length;
>         void *rss_info_addr;
>         __be32 *lut;
>         u16 i;
> @@ -1623,6 +1623,14 @@ static int gve_adminq_process_rss_query(struct gve_priv *priv,
>         total_memory_length = be32_to_cpu(descriptor->total_length);
>         hash_lut_length = priv->rss_lut_size * sizeof(*rxfh->indir);
>
> +       if (sizeof(*descriptor) + priv->rss_key_size + hash_lut_length >
> +           GVE_ADMINQ_BUFFER_SIZE) {
> +               dev_err(&priv->dev->dev,
> +                       "rss sizes from device do not fit the adminq buffer: key %u lut %u\n",
> +                       priv->rss_key_size, priv->rss_lut_size);
> +               return -EINVAL;
> +       }
> +
>         if (sizeof(*descriptor) + priv->rss_key_size + hash_lut_length != total_memory_length) {
>                 dev_err(&priv->dev->dev,
>                         "rss query desc from device has invalid length parameter.\n");
>
> base-commit: 787268686c64be60023743b527645a907e38ccca
> --
> 2.43.0
>