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

Message-ID: <CANn89i+jk=OWS3L1OV-aVbeO85eU6u6yK=FRoRadNEihpAOcHQ@mail.gmail.com>
Date: Mon, 23 Dec 2024 19:18:15 +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 6:00 PM Antonio Pastor <antonio.pastor@...il.com> wrote:
>
> On 2024-12-23 08:54, Eric Dumazet wrote:
>
> 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>
>
> Thanks for your reply.
>
> 11 years with this stuff being broken...
>
> It works great without DSA thrown in the mix, for some reason I have not
> been able to determine, not for the lack of trying. Wonder how many
> configurations use DSA and care about SNAP frames.
> DSA was added recently to the implementation I'm working on, so for
> practical purposes this is just broken since 1-2 years ago. Tripped on it
> about 6 months ago. It's fresh. Or well aged. :)
>
> I wonder if we could remove it from the kernel, given nobody cared.
>
> Well, at least I do, and ballpark 40 other people for what I can tell.
> Some others might be lurking in the sidelines.
>
> Can you at the same time fix net/802/psnap.c,
> snap_rcv() is probably having the same issue ?
>
> That's how I got here, because of snap_rcv(). But no fix is required at
> snap_rcv(). This patch is to fix problem there.
> snap_rcv() doesn't advance the transport_header offset or pull the skb as
> far as I saw so adding a reset there, that would be redundant. Once
> this piece of code determines LLC header size SNAP header always follows
> for SNAP frames.
> I'll double-check and submit a v3 w/that included if I think it makes
> sense but honestly I don't think it does.

I see skb->transport_header being advanced at line 61 :

    /* Pass the frame on. */
    skb->transport_header += 5;


Note that setting the transport header (conditionally or not) in
__netif_receive_skb()
is probably a mistake. Only network (ipv4, ipv6) handlers can possibly
know the concept
of transport header.

Hopefully at some point we can remove this defensive code.

diff --git a/net/core/dev.c b/net/core/dev.c
index c7f3dea3e0eb9eb05865e49dd7a8535afb974149..b6722e11ee4e171e6a2f379fc1d0197dfcb1a842
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5474,8 +5474,6 @@ static int __netif_receive_skb_core(struct
sk_buff **pskb, bool pfmemalloc,
        orig_dev = skb->dev;

        skb_reset_network_header(skb);
-       if (!skb_transport_header_was_set(skb))
-               skb_reset_transport_header(skb);
        skb_reset_mac_len(skb);

        pt_prev = NULL;