eth: fix bugs in ntuple filter reporting

Looking thru some reports prompted by:
Add new way to add BPF LSM hooks
https://lore.kernel.org/[email protected]

I/Claude noticed 3 drivers with buggy n-tuple filter dump. PoC built based on intentionally adding the same bug in fbnic under QEMU confirms:

    # install 8 rules (this part does need CAP_NET_ADMIN)
    for p in 100 101 102 103 104 105 106 107; do
            ethtool -N eth0 flow-type tcp4 dst-port $p action 0
    done

# Python
SIOCETHTOOL = 0x8946
ETHTOOL_GRXCLSRLALL = 0x30
RXNFC_SIZE = 192 # sizeof(struct ethtool_rxnfc)
RULE_CNT_OFF = 184 # offsetof(struct ethtool_rxnfc, rule_cnt)

buf = array.array('B', bytes(RXNFC_SIZE + 4096)) struct.pack_into('=I', buf, 0, ETHTOOL_GRXCLSRLALL)
struct.pack_into('=I', buf, RULE_CNT_OFF, 1) # room for one location

sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) ifr = struct.pack('16sP', b'eth0', buf.buffer_info()[0]) fcntl.ioctl(sock, SIOCETHTOOL, ifr)

==================================================================

  BUG: KASAN: slab-out-of-bounds in fbnic_get_rxnfc+0x144d/0x1910
  Write of size 4 at addr ff11000007522be4 by task python3.12/647

Fix the 3 drivers, add a hopefully clearer mention in the doc.

