netdev - Re: [PATCH net] virtio: fix GSO with frames unaligned to size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAF=yD-LJ+-S4gC9EVo6ijJTGjR6KfPMNPrpxoffgoQBFpo8GNQ@mail.gmail.com>
Date: Tue, 23 Jul 2024 23:48:24 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, mst@...hat.com, jasowang@...hat.com, 
	xuanzhuo@...ux.alibaba.com, eperezma@...hat.com, shuah@...nel.org, 
	arefev@...mel.ru, virtualization@...ts.linux.dev, 
	linux-kselftest@...r.kernel.org, Alexander Duyck <alexander.duyck@...il.com>
Subject: Re: [PATCH net] virtio: fix GSO with frames unaligned to size

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