linux-kernel - Re: [PATCH net v2] net: llc: explicitly set skb->transport

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] [day] [month] [year] [list]

Message-ID: <CANn89i+9Lt78ErDdbgVuOgvSy=UBz2Vhnp=cJYGvwuuQLp6qjg@mail.gmail.com>
Date: Mon, 23 Dec 2024 14:54:16 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Antonio Pastor <antonio.pastor@...il.com>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, horms@...nel.org, 
	kuba@...nel.org, "David S. Miller" <davem@...emloft.net>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] net: llc: explicitly set skb->transport_header

On Mon, Dec 23, 2024 at 2:40 PM Antonio Pastor <antonio.pastor@...il.com> wrote:
>
> 802.2+LLC+SNAP frames received by napi_complete_done with GRO and DSA
> have skb->transport_header set two bytes short, or pointing 2 bytes
> before network_header & skb->data. As snap_rcv expects transport_header
> to point to SNAP header (OID:PID) after LLC processing advances offset
> over LLC header (llc_rcv & llc_fixup_skb), code doesn't find a match
> and packet is dropped.
>
> Between napi_complete_done and snap_rcv, transport_header is not used
> until __netif_receive_skb_core, where originally it was being reset.
> Commit fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> only does so if not set, on the assumption the value was set correctly
> by GRO (and also on assumption that "network stacks usually reset the
> transport header anyway"). Afterwards it is moved forward by
> llc_fixup_skb.
>
> Locally generated traffic shows up at __netif_receive_skb_core with no
> transport_header set and is processed without issue. On a setup with
> GRO but no DSA, transport_header and network_header are both set to
> point to skb->data which is also correct.
>
> As issue is LLC specific, to avoid impacting non-LLC traffic, and to
> follow up on original assumption made on previous code change,
> llc_fixup_skb to reset and advance the offset. llc_fixup_skb already
> assumes the LLC header is at skb->data, and by definition SNAP header
> immediately follows.
>
> Fixes: fda55eca5a33 ("net: introduce skb_transport_header_was_set()")
> Signed-off-by: Antonio Pastor <antonio.pastor@...il.com>


11 years with this stuff being broken...
I wonder if we could remove it from the kernel, given nobody cared.

Can you at the same time fix net/802/psnap.c,
snap_rcv() is probably having the same issue ?

I would also use skb_reset_transport_header() as in

diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
index 51bccfb00a9cd9f16318bbe9a8cc3fe2460912b1..61b0159b2fbee60bdc2623b6c73ed1651b17a050
100644
--- a/net/llc/llc_input.c
+++ b/net/llc/llc_input.c
@@ -124,8 +124,8 @@ static inline int llc_fixup_skb(struct sk_buff *skb)
        if (unlikely(!pskb_may_pull(skb, llc_len)))
                return 0;

-       skb->transport_header += llc_len;
        skb_pull(skb, llc_len);
+       skb_reset_transport_header(skb);
        if (skb->protocol == htons(ETH_P_802_2)) {
                __be16 pdulen;
                s32 data_size;