[PATCH 1/2 net-next] net: openvswitch: skip IPv4 later fragments on TCP/UDP set actions

[PATCH 1/2 net-next] net: openvswitch: skip IPv4 later fragments on TCP/UDP set actions

[Fernando Fernandez Mancera]

An IPv4 later fragment carries no L4 header, but key extraction leaves
ip.proto set to the real protocol and the flow may wildcard the frag
field, so validate_set() accepts such packets. set_tcp() and set_udp()
then treat the first bytes of the fragment payload as an L4 header,
rewrite them as ports and patch the bytes at the checksum offset,
corrupting the packet.

Skip these actions for OVS_FRAG_TYPE_LATER. IPv6 later fragments are not
affected, since their ip.proto is NEXTHDR_FRAGMENT and validate_set()
rejects L4 set actions for them.

Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/openvswitch/actions.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 27152bca8a8c..4aade471afc4 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -595,6 +595,9 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	__be16 src, dst;
 	int err;
 
+	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
+		return 0;
+
 	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
 				  sizeof(struct udphdr));
 	if (unlikely(err))
@@ -638,6 +641,9 @@ static int set_tcp(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	__be16 src, dst;
 	int err;
 
+	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
+		return 0;
+
 	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
 				  sizeof(struct tcphdr));
 	if (unlikely(err))
-- 
2.55.0

[PATCH 2/2 net-next] net: openvswitch: drop IP fragments on SCTP set action

[Fernando Fernandez Mancera]

The SCTP checksum is a CRC32c over the whole SCTP packet. set_sctp()
updates it as old_csum ^ old_correct_csum ^ new_csum, with the two CRCs
computed over the bytes of the current skb only. That is correct only
when the skb holds the whole SCTP packet. For a first fragment, and for
a later fragment, it holds just a part of it, and the CRC delta of a
change at the start of the packet depends on how many bytes follow, so
the checksum written is wrong. The receiver drops the reassembled
packet.

validate_set() accepts SCTP set action for any flow with ip.proto set to
SCTP, regardless of the fragmentation bits, as it only receives the
masked key. As the checksum cannot be fixed up without the whole
datagram, fail the action for packets that are IP fragments of either
family, so the packet is dropped with OVS_DROP_ACTION_ERROR instead of
being forwarded with modified ports and an invalid checksum.

Link: https://lore.kernel.org/netdev/d4fc14f3-4308-4560-9012-43c09c1b3845@ovn.org/
Fixes: a175a723301a ("openvswitch: Add SCTP support")
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/openvswitch/actions.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 4aade471afc4..12e339cbf791 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -674,6 +674,9 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	__le32 old_correct_csum, new_csum, old_csum;
 	int err;
 
+	if (flow_key->ip.frag != OVS_FRAG_TYPE_NONE)
+		return -EINVAL;
+
 	err = skb_ensure_writable(skb, sctphoff + sizeof(struct sctphdr));
 	if (unlikely(err))
 		return err;
-- 
2.55.0

Re: [PATCH 1/2 net-next] net: openvswitch: skip IPv4 later fragments on TCP/UDP set actions

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH 1/2 net-next] net: openvswitch: skip IPv4 later fragments on TCP/UDP set actions

[Ilya Maximets]

On 10/15/26 2:53 PM, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

Sashiko report while reviewing another patch, IIRC.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

It can be triggered manually, but should not be triggerable in any
real OVS setups, as ovs-vswitchd always has the fragmentation bits
exactly matched and will not produce set(tcp/udp) actions for later
fragments.

Best regards, Ilya Maximets.

Re: [PATCH 1/2 net-next] net: openvswitch: skip IPv4 later fragments on TCP/UDP set actions

[Ilya Maximets]

On 10/15/26 2:47 PM, Fernando Fernandez Mancera wrote:
> An IPv4 later fragment carries no L4 header, but key extraction leaves
> ip.proto set to the real protocol and the flow may wildcard the frag
> field, so validate_set() accepts such packets. set_tcp() and set_udp()
> then treat the first bytes of the fragment payload as an L4 header,
> rewrite them as ports and patch the bytes at the checksum offset,
> corrupting the packet.
> 
> Skip these actions for OVS_FRAG_TYPE_LATER. IPv6 later fragments are not
> affected, since their ip.proto is NEXTHDR_FRAGMENT and validate_set()
> rejects L4 set actions for them.
> 
> Fixes: ccb1352e76cf ("net: Add Open vSwitch kernel components.")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/openvswitch/actions.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 27152bca8a8c..4aade471afc4 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -595,6 +595,9 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>  	__be16 src, dst;
>  	int err;
>  
> +	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
> +		return 0;
> +
>  	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
>  				  sizeof(struct udphdr));
>  	if (unlikely(err))
> @@ -638,6 +641,9 @@ static int set_tcp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>  	__be16 src, dst;
>  	int err;
>  
> +	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
> +		return 0;
> +
>  	err = skb_ensure_writable(skb, skb_transport_offset(skb) +
>  				  sizeof(struct tcphdr));
>  	if (unlikely(err))

