On Tue, Jul 23, 2024 at 3:31 PM Jakub Kicinski kuba@kernel.org wrote:
The commit under fixes added a questionable check to virtio_net_hdr_to_skb(). I'm guessing the check was supposed to protect from csum offset being outside of a segment (especially if len is not multiple of segment size).
The condition can't be right, tho, as it breaks previously working sending of GSO frames with only one segment (i.e. when gso_size <= len we silently ignore the GSO request and send a single non-GSO frame).
Fix the logic and move it to the GSO part.
I missed the previous patch. Should we revert that and create a new fix against the original issue?
Normally the checksum start + offset should always be in the header, so not even part of gso_size. So needed need not be related to gso_size.
The exception to this is UDP fragmentation offload, I suppose. As there the network and transport headers are part of the UFO payload.
But even for the normal TSO and USO cases we cannot verify in virtio_net_hdr_to_skb that the csum_start + csum_off passed from userspace are really pointing into the transport header.
For SKB_GSO_UDP_L4 I added a minimal check that csum_off must be offsetof(struct udphdr, check). We can arguably tighten these csum_off for all requests, as only UDP and TCP offsets are valid. But no such simple check exists for csum_start. This requires full packet parsing, which we don't do until skb_gso_segment.
One option may be to test csum_start in tcp_gso_segment and udp_gso_fragment and fail segmentation when it points not where expected.
Btw, do we have a better idea what exact packet triggered this WARN_ON_ONCE in skb_checksum_help? Usually, more interesting than the skb_dump of the segment that reached the WARN is the skb_dump at the time of virtio_net_hdr_to_skb, along with the vnet_hdr.
This has been caught by net/tap and net/psock_send.sh tests.
That's very nice!
Fixes: e269d79c7d35 ("net: missing check virtio") Signed-off-by: Jakub Kicinski kuba@kernel.org
if (csum_needed) {unsigned int p_rem, p_size;p_size = gso_size;p_rem = (skb->len - nh_off) % gso_size;if (p_rem)p_size = p_rem;/* Make sure csum still within packet after GSO */if (p_size + nh_off < csum_needed)return -EINVAL;}
A check could even go in the below branch.
The warning apparently was not that csum_needed is outside the segment entirely, but that the segment is non-linear and csum_start points in the non-linear part (offset >= skb_headlen(skb)).
I don't think we should be playing SKBFL_SHARED_FRAG tricks to trigger linearization, to be clear.
We also cannot just silence the WARN and trust that the stack detects these bad packets and drops them (as ip_do_fragment does), as they might end up not in ip_do_fragment, but in a device ndo_start_xmit.
/* Too small packets are not really GSO ones. */ if (skb->len - nh_off > gso_size) { shinfo->gso_size = gso_size;diff --git a/tools/testing/selftests/net/tap.c b/tools/testing/selftests/net/tap.c index 247c3b3ac1c9..8527d51449cf 100644 --- a/tools/testing/selftests/net/tap.c +++ b/tools/testing/selftests/net/tap.c @@ -418,6 +418,36 @@ TEST_F(tap, test_packet_valid_udp_csum) ASSERT_EQ(ret, off); }
+TEST_F(tap, test_packet_invalid_udp_gso_csum) +{
uint8_t pkt[TEST_PACKET_SZ];uint16_t payload;size_t off;int ret;int i;payload = ETH_DATA_LEN - sizeof(struct iphdr) - sizeof(struct udphdr);memset(pkt, 0, sizeof(pkt));off = build_test_packet_valid_udp_gso(pkt, payload);for (i = -16; i < 16; i++) {ret = write(self->fd, pkt, off + i);if (i <= 0 ||i > __builtin_offsetof(struct udphdr, check) + 1) {EXPECT_EQ(ret, off + i)TH_LOG("mismatch with offset: %d (%zd)",i, off + i);} else {EXPECT_EQ(ret, -1)TH_LOG("mismatch with offset: %d (%zd)",i, off + i);EXPECT_EQ(errno, 22);}}+}
TEST_F(tap, test_packet_crash_tap_invalid_eth_proto) { uint8_t pkt[TEST_PACKET_SZ]; -- 2.45.2