[PATCH net-next 0/2] net: dsa: mv88e6xxx: ethtool n-tuple follow ups

[PATCH net-next 0/2] net: dsa: mv88e6xxx: ethtool n-tuple follow ups

[Jakub Kicinski]

Here are two small tweaks to the mv88e6xxx n-tuple filter handling
suggested by Sashiko when reviewing commit b1fffc273112 ("net: dsa:
mv88e6xxx: bound the policy rule dump by the caller's buffer size")

At the high level the problem is that the driver has shared rule
table but for some ops it doesn't check whether the rule operated
on belongs to the port on which the ethtool request was sent.

I initially thought that this could be intentional, but on closer
look I don't think it really works..

Sending for net-next, because this is AI-induced, build-tested-only
slop-code. The outcome is unlikely and results in mis-configuration,
no crashes.

Jakub Kicinski (2):
  net: dsa: mv88e6xxx: check the port when reading back a policy rule
  net: dsa: mv88e6xxx: check the port when deleting a policy rule

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

-- 
2.55.0

[PATCH net-next 1/2] net: dsa: mv88e6xxx: check the port when reading back a policy rule

[Jakub Kicinski]

chip->policies is one IDR for the whole switch, so the location an
ETHTOOL_GRXCLSRULE caller supplies can name a rule belonging to any port.
mv88e6xxx_get_rxnfc() copies it out without looking at policy->port, which
lets a user on one user port read the MAC address and VID of a policy an
admin installed on another.  ETHTOOL_GRXCLSRULE needs no CAP_NET_ADMIN.

The GRXCLSRLCNT and GRXCLSRLALL arms of the same switch statement already
filter on policy->port, which makes the GRXCLSRULE omission look
unintentional. Nonetheless, changing this does carry some regression
risk.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: olteanv@gmail.com
CC: f.fainelli@gmail.com
CC: vivien.didelot@gmail.com
---
 drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7f68a0c55802..daef7c78b860 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2457,7 +2457,7 @@ static int mv88e6xxx_get_rxnfc(struct dsa_switch *ds, int port,
 	case ETHTOOL_GRXCLSRULE:
 		err = -ENOENT;
 		policy = idr_find(&chip->policies, fs->location);
-		if (policy) {
+		if (policy && policy->port == port) {
 			memcpy(fs, &policy->fs, sizeof(*fs));
 			err = 0;
 		}
-- 
2.55.0

[PATCH net-next 2/2] net: dsa: mv88e6xxx: check the port when deleting a policy rule

[Jakub Kicinski]

ETHTOOL_SRXCLSRLDEL removes the policy the location names out of the
switch-wide IDR without checking which port owns it, then hands it to
mv88e6xxx_policy_apply() along with the port the request arrived on.
Deleting another port's rule therefore purges an ATU entry on and clears
the policy register of the requesting port, while the owning port keeps
its hardware policy - and, with the software object freed, no longer has
a location that can be used to remove it.

Initially I thought that we're just able to delete a rule from one port
using another port as a handle. But if the reading of the code outlined
above is correct, this is just borken, not a "feature" someone could
depend on.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: olteanv@gmail.com
CC: f.fainelli@gmail.com
CC: vivien.didelot@gmail.com
---
 drivers/net/dsa/mv88e6xxx/chip.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index daef7c78b860..7b34cae9c542 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2503,8 +2503,9 @@ static int mv88e6xxx_set_rxnfc(struct dsa_switch *ds, int port,
 		break;
 	case ETHTOOL_SRXCLSRLDEL:
 		err = -ENOENT;
-		policy = idr_remove(&chip->policies, fs->location);
-		if (policy) {
+		policy = idr_find(&chip->policies, fs->location);
+		if (policy && policy->port == port) {
+			idr_remove(&chip->policies, fs->location);
 			policy->action = MV88E6XXX_POLICY_ACTION_NORMAL;
 			err = mv88e6xxx_policy_apply(chip, port, policy);
 			devm_kfree(chip->dev, policy);
-- 
2.55.0

Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: check the port when reading back a policy rule

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Missing `Fixes:` tag.

Pre-existing issues:
- [Low] SRXCLSRLDEL cross-port delete: mv88e6xxx_set_rxnfc()'s
  ETHTOOL_SRXCLSRLDEL arm looks up fs->location in the switch-wide…

--

net: dsa: mv88e6xxx: check the port when reading back a policy rule

This adds a policy->port == port test to the ETHTOOL_GRXCLSRULE arm of
mv88e6xxx_get_rxnfc(), so a location belonging to another user port is no
longer copied back to the caller.  The GRXCLSRLCNT and GRXCLSRLALL arms of
the same switch already filter on policy->port.

> net: dsa: mv88e6xxx: check the port when reading back a policy rule
>
> chip->policies is one IDR for the whole switch, so the location an
> ETHTOOL_GRXCLSRULE caller supplies can name a rule belonging to any port.
> mv88e6xxx_get_rxnfc() copies it out without looking at policy->port, which
> lets a user on one user port read the MAC address and VID of a policy an
> admin installed on another.  ETHTOOL_GRXCLSRULE needs no CAP_NET_ADMIN.
[ ... ]
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

[Severity: Medium]
Should this carry a Fixes: tag?

git blame on the pre-patch GRXCLSRULE arm in mv88e6xxx_get_rxnfc():

	policy = idr_find(&chip->policies, fs->location);
	if (policy) {
		memcpy(fs, &policy->fs, sizeof(*fs));

attributes those lines to da7dc87553046 ("net: dsa: mv88e6xxx: add RXNFC
support").

The earlier fix to the same function, b1fffc273112e ("net: dsa: mv88e6xxx:
bound the policy rule dump by the caller's buffer size"), already uses:

	Fixes: da7dc8755304 ("net: dsa: mv88e6xxx: add RXNFC support")

Without that trailer, stable tooling has nothing to key on for the
unprivileged cross-port read described above.

The commit message says "changing this does carry some regression risk" --
is that hedge the reason the trailer was left out, and could that be stated
explicitly?

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 8cf5681f5806..461167199a71 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2457,7 +2457,7 @@ static int mv88e6xxx_get_rxnfc(struct dsa_switch *ds, int port,
>  	case ETHTOOL_GRXCLSRULE:
>  		err = -ENOENT;
>  		policy = idr_find(&chip->policies, fs->location);
> -		if (policy) {
> +		if (policy && policy->port == port) {
>  			memcpy(fs, &policy->fs, sizeof(*fs));
>  			err = 0;
>  		}

[Severity: Low]
This isn't a bug introduced by this patch, but the delete side of the same
IDR has the identical missing check.  In mv88e6xxx_set_rxnfc():

	case ETHTOOL_SRXCLSRLDEL:
		err = -ENOENT;
		policy = idr_remove(&chip->policies, fs->location);
		if (policy) {
			policy->action = MV88E6XXX_POLICY_ACTION_NORMAL;
			err = mv88e6xxx_policy_apply(chip, port, policy);
			devm_kfree(chip->dev, policy);
		}

A location owned by port A can be removed and freed while
mv88e6xxx_policy_apply() is called with the caller's port B, so the
hardware side effects (mv88e6xxx_port_db_load_purge() and
port_set_policy()) are applied to the wrong port, and the "mapping still in
use" loop filters on policy->port == port:

	if (action == MV88E6XXX_POLICY_ACTION_NORMAL)
		idr_for_each_entry(&chip->policies, policy, id)
			if (policy->port == port && policy->mapping == mapping &&
			    policy->action != action)
				return 0;

This arm sits in the privileged default of dev_ethtool(), so CAP_NET_ADMIN
is required to reach it.

This one is already handled inside the same series -- the following patch,
"net: dsa: mv88e6xxx: check the port when deleting a policy rule", adds the
same policy->port == port test to the delete arm, so nothing remains at the
end of the range.  Noting it here only for completeness.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918185326.3940857-1-kuba%40kernel.org

Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: check the port when deleting a policy rule

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Missing `Fixes:` tag.

Pre-existing issues:
- [Medium] In the ETHTOOL_SRXCLSRLDEL path of mv88e6xxx_set_rxnfc() the
  policy object is unlinked from chip->policies (idr_remove(),…

--

net: dsa: mv88e6xxx: check the port when deleting a policy rule

This changes the ETHTOOL_SRXCLSRLDEL path in mv88e6xxx_set_rxnfc() to look
the policy up with idr_find() and only remove it when policy->port matches
the port the request arrived on, so one port can no longer be used as a
handle to delete another port's rule.

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 461167199a71..70411d147677 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2503,8 +2503,9 @@ static int mv88e6xxx_set_rxnfc(struct dsa_switch *ds, int port,
>  		break;
>  	case ETHTOOL_SRXCLSRLDEL:
>  		err = -ENOENT;
> -		policy = idr_remove(&chip->policies, fs->location);
> -		if (policy) {
> +		policy = idr_find(&chip->policies, fs->location);
> +		if (policy && policy->port == port) {
> +			idr_remove(&chip->policies, fs->location);

[Severity: Medium]
Should this carry a Fixes: tag?

git blame of the pre-image of this hunk attributes the whole delete
sequence to da7dc87553046 ("net: dsa: mv88e6xxx: add RXNFC support"),
which went in for v5.4, so the broken cross-port delete exists in every
stable tree since then.  Without the trailer the stable tooling won't pick
this up:

	Fixes: da7dc87553046 ("net: dsa: mv88e6xxx: add RXNFC support")

>  			policy->action = MV88E6XXX_POLICY_ACTION_NORMAL;
>  			err = mv88e6xxx_policy_apply(chip, port, policy);
>  			devm_kfree(chip->dev, policy);

[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
while the delete path is being touched: should the idr_remove() and
devm_kfree() be conditional on mv88e6xxx_policy_apply() succeeding?

mv88e6xxx_policy_apply() can fail on a register/bus error, both in the ATU
rewrite and in the policy register write:

drivers/net/dsa/mv88e6xxx/chip.c:mv88e6xxx_policy_apply() {
	...
		err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
						   state);
		if (err)
			return err;
	...
	return chip->info->ops->port_set_policy(chip, port, mapping, action);
}

In that case the switch still has the ATU entry in
MV88E6XXX_G1_ATU_DATA_STATE_{UC,MC}_STATIC_POLICY and/or still has the
port's policy register mapping DA/SA to DISCARD, but the software object
and its IDR location are already gone.

Does that leave the rule unaddressable from userspace?  ETHTOOL_GRXCLSRLALL
no longer lists it, a retried ETHTOOL_SRXCLSRLDEL now returns -ENOENT
because idr_find() returns NULL, and the still-programmed mapping is
invisible to the in-use scan at the end of mv88e6xxx_policy_apply():

	if (action == MV88E6XXX_POLICY_ACTION_NORMAL)
		idr_for_each_entry(&chip->policies, policy, id)
			if (policy->port == port &&
			    policy->mapping == mapping &&
			    policy->action != action)
				return 0;

The freed id can also be handed out again by a later ETHTOOL_SRXCLSRLINS,
so a new location would describe hardware programmed for a different MAC.

Doing the idr_remove()/devm_kfree() only after mv88e6xxx_policy_apply()
returns 0 would avoid this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918185326.3940857-1-kuba%40kernel.org

Re: [PATCH net-next 0/2] net: dsa: mv88e6xxx: ethtool n-tuple follow ups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 18 Sep 2026 11:53:24 -0700 you wrote:
> Here are two small tweaks to the mv88e6xxx n-tuple filter handling
> suggested by Sashiko when reviewing commit b1fffc273112 ("net: dsa:
> mv88e6xxx: bound the policy rule dump by the caller's buffer size")
> 
> At the high level the problem is that the driver has shared rule
> table but for some ops it doesn't check whether the rule operated
> on belongs to the port on which the ethtool request was sent.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] net: dsa: mv88e6xxx: check the port when reading back a policy rule
    https://git.kernel.org/netdev/net-next/c/e3624f5eeac4
  - [net-next,2/2] net: dsa: mv88e6xxx: check the port when deleting a policy rule
    https://git.kernel.org/netdev/net-next/c/242ebf4c5114

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html