On Tue, Jul 23, 2024 at 3:31 PM Jakub Kicinski <kuba@xxxxxxxxxx> 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@xxxxxxxxxx>
> + 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
>