Note that Sashiko will likely complain about mv88e6xxx letting user read and delete rules from any port with ant netdev. We can fix that in net-next, if mv88e6xxx experts can confirm that the current behavior is not intentional (it's wrong but users may now depend on it).

Jakub Kicinski (5):
net: dsa: bcm_sf2: bound the CFP rule dump by the caller's buffer size
eth: nfp: bound the ntuple rule dump by the caller's buffer size
eth: nfp: drop the replaced rule from the list when reprogramming
fails
net: dsa: mv88e6xxx: bound the policy rule dump by the caller's buffer
size
ethtool: document that GRXCLSRLALL rule_cnt is a caller-provided limit

 include/linux/ethtool.h                       |  6 ++++++
 drivers/net/dsa/bcm_sf2_cfp.c                 |  2 ++
 drivers/net/dsa/mv88e6xxx/chip.c              | 16 ++++++++++++----
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 19 +++++++++++++++----
 4 files changed, 35 insertions(+), 8 deletions(-)

2.55.0

bcm_sf2_cfp_rule_get_all() walks the whole cfp.unique bitmap into rule_locs[] without consulting nfc->rule_cnt, which is how many entries the caller had room for. ETHTOOL_GRXCLSRLALL requires no CAP_NET_ADMIN and the ioctl sizes the buffer from the rule_cnt userspace passes in, so once an admin has installed CFP rules any user can ask for fewer slots than there are rules and run off the end of the allocation. A rule_cnt of 0 leaves the buffer pointer NULL and the walk dereferences it.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/dsa/bcm_sf2_cfp.c | 2 ++
 1 file changed, 2 insertions(+)

index 50d3a818eb1b..84a086c3e99b 100644
@@ -1088,6 +1088,8 @@ static int bcm_sf2_cfp_rule_get_all(struct bcm_sf2_priv *priv,
 	unsigned int index = 1, rules_cnt = 0;
 
 	for_each_set_bit_from(index, priv->cfp.unique, priv->num_cfp_rules) {
+		if (rules_cnt == nfc->rule_cnt)
+			return -EMSGSIZE;
 		rule_locs[rules_cnt] = index;
 		rules_cnt++;
 	}

2.55.0

Best regards,
Jonas

We had the exact same internal patch we were about to submit, thanks!

Florian

nfp_net_get_fs_loc() dumps every entry of nn->fs.list into rule_locs[] without consulting cmd->rule_cnt, which is how many entries the caller had room for. ETHTOOL_GRXCLSRLALL requires no CAP_NET_ADMIN and the ioctl sizes the buffer from the rule_cnt userspace passes in, so once an admin has installed flow steering rules any user can ask for fewer slots than there are rules and run off the end of the allocation. A rule_cnt of 0 leaves the buffer pointer NULL and the walk dereferences it.

Bail out with -EMSGSIZE when the buffer fills up, the way the other ntuple capable drivers do, and report how many locations were filled so a shrinking rule list does not leave the caller reading stale slots.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index a2a89d48e3ca..9419e1ed8466 100644
@@ -1421,7 +1421,8 @@ static int nfp_net_get_fs_rule(struct nfp_net *nn, struct ethtool_rxnfc *cmd)
 	return -ENOENT;
 }
 
-static int nfp_net_get_fs_loc(struct nfp_net *nn, u32 *rule_locs)
+static int nfp_net_get_fs_loc(struct nfp_net *nn, struct ethtool_rxnfc *cmd,
+			      u32 *rule_locs)
 {
 	struct nfp_fs_entry *entry;
 	u32 count = 0;

@@ -1429,8 +1430,12 @@ static int nfp_net_get_fs_loc(struct nfp_net *nn, u32 *rule_locs)
 	if (!(nn->cap_w1 & NFP_NET_CFG_CTRL_FLOW_STEER))
 		return -EOPNOTSUPP;
 
-	list_for_each_entry(entry, &nn->fs.list, node)
+	list_for_each_entry(entry, &nn->fs.list, node) {
+		if (count == cmd->rule_cnt)
+			return -EMSGSIZE;
 		rule_locs[count++] = entry->loc;
+	}
+	cmd->rule_cnt = count;
 
 	return 0;
 }

@@ -1455,7 +1460,7 @@ static int nfp_net_get_rxnfc(struct net_device *netdev,
 		return nfp_net_get_fs_rule(nn, cmd);
 	case ETHTOOL_GRXCLSRLALL:
 		cmd->data = NFP_FS_MAX_ENTRY;
-		return nfp_net_get_fs_loc(nn, rule_locs);
+		return nfp_net_get_fs_loc(nn, cmd, rule_locs);
 	default:
 		return -EOPNOTSUPP;
 	}

2.55.0

nfp_net_fs_add() replaces an existing rule by deleting it from the hardware, decrementing nn->fs.count and programming the new one. If nfp_net_fs_add_hw() fails the old entry stays on nn->fs.list - only the success path reaches list_replace() - so the list is one longer than nn->fs.count, and it advertises a rule whose hardware entry has already been torn down.

nn->fs.count is what ETHTOOL_GRXCLSRLCNT reports, so userspace then sizes its buffer one entry short of what the GRXCLSRLALL walk wants to write. That used to overwrite one u32 past the allocation; since the walk is bounded it is a permanent -EMSGSIZE instead, as nothing ever resyncs the counter.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 9419e1ed8466..4e83637715e0 100644
@@ -1703,8 +1703,14 @@ static int nfp_net_fs_add(struct nfp_net *nn, struct ethtool_rxnfc *cmd)
 
 			nn->fs.count--;
 			err = nfp_net_fs_add_hw(nn, new);
-			if (err)
+			if (err) {
+				/* mbox broken, adding the old rule back will
+				 * likely also fail.
+				 */
+				list_del(&entry->node);
+				kfree(entry);
 				goto err;
+			}
 
 			nn->fs.count++;
 			list_replace(&entry->node, &new->node);

2.55.0

mv88e6xxx_get_rxnfc() uses rxnfc->rule_cnt as the write index while dumping the policy IDR, clobbering the input value before it has been looked at. That input is the number of entries the caller had room for. ETHTOOL_GRXCLSRLALL requires no CAP_NET_ADMIN and the ioctl sizes the buffer from the rule_cnt userspace passes in, so once an admin has installed policy rules any user can ask for fewer slots than there are rules and run off the end of the allocation. A rule_cnt of 0 leaves the buffer pointer NULL and the walk dereferences it.

Count into a local so the caller's limit survives the walk, and stop with -EMSGSIZE once it is reached.

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 drivers/net/dsa/mv88e6xxx/chip.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

index 80b877c74513..7f68a0c55802 100644
@@ -2438,6 +2438,7 @@ static int mv88e6xxx_get_rxnfc(struct dsa_switch *ds, int port,
 	struct ethtool_rx_flow_spec *fs = &rxnfc->fs;
 	struct mv88e6xxx_chip *chip = ds->priv;
 	struct mv88e6xxx_policy *policy;
+	u32 cnt = 0;
 	int err;
 	int id;
 

@@ -2463,11 +2464,18 @@ static int mv88e6xxx_get_rxnfc(struct dsa_switch *ds, int port,
 		break;
 	case ETHTOOL_GRXCLSRLALL:
 		rxnfc->data = 0;
-		rxnfc->rule_cnt = 0;
-		idr_for_each_entry(&chip->policies, policy, id)
-			if (policy->port == port)
-				rule_locs[rxnfc->rule_cnt++] = id;
 		err = 0;
+		idr_for_each_entry(&chip->policies, policy, id) {
+			if (policy->port != port)
+				continue;
+			if (cnt == rxnfc->rule_cnt) {
+				err = -EMSGSIZE;
+				break;
+			}
+			rule_locs[cnt++] = id;
+		}
+		if (!err)
+			rxnfc->rule_cnt = cnt;
 		break;
 	default:
 		err = -EOPNOTSUPP;

2.55.0

Three drivers have shipped a get_rxnfc() which dumps its entire rule table into rule_locs, reading rule_cnt as "how many rules do I have" rather than "how many entries did the caller allocate". Nothing in the callback's documentation contradicted that reading. The distinction only matters because the ioctl lets an unprivileged caller pick rule_cnt directly, so getting it wrong is a heap overflow rather than a truncated dump.

---
CC: [email protected]

---
 include/linux/ethtool.h | 6 ++++++
 1 file changed, 6 insertions(+)

index 12683b5d125e..253600c0eccd 100644
@@ -1057,6 +1057,12 @@ struct kernel_ethtool_ts_info {
  * @get_sset_count: Get number of strings that @get_strings will write.
  * @get_rxnfc: Get RX flow classification rules.  Returns a negative
  *	error code or zero.
+ *	Note that for %ETHTOOL_GRXCLSRLALL rule_cnt and size of the arrays
+ *	is user-provided, and not guaranteed to match what driver would
+ *	have reported via %ETHTOOL_GRXCLSRLCNT. Drivers must return -%EMSGSIZE
+ *	when rule_cnt is too small. rule_locs is %NULL when rule_cnt is zero.
+ *	On success drivers must set rule_cnt to the number of locations they
+ *	filled in, the core copies out exactly that many.
  * @set_rxnfc: Set RX flow classification rules.  Returns a negative
  *	error code or zero.
  * @flash_device: Write a firmware image to device's flash memory.

2.55.0