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

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

[Yehyeong Lee]

gve_adminq_process_flow_rules_query() turns num_queried_rules, which the
device wrote into the admin queue buffer, into a copy length:

	num_queried_rules = be32_to_cpu(descriptor->num_queried_rules);
	rule_info = (void *)(descriptor + 1);
	...
	rule_info_len = num_queried_rules *
			sizeof(*flow_rules_cache->rules_cache);
	if (sizeof(*descriptor) + rule_info_len != total_memory_len) {
		dev_err(&priv->dev->dev, "flow rules query is out of memory.\n");
		return -ENOMEM;
	}

	memcpy(flow_rules_cache->rules_cache, rule_info, rule_info_len);

Nothing bounds that length.  The test above does not, because
total_memory_len is descriptor->total_length and the same device wrote it,
so it only asks the device to agree with itself.

There are two consequences.  For GVE_FLOW_RULE_QUERY_RULES the copy runs
past the cache: at 47 rules it writes 4136 bytes into the 4048 byte
rules_cache, and rules_cache_num is then set to that count for the ethtool
paths to walk.  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.  Those words reach
userspace through ETHTOOL_GRXCLSRLALL, which needs no CAP_NET_ADMIN
because net/ethtool/ioctl.c exempts it, and gve_get_flow_rule_ids() asks
the device again on every call.

Bound the count by the space the array actually has, the admin queue
buffer less the descriptor, before the multiply that cannot then overflow
either.  That is 46 rules and 1020 ids, and it is never larger than the
destination cache.  The total_length test stays, since it still catches a
descriptor whose payload was truncated.

Reproduced with a KUnit harness that injects the device reply and is not
part of this patch.  Unpatched, 47 rules gave the report below, and 1021
and 1024 ids copied 4084 and 4096 bytes from the descriptor's end, 4 and
16 bytes past the block.  With the patch all three are rejected with
-EINVAL, and 46 rules and 1020 ids still work.

  BUG: KASAN: slab-out-of-bounds in gve_adminq_process_flow_rules_query+0xe7/0x390
  Write of size 4136 at addr ffff8880027ae000 by task kunit_try_catch/33

  CPU: 0 UID: 0 PID: 33 Comm: kunit_try_catch Tainted: G                 N  7.3.0-dirty #8 PREEMPT(lazy)
  Tainted: [N]=TEST
  Call Trace:
   <TASK>
   dump_stack_lvl+0x3f/0x50
   print_report+0xcd/0x620
   ? gve_adminq_process_flow_rules_query+0xe7/0x390
   kasan_report+0xde/0x110
   ? gve_adminq_process_flow_rules_query+0xe7/0x390
   kasan_check_range+0x105/0x1b0
   __asan_memcpy+0x3c/0x60
   gve_adminq_process_flow_rules_query+0xe7/0x390
   gve_fr_rules_overflow_test+0xc9/0x430
   ? __pfx_gve_fr_rules_overflow_test+0x10/0x10
   ? __pfx___set_cpus_allowed_ptr+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 33:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kvmalloc_node_noprof+0x1e8/0x4e0
   gve_fr_ctx_init+0x24d/0x920
   gve_fr_rules_overflow_test+0xae/0x430
   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 ffff8880027ae000
   which belongs to the cache kmalloc-4k of size 4096
  The buggy address is located 0 bytes inside of
   allocated 4048-byte region [ffff8880027ae000, ffff8880027aefd0)

  The buggy address belongs to the physical page:
  page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x27a8
  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 ffffea000009ea01 00000000ffffffff 00000000ffffffff
  head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
  page dumped because: kasan: bad access detected

  Memory state around the buggy address:
   ffff8880027aee80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   ffff8880027aef00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  >ffff8880027aef80: 00 00 00 00 00 00 00 00 00 00 fc fc fc fc fc fc
                                                   ^
   ffff8880027af000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
   ffff8880027af080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

Fixes: 57718b60df9b ("gve: Add flow steering adminq commands")
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, 10 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 8b215fca9c1b63..02c1f398f6e395 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);
 
 	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;
+
 		rule_info_len = num_queried_rules * sizeof(*flow_rules_cache->rules_cache);
 		if (sizeof(*descriptor) + rule_info_len != total_memory_len) {
 			dev_err(&priv->dev->dev, "flow rules query is out of memory.\n");
@@ -1560,6 +1566,10 @@ static int gve_adminq_process_flow_rules_query(struct gve_priv *priv, u16 query_
 		flow_rules_cache->rules_cache_num = num_queried_rules;
 		break;
 	case GVE_FLOW_RULE_QUERY_IDS:
+		if (num_queried_rules >
+		    rule_info_max_len / sizeof(*flow_rules_cache->rule_ids_cache))
+			return -EINVAL;
+
 		rule_info_len = num_queried_rules * sizeof(*flow_rules_cache->rule_ids_cache);
 		if (sizeof(*descriptor) + rule_info_len != total_memory_len) {
 			dev_err(&priv->dev->dev, "flow rule ids query is out of memory.\n");

base-commit: 787268686c64be60023743b527645a907e38ccca
-- 
2.43.0

Re: [PATCH net] gve: bound the device reported rule count in the flow rules 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.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

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.

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

[Yehyeong Lee]

Hi,

Hardware: not tested on real gVNIC hardware.  The overflow was
reproduced with a KUnit harness that feeds
gve_adminq_process_flow_rules_query() crafted device replies under KASAN
(x86_64, kunit.py on QEMU); the harness is not part of the patch.

Best regards,
Yehyeong Lee