Message-Id: <20230724151849.323497-1-jhs@mojatatu.com>
Date: Mon, 24 Jul 2023 11:18:49 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: davem@...emloft.net,
	kuba@...nel.org,
	edumazet@...gle.com,
	pabeni@...hat.com
Cc: jiri@...nulli.us,
	xiyou.wangcong@...il.com,
	netdev@...r.kernel.org,
	mgcho.minic@...il.com,
	security@...nel.org,
	Jamal Hadi Salim <jhs@...atatu.com>
Subject: [PATCH net 1/1] net: sched: cls_u32: Fix match key mis-addressing

A match entry is uniquely identified with an "address" or "path" in the
form of: hashtable ID(12b):bucketid(8b):nodeid(12b).
A table on which the entry is inserted assumes the address with nodeid of
zero.

When creating table match entries all of hash table id, bucket id and
node (match entry id) are needed to be either specified by the user or
via reasonable in-kernel defaults. The in-kernel default for a table
id is 0x800(omnipresent root table); for bucketid it is 0x0. Prior to
this fix there was none for a nodeid i.e. the code assumed that the
user passed the correct nodeid and if the user passes a nodeid of 0
(as Mingi Cho did) then that is what was assumed. But nodeid of 0
is reserved for identifying the table. This is not a problem until
we dump. The dump code notices that the nodeid is zero and assumes
it is referencing a table and therefore table struct tc_u_hnode instead
of what was created i.e match entry struct tc_u_knode.

Ming does an equivalent of:
tc filter add dev dummy0 parent 10: prio 1 handle 0x1000 \
protocol ip u32 match ip src 10.0.0.1/32 classid 10:1 action ok

Essentially specifying a table id 0, bucketid 1 and nodeid of zero
Tableid 0 is remapped to the default of 0x800.
Bucketid 1 is ignored and defaults to 0x00.
Nodeid was assumed to be what Ming passed - 0x000

dumping before fix shows:
~$ tc filter ls dev dummy0 parent 10:
filter protocol ip pref 1 u32 chain 0
filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor 1
filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor -30591

Note that the last line reports a table instead of a match entry
(you can tell this because it says "ht divisor...").
As a result of reporting the wrong data type (misinterpretting of struct
tc_u_knode as being struct tc_u_hnode) the divisor is reported with value
of -30591. Ming identified this as part of the heap address
(physmap_base is 0xffff8880 (-30591 - 1)).

The fix is to ensure that when table entry matches are added and no
nodeid is specified (i.e nodeid == 0) then we get the next available
nodeid from the table's pool.

After the fix, this is what the dump shows:
$ tc filter ls dev dummy0 parent 10:
filter protocol ip pref 1 u32 chain 0
filter protocol ip pref 1 u32 chain 0 fh 800: ht divisor 1
filter protocol ip pref 1 u32 chain 0 fh 800::800 order 2048 key ht 800 bkt 0 flowid 10:1 not_in_hw
  match 0a000001/ffffffff at 12
	action order 1: gact action pass
	 random type none pass val 0
	 index 1 ref 1 bind 1

Reported-by: Mingi Cho <mgcho.minic@...il.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
---
 net/sched/cls_u32.c | 48 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 5abf31e432ca..e0eabbcce9d4 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -1024,18 +1024,54 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return -EINVAL;
 	}
 
+	/* If we got this far _we have the table(ht)_ with its matching htid.
+	 * Note that earlier the ht selection is a result of either a) the user
+	 * providing the htid specified via TCA_U32_HASH attribute or b) when
+	 * no such attribute is passed then a default ht, typically root at
+	 * 0x80000000, is chosen.
+	 * The passed htid will also hold the bucketid. 0 is fine. For example
+	 * of the root ht, 0x80000000 is indicating bucketid 0, whereas a user
+	 * passed htid may have 0x60001000 indicating hash bucketid 1.
+	 *
+	 * We may also have a handle, if the user passed one. The handle carries
+	 * annotation of htid(12b):bucketid(8b):node/entryid(12b).
+	 * The value of bucketid on the handle is ignored even if one was passed;
+	 * rather the value on the htid is always assumed to be the bucketid.
+	 */
 	if (handle) {
+		/* The tableid from handle and tableid from htid must match */
 		if (TC_U32_HTID(handle) && TC_U32_HTID(handle ^ htid)) {
 			NL_SET_ERR_MSG_MOD(extack, "Handle specified hash table address mismatch");
 			return -EINVAL;
 		}
-		handle = htid | TC_U32_NODE(handle);
-		err = idr_alloc_u32(&ht->handle_idr, NULL, &handle, handle,
-				    GFP_KERNEL);
-		if (err)
-			return err;
-	} else
+		/* Ok, so far we have a valid htid(12b):bucketid(8b) but we
+		 * need to finalize our handle to point to the entry as well
+		 * (with a proper node/entryid(12b)). Nodeid _cannot be 0_ for
+		 * entries since it is reserved only for tables(see earlier
+		 * code which processes TC_U32_DIVISOR attribute).
+		 * if the handle did not specify a non-zero nodeid (example
+		 * passed 0x60000000) then pick a new one from the pool of IDs
+		 * this hash table has been allocating from.
+		 * If OTOH it is specified (i.e for example the user passed a
+		 * handle such as 0x60000123), then we use it generate our final
+		 * handle which is used to uniquely identify the match entry.
+		 */
+		if (!TC_U32_NODE(handle)) {
+			handle = gen_new_kid(ht, htid);
+		} else {
+			handle = htid | TC_U32_NODE(handle);
+			err = idr_alloc_u32(&ht->handle_idr, NULL, &handle,
+					    handle, GFP_KERNEL);
+			if (err)
+				return err;
+		}
+	} else {
+		/* we dont have a handle lets just generate one based on htid
+		 * recall that htid has both the table and bucket ids already
+		 * encoded, only missing piece is the nodeid.
+		 */
 		handle = gen_new_kid(ht, htid);
+	}
 
 	if (tb[TCA_U32_SEL] == NULL) {
 		NL_SET_ERR_MSG_MOD(extack, "Selector not specified");
-- 
2.34.1

Powered by blists - more mailing lists