[PATCH net 0/5] eth: fix bugs in ntuple filter reporting

[PATCH net 0/5] eth: fix bugs in ntuple filter reporting

[Jakub Kicinski]

Looking thru some reports prompted by:
  Add new way to add BPF LSM hooks
  https://lore.kernel.org/20260831110934.241898-1-a.s.protopopov@gmail.com

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

[PATCH net 1/5] net: dsa: bcm_sf2: bound the CFP rule dump by the caller's buffer size

[Jakub Kicinski]

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.

Fixes: 7318166cacad ("net: dsa: bcm_sf2: Add support for ethtool::rxnfc")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: florian.fainelli@broadcom.com
CC: jonas.gorski@gmail.com
CC: andrew@lunn.ch
CC: olteanv@gmail.com
---
 drivers/net/dsa/bcm_sf2_cfp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2_cfp.c b/drivers/net/dsa/bcm_sf2_cfp.c
index 50d3a818eb1b..84a086c3e99b 100644
--- a/drivers/net/dsa/bcm_sf2_cfp.c
+++ b/drivers/net/dsa/bcm_sf2_cfp.c
@@ -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

[PATCH net 2/5] eth: nfp: bound the ntuple rule dump by the caller's buffer size

[Jakub Kicinski]

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.

Reported-by: VEGA <vega@nebusec.ai>
Fixes: 9eb03bb1c035 ("nfp: add ethtool flow steering callbacks")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: leitao@debian.org
CC: louis.peens@corigine.com
CC: yinjun.zhang@corigine.com
CC: oss-drivers@corigine.com
---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index a2a89d48e3ca..9419e1ed8466 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -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

[PATCH net 3/5] eth: nfp: drop the replaced rule from the list when reprogramming fails

[Jakub Kicinski]

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.

Fixes: 9eb03bb1c035 ("nfp: add ethtool flow steering callbacks")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: leitao@debian.org
CC: yinjun.zhang@corigine.com
CC: louis.peens@corigine.com
CC: oss-drivers@corigine.com
---
 drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 9419e1ed8466..4e83637715e0 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -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

[PATCH net 4/5] net: dsa: mv88e6xxx: bound the policy rule dump by the caller's buffer size

[Jakub Kicinski]

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.

Fixes: da7dc8755304 ("net: dsa: mv88e6xxx: add RXNFC support")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: olteanv@gmail.com
CC: vivien.didelot@gmail.com
CC: f.fainelli@gmail.com
---
 drivers/net/dsa/mv88e6xxx/chip.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 80b877c74513..7f68a0c55802 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -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

[PATCH net 5/5] ethtool: document that GRXCLSRLALL rule_cnt is a caller-provided limit

[Jakub Kicinski]

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.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
---
 include/linux/ethtool.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12683b5d125e..253600c0eccd 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -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

Re: [PATCH net 1/5] net: dsa: bcm_sf2: bound the CFP rule dump by the caller's buffer size

[Jonas Gorski]

On Thu, Sep 3, 2026 at 5:26 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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.
>
> Fixes: 7318166cacad ("net: dsa: bcm_sf2: Add support for ethtool::rxnfc")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: florian.fainelli@broadcom.com
> CC: jonas.gorski@gmail.com
> CC: andrew@lunn.ch
> CC: olteanv@gmail.com

Reviewed-by: Jonas Gorski <jonas.gorski@gmail.com>

Best regards,
Jonas

Re: [PATCH net 1/5] net: dsa: bcm_sf2: bound the CFP rule dump by the caller's buffer size

[Florian Fainelli]

On 9/2/2026 8:26 PM, Jakub Kicinski wrote:
> 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.
> 
> Fixes: 7318166cacad ("net: dsa: bcm_sf2: Add support for ethtool::rxnfc")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net 1/5] net: dsa: bcm_sf2: bound the CFP rule dump by the caller's buffer size

[Joe Damato]

On Wed, Sep 02, 2026 at 08:26:07PM -0700, Jakub Kicinski wrote:
> 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.
> 
> Fixes: 7318166cacad ("net: dsa: bcm_sf2: Add support for ethtool::rxnfc")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: florian.fainelli@broadcom.com
> CC: jonas.gorski@gmail.com
> CC: andrew@lunn.ch
> CC: olteanv@gmail.com
> ---
>  drivers/net/dsa/bcm_sf2_cfp.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net 2/5] eth: nfp: bound the ntuple rule dump by the caller's buffer size

[Joe Damato]

On Wed, Sep 02, 2026 at 08:26:08PM -0700, Jakub Kicinski wrote:
> 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.
> 
> Reported-by: VEGA <vega@nebusec.ai>
> Fixes: 9eb03bb1c035 ("nfp: add ethtool flow steering callbacks")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: leitao@debian.org
> CC: louis.peens@corigine.com
> CC: yinjun.zhang@corigine.com
> CC: oss-drivers@corigine.com
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net 5/5] ethtool: document that GRXCLSRLALL rule_cnt is a caller-provided limit

[Joe Damato]

On Wed, Sep 02, 2026 at 08:26:11PM -0700, Jakub Kicinski wrote:
> 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.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> ---
>  include/linux/ethtool.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net 4/5] net: dsa: mv88e6xxx: bound the policy rule dump by the caller's buffer size

[Joe Damato]

On Wed, Sep 02, 2026 at 08:26:10PM -0700, Jakub Kicinski wrote:
> 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.
> 
> Fixes: da7dc8755304 ("net: dsa: mv88e6xxx: add RXNFC support")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> CC: olteanv@gmail.com
> CC: vivien.didelot@gmail.com
> CC: f.fainelli@gmail.com
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Joe Damato <joe@dama.to>

Re: [PATCH net 3/5] eth: nfp: drop the replaced rule from the list when reprogramming fails

[Joe Damato]

On Wed, Sep 02, 2026 at 08:26:09PM -0700, Jakub Kicinski wrote:
> 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.
> 
> Fixes: 9eb03bb1c035 ("nfp: add ethtool flow steering callbacks")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: leitao@debian.org
> CC: yinjun.zhang@corigine.com
> CC: louis.peens@corigine.com
> CC: oss-drivers@corigine.com
> ---
>  drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Joe Damato <joe@dama.to>