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

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

[Yehyeong Lee]

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@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.