Ideally, we would not allow this actions to be installed in the first place,
but that would require quite a bit of rework in the flow parsing and needs
some extra assessment on the uAPI impact.  Though it would be a more complete
fix and LLMs may complain that the current patch is not.  But, IMO, it's
good enough for what it is trying to achieve right now.

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH 2/2 net-next] net: openvswitch: drop IP fragments on SCTP set action

[Ilya Maximets]

On 10/15/26 2:47 PM, Fernando Fernandez Mancera wrote:
> The SCTP checksum is a CRC32c over the whole SCTP packet. set_sctp()
> updates it as old_csum ^ old_correct_csum ^ new_csum, with the two CRCs
> computed over the bytes of the current skb only. That is correct only
> when the skb holds the whole SCTP packet. For a first fragment, and for
> a later fragment, it holds just a part of it, and the CRC delta of a
> change at the start of the packet depends on how many bytes follow, so
> the checksum written is wrong. The receiver drops the reassembled
> packet.
> 
> validate_set() accepts SCTP set action for any flow with ip.proto set to
> SCTP, regardless of the fragmentation bits, as it only receives the
> masked key. As the checksum cannot be fixed up without the whole
> datagram, fail the action for packets that are IP fragments of either
> family, so the packet is dropped with OVS_DROP_ACTION_ERROR instead of
> being forwarded with modified ports and an invalid checksum.
> 
> Link: https://lore.kernel.org/netdev/d4fc14f3-4308-4560-9012-43c09c1b3845@ovn.org/
> Fixes: a175a723301a ("openvswitch: Add SCTP support")
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/openvswitch/actions.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 4aade471afc4..12e339cbf791 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -674,6 +674,9 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>  	__le32 old_correct_csum, new_csum, old_csum;
>  	int err;
>  
> +	if (flow_key->ip.frag != OVS_FRAG_TYPE_NONE)
> +		return -EINVAL;
> +
>  	err = skb_ensure_writable(skb, sctphoff + sizeof(struct sctphdr));
>  	if (unlikely(err))
>  		return err;

Similar story here as with the previous patch.

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH 2/2 net-next] net: openvswitch: drop IP fragments on SCTP set action

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 2

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).

- [High] net/openvswitch: set_sctp corrupts SCTP checksum for padded
  packets
- [Medium] In net/openvswitch/actions.c, set_sctp() now returns -EINVAL
  whenever `flow_key->ip.frag != OVS_FRAG_TYPE_NONE`.
- [Low] The commit message says first and later fragments failed the same
  way: the skb holds part of the SCTP packet and 'the CRC delta of a…
- [Low] set_tcp() and set_udp() (actions.c:644-645, 598-599) skip
  OVS_FRAG_TYPE_LATER packets: they return 0 and the action list…

--

net: openvswitch: drop IP fragments on SCTP set action

This patch makes set_sctp() return -EINVAL for any packet whose flow key
has ip.frag set to anything other than OVS_FRAG_TYPE_NONE. The goal is to
drop SCTP fragments with OVS_DROP_ACTION_ERROR instead of forwarding them
with rewritten ports and an invalid CRC32c checksum.

Link: https://lore.kernel.org/netdev/d4fc14f3-4308-4560-9012-43c09c1b3845@ovn.org/

> For a first fragment, and for
> a later fragment, it holds just a part of it, and the CRC delta of a
> change at the start of the packet depends on how many bytes follow, so
> the checksum written is wrong.

[Severity: Low]
Is this the right description of what happened to later fragments?

An IPv4 later fragment has no SCTP header. key_extract() in
net/openvswitch/flow.c sets OVS_FRAG_TYPE_LATER but leaves ip.proto as
IPPROTO_SCTP:

	offset = nh->frag_off & htons(IP_OFFSET);
	if (offset) {
		key->ip.frag = OVS_FRAG_TYPE_LATER;
		memset(&key->tp, 0, sizeof(key->tp));
		return 0;
	}

