netdev - Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF

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: <CA+FuTSc-8uO2wUNZH_HhcJ-+SrncyCvJ2NaASCzixpoZQ6cfpQ@mail.gmail.com>
Date:   Fri, 25 Jun 2021 14:43:47 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     David Woodhouse <dwmw2@...radead.org>
Cc:     netdev@...r.kernel.org, Jason Wang <jasowang@...hat.com>,
        Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF_TUN mode

On Thu, Jun 24, 2021 at 8:30 AM David Woodhouse <dwmw2@...radead.org> wrote:
>
> From: David Woodhouse <dwmw@...zon.co.uk>
>
> In tun_get_user(), skb->protocol is either taken from the tun_pi header
> or inferred from the first byte of the packet in IFF_TUN mode, while
> eth_type_trans() is called only in the IFF_TAP mode where the payload
> is expected to be an Ethernet frame.
>
> The equivalent code path in tun_xdp_one() was unconditionally using
> eth_type_trans(), which is the wrong thing to do in IFF_TUN mode and
> corrupts packets.
>
> Pull the logic out to a separate tun_skb_set_protocol() function, and
> call it from both tun_get_user() and tun_xdp_one().

I think this should be two patches. The support for parsing pi is an
independent fix.

> XX: It is not entirely clear to me why it's OK to call eth_type_trans()
> in some cases without first checking that enough of the Ethernet header
> is linearly present by calling pskb_may_pull(). Such a check was never
> present in the tun_xdp_one() code path, and commit 96aa1b22bd6bb ("tun:
> correct header offsets in napi frags mode") deliberately added it *only*
> for the IFF_NAPI_FRAGS mode.

IFF_NAPI_FRAGS exercises napi_gro_frags, which uses the frag0
optimization where all data is in frags. The other receive paths do
not.

For the other cases, linear is guaranteed to include the link layer
header. __tun_build_skb, for instance, just allocates one big
skb->data. It is admittedly not trivial to prove this point
exhaustively for all paths.

commit 96aa1b22bd6bb restricted the new test to the frags case, to
limit the potential blast radius of a bug fix to only the code path
affected by the bug.

> I would like to see specific explanations of *why* it's ever valid and
> necessary (is it so much faster?) to skip the pskb_may_pull()

It was just not needed and that did not complicate anything until this
patch. It's fine to unconditionally check if that simplifies this
change.