linux-kernel - Re: [PATCH] af_packet: Handle outgoing VLAN packets without hardware offloading

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: <CAPza5qcGyfcUYOoznci4e=1eaScVTgkzAhXfKSG3bTzC=aOwew@mail.gmail.com>
Date: Tue, 21 May 2024 11:31:42 +0800
From: Chengen Du <chengen.du@...onical.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] af_packet: Handle outgoing VLAN packets without hardware offloading

Hi Willem,

Thank you for your response.

I would appreciate any suggestions you could offer, as I am not as
familiar with this area as you are.

I encountered an issue while capturing packets using tcpdump, which
leverages the libpcap library for sniffing functionalities.
Specifically, when I use "tcpdump -i any" to capture packets and
hardware VLAN offloading is unavailable, some bogus packets appear.
In this scenario, Linux uses cooked-mode capture (SLL) for the "any"
device, reading from a PF_PACKET/SOCK_DGRAM socket instead of the
usual PF_PACKET/SOCK_RAW socket.

Using SOCK_DGRAM instead of SOCK_RAW means that the Linux socket code
does not supply the packet's link-layer header.
Based on the code in af_packet.c, SOCK_DGRAM strips L2 headers from
the original packets and provides SLL for some L2 information.
>From the receiver's perspective, the VLAN information can only be
parsed from SLL, which causes issues if the kernel stores VLAN
information in the payload.

As you mentioned, this modification affects existing PF_PACKET receivers.
For example, libpcap needs to change how it parses VLAN packets with
the PF_PACKET/SOCK_RAW socket.
The lack of VLAN information in SLL may prevent the receiver from
properly decoding the L3 frame in cooked mode.

I am new to this area and would appreciate it if you could kindly
correct any misunderstandings I might have about the mechanism.
I would also be grateful for any insights you could share on this issue.
Additionally, I am passionate about contributing to resolving this
issue and am willing to work on patches based on your suggestions.

Thank you for your time and assistance.

Best regards,
Chengen Du

On Tue, May 21, 2024 at 2:35 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Chengen Du wrote:
> > In the outbound packet path, if hardware VLAN offloading is unavailable,
> > the VLAN tag is inserted into the payload but then cleared from the
> > metadata. Consequently, this could lead to a false negative result when
> > checking for the presence of a VLAN tag using skb_vlan_tag_present(),
> > causing the packet sniffing outcome to lack VLAN tag information. As a
> > result, the packet capturing tool may be unable to parse packets as
> > expected.
> >
> > Signed-off-by: Chengen Du <chengen.du@...onical.com>
>
> This is changing established behavior, which itself may confuse
> existing PF_PACKET receivers.
>
> The contract is that the VLAN tag can be observed in the payload or
> as tp_vlan_* fields if it is offloaded.
>
> > @@ -2457,7 +2464,8 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> >       sll->sll_halen = dev_parse_header(skb, sll->sll_addr);
> >       sll->sll_family = AF_PACKET;
> >       sll->sll_hatype = dev->type;
> > -     sll->sll_protocol = skb->protocol;
> > +     sll->sll_protocol = eth_type_vlan(skb->protocol) ?
> > +             vlan_eth_hdr(skb)->h_vlan_encapsulated_proto : skb->protocol;
>
> This is a particularly subtle change of behavior.
>
> >               if (skb_vlan_tag_present(skb)) {
> >                       aux.tp_vlan_tci = skb_vlan_tag_get(skb);
> >                       aux.tp_vlan_tpid = ntohs(skb->vlan_proto);
> > -                     aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> > +             } else if (eth_type_vlan(skb->protocol)) {
> > +                     aux.tp_vlan_tci = ntohs(vlan_eth_hdr(skb)->h_vlan_TCI);
> > +                     aux.tp_vlan_tpid = ntohs(skb->protocol);
> >               } else {
> >                       aux.tp_vlan_tci = 0;
> >                       aux.tp_vlan_tpid = 0;
> >               }
> > +             if (aux.tp_vlan_tci || aux.tp_vlan_tpid)
> > +                     aux.tp_status |= TP_STATUS_VLAN_VALID | TP_STATUS_VLAN_TPID_VALID;
> >               put_cmsg(msg, SOL_PACKET, PACKET_AUXDATA, sizeof(aux), &aux);
>
> vlan_tci 0 is valid identifier. That's the reason explicit field
> TP_STATUS_VLAN_VALID was added.
>