So validate_set() accepts the action. The old set_sctp() then wrote the
"ports" over payload bytes 0-3, wrote a computed "checksum" over payload
bytes 8-11, and copied payload bytes into flow_key->tp. That reads as
silent payload corruption rather than just a wrong CRC. It is the same
problem the earlier patch in this series ("net: openvswitch: skip IPv4
later fragments on TCP/UDP set actions") describes for TCP/UDP.

IPv6 later fragments never reach set_sctp(), because parse_ipv6hdr() sets
ip.proto to NEXTHDR_FRAGMENT:

		if (frag_off) {
			key->ip.frag = OVS_FRAG_TYPE_LATER;
			key->ip.proto = NEXTHDR_FRAGMENT;
			return 0;
		}

validate_set() then rejects SCTP set actions on flows that could match
them.

> fail the action for packets that are IP fragments of either
> family

In practice this covers IPv4 first and later fragments plus IPv6 first
fragments. The code does handle the IPv4 later fragment case. Could the
commit message describe the old behaviour for each case, to help anyone
judging impact or backporting?

> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 4aade471afc45..12e339cbf7912 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -674,6 +674,9 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *flow_key,
>  	__le32 old_correct_csum, new_csum, old_csum;
>  	int err;
>  
> +	if (flow_key->ip.frag != OVS_FRAG_TYPE_NONE)
> +		return -EINVAL;

[Severity: High]
This patch doesn't introduce it, but does the same CRC delta problem
also hit unfragmented packets that carry trailing link layer padding?

sctp_compute_cksum() covers skb->len - sctphoff bytes. As far as I can
see, nothing on the OVS receive path trims skb->len down to the IPv4
total length or the IPv6 payload length before actions run. The only
trim I can find is ovs_skb_network_trim() in
net/openvswitch/conntrack.c, and that only runs for ct actions. So a
small SCTP packet that was padded to the 60 byte Ethernet minimum
reaches set_sctp() with the pad bytes still in the skb.

In that case old_correct_csum and new_csum are both computed over the
SCTP packet plus the padding. As the commit message says, the CRC delta
of a change at the start of the packet depends on how many bytes follow
it. That makes old_correct_csum ^ new_csum the wrong delta for the
receiver, and old_correct_csum also no longer matches a correct
old_csum. The value written to sh->checksum is wrong even for a packet
that arrived with a valid checksum. ip_rcv() or ipv6_rcv() on the
receiver trims the padding, sctp_rcv() then sees a CRC32c mismatch, and
the packet is dropped.

This looks easy to hit. Over IPv4, an SCTP packet that carries only a
COOKIE ACK, SHUTDOWN ACK or SHUTDOWN COMPLETE chunk is 36 bytes of IP,
well under the Ethernet minimum. A flow that rewrites SCTP ports, for
example for NAT, would then corrupt those chunks. I think losing COOKIE
ACK would keep an association from ever being established.

Could set_sctp() limit the CRC computation to the length taken from the
IP header? Another option would be to trim the skb first, the way
ovs_skb_network_trim() does. Do you want to handle this in this series,
or as a separate fix?

[Severity: Medium]
Does this also drop IPv6 atomic fragments? These are packets with a
Fragment header whose offset is 0 and whose M bit is clear, so the skb
holds the complete SCTP packet.

parse_ipv6hdr() in net/openvswitch/flow.c marks any Fragment header with
a zero offset as OVS_FRAG_TYPE_FIRST:

	nexthdr = ipv6_find_hdr(skb, &payload_ofs, -1, &frag_off, &flags);
	if (flags & IP6_FH_F_FRAG) {
		if (frag_off) {
			...
		}
		key->ip.frag = OVS_FRAG_TYPE_FIRST;

ipv6_find_hdr() in net/ipv6/exthdrs_core.c sets IP6_FH_F_FRAG whenever a
Fragment header is present, and it masks off the M bit:

	_frag_off = ntohs(*fp) & ~0x7;

As a result, set_sctp() cannot tell an atomic fragment from a real first
fragment.

Before this patch, sctp_compute_cksum() covered the whole SCTP packet in
this case, so the old_csum ^ old_correct_csum ^ new_csum update was
correct and the packet was forwarded with its ports rewritten. Now
do_execute_actions() frees it with OVS_DROP_ACTION_ERROR and skips the
rest of the action list.

RFC 8021 deprecates generating atomic fragments, but RFC 6946 still says
receivers must process them.

Could the check look at the M bit, or confirm that the skb covers the
full IPv6 payload, rather than rejecting every OVS_FRAG_TYPE_FIRST packet?
IPv4 SCTP looks fine here, because key_extract() only sets
OVS_FRAG_TYPE_FIRST for it when IP_MF is set.

[Severity: Low]
This isn't a bug, but should later fragments be handled the same way here
as in set_tcp() and set_udp()? Both of those treat OVS_FRAG_TYPE_LATER as
a successful no-op:

	if (flow_key->ip.frag == OVS_FRAG_TYPE_LATER)
		return 0;

This check returns -EINVAL for both FIRST and LATER. Take a flow that
wildcards ip.frag and rewrites L4 ports. IPv4 TCP/UDP later fragments are
forwarded unchanged and the action list continues. IPv4 SCTP later
fragments are dropped with OVS_DROP_ACTION_ERROR, and any actions after
the set are skipped.

The commit message says the drop is intended. The same flow drops the
first fragment anyway, so the visible difference is mostly drop
accounting and whether later actions run.

The uAPI description of OVS_ACTION_ATTR_SET in
include/uapi/linux/openvswitch.h also doesn't mention that a validated
SCTP set can fail at runtime on fragments while TCP/UDP sets are skipped.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015124749.4694-1-fmancera%40suse.de