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

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/17/26 6:38 AM, netdev-bot+sashiko@kernel.org wrote:
> 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: 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?
> 

This might be right. I need to look better into SCTP and openvswitch 
flows, I will write this down and look into it later. I think it should 
be a separated patch

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

Yes, it does that and AFAIU, we do not care about it. Indeed, the next 
step is to reject the flow directly for fragments.

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

Based on the discussions with the maintainers, we do not care about it 
and in the future we are rejecting it directly.