Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Apr 2023 08:44:52 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To:     "luwei (O)" <luwei32@huawei.com>,
        Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
        Eric Dumazet <edumazet@google.com>
Cc:     "davem@davemloft.net" <davem@davemloft.net>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "asml.silence@gmail.com" <asml.silence@gmail.com>,
        "imagedong@tencent.com" <imagedong@tencent.com>,
        "brouer@redhat.com" <brouer@redhat.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "jbenc@redhat.com" <jbenc@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Message-ID: <643e90c4c7d0b_327ccc294a8@willemb.c.googlers.com.notmuch>
In-Reply-To: <0f1d25fa-0704-f3b0-cc33-d89a5f87daac@huawei.com>
References: <20230410022152.4049060-1-luwei32@huawei.com>
 <CANn89iKFLREJV_cfHEk6wz6xXVv_jSrZ_UyXAB8VpH7gMXacxQ@mail.gmail.com>
 <643447ba5224a_83e69294b6@willemb.c.googlers.com.notmuch>
 <450994d7-4a77-99df-6317-b535ea73e01d@huawei.com>
 <CANn89iLOcvDRMi9kVr86xNp5=h4JWpx9yYWicVxCwSMgAJGf_g@mail.gmail.com>
 <c90abe8c-ffa0-f986-11eb-bde65c84d18b@huawei.com>
 <6436b5ba5c005_41e2294dd@willemb.c.googlers.com.notmuch>
 <a30a8ffaa8dd4cb6a84103eecf0c3338@huawei.com>
 <643983f69b440_17854f2948c@willemb.c.googlers.com.notmuch>
 <64398b4c4585f_17abe429442@willemb.c.googlers.com.notmuch>
 <47fca2c7-db7c-0265-d724-38dffc62debe@huawei.com>
 <643aab0f2f39c_1afa5d2943f@willemb.c.googlers.com.notmuch>
 <0f1d25fa-0704-f3b0-cc33-d89a5f87daac@huawei.com>
Subject: =?UTF-8?Q?Re:_=E7=AD=94=E5=A4=8D:_[PATCH_net]_net:_Add_check_for?=
 =?UTF-8?Q?_csum=5Fstart_in_skb=5Fpartial=5Fcsum=5Fset=28=29?=
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

luwei (O) wrote:
> =

> =E5=9C=A8 2023/4/15 9:47 PM, Willem de Bruijn =E5=86=99=E9=81=93:
> > luwei (O) wrote:
> >> =E5=9C=A8 2023/4/15 1:20 AM, Willem de Bruijn =E5=86=99=E9=81=93:
> >>> Willem de Bruijn wrote:
> >>>> luwei (O) wrote:
> >>>>> yes, here is the vnet_hdr:
> >>>>>
> >>>>>       flags: 3
> >>>>>       gso_type: 3
> >>>>>       hdr_len: 23
> >>>>>       gso_size: 58452
> >>>>>       csum_start: 5
> >>>>>       csum_offset: 16
> >>>>>
> >>>>> and the packet:
> >>>>>
> >>>>> | vnet_hdr | mac header | network header | data ... |
> >>>>>
> >>>>>     memcpy((void*)0x20000200,
> >>>>>            "\x03\x03\x02\x00\x54\xe4\x05\x00\x10\x00\x80\x00\x00\=
x53\xcc\x9c\x2b"
> >>>>>            "\x19\x3b\x00\x00\x00\x89\x4f\x08\x03\x83\x81\x04",
> >>>>>            29);
> >>>>>     *(uint16_t*)0x200000c0 =3D 0x11;
> >>>>>     *(uint16_t*)0x200000c2 =3D htobe16(0);
> >>>>>     *(uint32_t*)0x200000c4 =3D r[3];
> >>>>>     *(uint16_t*)0x200000c8 =3D 1;
> >>>>>     *(uint8_t*)0x200000ca =3D 0;
> >>>>>     *(uint8_t*)0x200000cb =3D 6;
> >>>>>     memset((void*)0x200000cc, 170, 5);
> >>>>>     *(uint8_t*)0x200000d1 =3D 0;
> >>>>>     memset((void*)0x200000d2, 0, 2);
> >>>>>     syscall(__NR_sendto, r[1], 0x20000200ul, 0xe45ful, 0ul, 0x200=
000c0ul, 0x14ul);
> >>>> Thanks. So this can happen whenever a packet is injected into the =
tx
> >>>> path with a virtio_net_hdr.
> >>>>
> >>>> Even if we add bounds checking for the link layer header in pf_pac=
ket,
> >>>> it can still point to the network header.
> >>>>
> >>>> If packets are looped to the tx path, skb_pull is common if a pack=
et
> >>>> traverses tunnel devices. But csum_start does not directly matter =
in
> >>>> the rx path (CHECKSUM_PARTIAL is just seen as CHECKSUM_UNNECESSARY=
).
> >>>> Until it is forwarded again to the tx path.
> >>>>
> >>>> So the question is which code calls skb_checksum_start_offset on t=
he
> >>>> tx path. Clearly, skb_checksum_help. Also a lot of drivers. Which
> >>>> may cast the signed int return value to an unsigned. Even an u8 in=

> >>>> the first driver I spotted (alx).
> >>>>
> >>>> skb_postpull_rcsum anticipates a negative return value, as do othe=
r
> >>>> core functions. So it clearly allowed in certain cases. We cannot
> >>>> just bound it.
> >>>>
> >>>> Summary after a long story: an initial investigation, but I don't =
have
> >>>> a good solution so far. Maybe others have a good suggestiong based=
 on
> >>>> this added context.
> >>> Specific to skb_checksum_help, it appears that skb_checksum will
> >>> work with negative offset just fine.
> >>   =C2=A0=C2=A0=C2=A0 =C2=A0 In this case maybe not, since it checksu=
ms from within the mac
> >> header, and the mac header
> >>
> >>   =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 will be stripped when the rx =
path checks the checksum.
> > The header is pulled, but still present. Obviously something bogus ge=
ts
> > written if the virtio_net_hdr configures csum offload with a bogus
> > offset. But as long as the offset is zero or positive from skb->head,=

> > the checksum helper works as intended.
> =

>  =C2=A0=C2=A0 OK, Thanks for your reply

We still should address the unnecessary warning triggerable by syzbot.

If I'm correct that any offset programmable through virtio_net_hdr ends
up in the skb linear section and skb_checksum_help will compute it fine,
then the WARN_ON_ONCE just needs an explicit cast to signed.

I have only skimmed, so not 100% sure yet. But that's the short take.
 =

> >   =

> >>> Perhaps the only issue is that the WARN_ON_ONCE compares signed to
> >>> unsigned, and thus incorrectly interprets a negative offset as
> >>>    >=3D skb_headlen(skb)
> > .
> =

> -- =

> Best Regards,
> Lu Wei
> =