Re: [PATCH] net/sched: act_pedit: require matching IPv4 L4 protocol

[Victor Nogueira]

On Mon, Jun 8, 2026 at 7:46 AM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
>
> On Sun, Jun 7, 2026 at 10:33 AM Samuel Moelius
> <sam.moelius@xxxxxxxxxxxxxxx> wrote:
> >
> > On Sat, Jun 6, 2026 at 5:29 PM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Jun 5, 2026 at 3:46 PM Samuel Moelius
> > > <sam.moelius@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > The extended IPv4 L4 header mode in act_pedit can select TCP or UDP
> > > > header fields without confirming that the IPv4 protocol field matches
> > > > the selected transport header.
> > > >
> > > > That lets a rule written for TCP or UDP modify unrelated payload bytes
> > > > in a packet carrying a different protocol.
> > > >
> > > > Verify the IPv4 protocol before applying TCP or UDP extended header
> > > > edits.
> > > >
> > > > Assisted-by: Codex:gpt-5.5-cyber-preview
> > > > Signed-off-by: Samuel Moelius <sam.moelius@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  net/sched/act_pedit.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > > > index bc20f08a2789..9a4590451f7e 100644
> > > > --- a/net/sched/act_pedit.c
> > > > +++ b/net/sched/act_pedit.c
> > > > @@ -341,6 +341,8 @@ static int pedit_l4_skb_offset(struct sk_buff *skb, int *hoffset, const int head
> > > >
> > > >                 if (!iph)
> > > >                         goto out;
> > > > +               if (iph->ihl < 5 || iph->protocol != header_type)
> > > > +                       goto out;
> > >
> > > From inspection the fix looks reasonable.
> > > At first glance it seems that the only fix that resolves the issue you
> > > are describing is to check the protocol header.
> > > header length seems to be an extra thing. But let's do what the v6
> > > side seems to and skip frags as well? Something maybe along the lines
> > > of:
> > >
> > > if (iph->ihl < 5 || iph->protocol != header_type || (iph->frag_off &
> > > htons(IP_OFFSET)))
> > >         goto out;
> >
> > I will submit an updated patch.
> >
> > > Are you able to describe how someone would configure a tc rule that
> > > will allow this to happen?
> > > IOW, how did you verify the problem and then validate that your fix works?
> >
> > The bug was validated using a custom init script in QEMU. The specific
> > line used was:
> >
> > /usr/sbin/tc filter add dev veth0 egress protocol ip pref 1 matchall
> > action pedit ex munge udp dport set 18053
> >
> > An IPv4 TCP packet with destination port 2222 was sent through that
> > path. With the unpatched kernel, the packet was received with TCP
> > destination port 18053. With the patched kernel, the same packet was
> > received unchanged.
> >
> > I can provide more details or artifacts if you would like.
>
> A tdc test case is better. Victor or I can create one for you if you
> dont know how.

It seems like v2 isn't applying so you'll have to send a new version.
When you do, after waiting for the 24h period, can you resend with
the attached patch (containing the tdc test)? Since this is a
fix, in the new version, remember to target net and add a proper fixes
tag.

cheers,
Victor
From 9fc039564addac0312ceb9c9210cb93ea44e8a9f Mon Sep 17 00:00:00 2001
From: Victor Nogueira <victor@xxxxxxxxxxxx>
Date: Mon, 8 Jun 2026 11:10:05 -0300
Subject: [PATCH net v3] selftests/tc-testing: Verify pedit does not mangle
 mismatched L4 protocol

Add a tdc test that checks the act_pedit extended L4 header mode does not
edit a packet whose IPv4 protocol does not match the selected transport
header.

The test installs an ingress pedit rule that sets the UDP destination
port, then injects a TCP packet with dport 2222. The UDP and TCP
destination ports sit at the same L4 offset, so a buggy kernel rewrites
the TCP dport. A second flower filter matches TCP dport 2222 and drops
the packet through an indexed gact action; the test then verifies via
JSON that this action saw exactly one packet, i.e. the dport was left
untouched and still matched 2222.

Signed-off-by: Victor Nogueira <victor@xxxxxxxxxxxx>
---
 .../tc-testing/tc-tests/actions/pedit.json    | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json b/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
index 37c4103321749..d8b685cfc62de 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
@@ -1920,5 +1920,54 @@
         "teardown": [
             "$TC actions flush action pedit"
         ]
+    },
+    {
+        "id": "1a4f",
+        "name": "Pedit udp dport should not mangle TCP packet dport",
+        "category": [
+            "actions",
+            "pedit"
+        ],
+        "plugins": {
+            "requires": [
+                "nsPlugin",
+                "scapyPlugin"
+            ]
+        },
+        "setup": [
+            "$TC qdisc add dev $DEV1 clsact",
+            "$TC filter add dev $DEV1 ingress protocol ip pref 1 matchall action pedit ex munge udp dport set 18053 continue"
+        ],
+        "cmdUnderTest": "$TC filter add dev $DEV1 ingress protocol ip pref 2 flower ip_proto tcp dst_port 2222 action drop index 1",
+        "scapy": {
+            "iface": "$DEV0",
+            "count": 1,
+            "packet": "Ether()/IP(dst='10.10.10.1')/TCP(dport=2222)"
+        },
+        "expExitCode": "0",
+        "verifyCmd": "$TC -j -s actions get action gact index 1",
+        "matchJSON": [
+            {
+                "total acts": 0
+            },
+            {
+                "actions": [
+                    {
+                        "order": 1,
+                        "kind": "gact",
+                        "control_action": {
+                            "type": "drop"
+                        },
+                        "index": 1,
+                        "stats": {
+                            "packets": 1
+                        }
+                    }
+                ]
+            }
+        ],
+        "teardown": [
+            "$TC qdisc del dev $DEV1 clsact"
+        ]
     }
 ]
-- 
2.54.0