Re: [PATCH net 1/3] xsk: validate checksum offload requests in copy mode

["Björn Töpel" <bjorn@kernel.org>]

Nick Simpson <nick.simo@gmail.com> writes:

> In copy mode, XDP_TXMD_FLAGS_CHECKSUM turns the csum_start and
> csum_offset from the TX metadata into a CHECKSUM_PARTIAL skb.
> xsk_skb_metadata() only checks that the checksum fits in the frame, so
> csum_start can point into the Ethernet or IP header.
>
> On a veth, the skb reaches the peer namespace still CHECKSUM_PARTIAL.
> When that namespace forwards IPv6 with nf_defrag_ipv6 loaded,
> nf_ct_frag6_reasm() moves the IPv6 header forward by the 8 bytes of the
> fragment header without adjusting csum_start, the forward hook accepts
> the reassembled packet, and ip6_fragment() then completes the checksum
> with skb_checksum_help(), writing two bytes into the accepted header.
> With csum_start = 22 and csum_offset = 22 on a fragmented IPv6/UDP
> datagram from fd00:1::100, the write lands on source address bytes
> 14-15: an nftables forward rule matching "ip6 saddr fd00:1::100"
> accepted the packet and the fragments sent out carried saddr
> fd00:1::e87. For an unfragmented packet the same write happens at
> egress when the output device does not offload the checksum; with
> csum_start = 14 and csum_offset = 22 the packet left as fd00:1::ebf. A
> csum_start that ends up before the network header (14 to 21 on the
> fragmented path) makes skb_checksum_help() warn instead:
>
>   offset (-2) >= skb_headlen() (640)
>   WARNING: net/core/dev.c:3647 at skb_checksum_help+0x4c8/0x850
>   Call trace:
>    skb_checksum_help
>    ip6_fragment
>    ip6_finish_output
>    ip6_output
>    ip6_forward
>    ipv6_rcv
>    ...
>    __dev_direct_xmit
>    __xsk_generic_xmit
>    xsk_sendmsg
>
> socket(AF_XDP) only requires CAP_NET_RAW in the user namespace that owns
> the network namespace, so the sender can be a container that owns one
> end of the veth. With panic_on_warn, the warning panics the forwarding
> host.
>
> The interface is documented for L4 checksums. Leave CHECKSUM_PARTIAL in
> place only when the request designates the TCP or UDP checksum field of
> an unfragmented IPv4 or IPv6 packet, not IP-in-IP or GRE encapsulated,
> on an Ethernet device, and complete any other request in software, as
> XDP_UMEM_TX_SW_CSUM would, before the frame is handed to the device.
> The frame is dissected from its Ethernet type, as skb->protocol is not
> set for xsk skbs. The pending "net: resolve uncommon partial checksums
> at ingress" applies the same rule to virtio_net_hdr sources.
>
> A request on a frame that is itself an IP fragment is therefore
> completed over that fragment alone, which is also all a device could
> compute, so the reassembled datagram does not carry a valid transport
> checksum. Before, if every fragment carried such a request, the
> reassembled datagram stayed CHECKSUM_PARTIAL and was completed on
> refragmentation; now conntrack marks it invalid.
>
> Skbs without a linear part (IFF_TX_SKB_NO_LINEAR, i.e. virtio_net) are
> left unchanged: their data stays in the umem, where user space can
> still modify it after any check, and they go to the device rather than
> to the local stack.
>
> The problem was found with LLM-assisted code review.

AI;DR. Ask you agent to be terse, please. Also, please do a proper write
down of how to test/reproduce. The slop in the cover is no fun to read.
:-(

> @@ -1277,6 +1330,11 @@ static int __xsk_generic_xmit(struct sock *sk)
>  			continue;
>  		}
>  
> +		if (unlikely(xsk_skb_resolve_csum(skb))) {
> +			xsk_drop_skb(skb);
> +			continue;
> +		}
> +

Yes, we need to validate the csum offsets, similar to the virtio-net
work MST is doing. Look what MST is doing in [1]. When that lands, move
the helpers to ip.h as the TODO says, and use for XSK.

I *think* the IFF_TX_SKB_NO_LINEAR csum_partial scenario is OK, since
we'll end up at a virtio device, but TBH I'm a bit skeptic about the
whole reference to UMEM via an skb case...


Björn

[1] 5de86f1afe1165a1a13ac4ef0615b19f4a10cb7a.1792565282.git.mst@redhat.com