netdev - Re: AF_PACKET mmap() v4...

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: <563BDEA5.9080907@iogearbox.net>
Date:	Thu, 05 Nov 2015 23:56:37 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	Arnd Bergmann <arnd@...db.de>, David Miller <davem@...emloft.net>,
	netdev@...r.kernel.org, tklauser@...tanz.ch
Subject: Re: AF_PACKET mmap() v4...

On 11/05/2015 05:17 PM, Eric Dumazet wrote:
> On Thu, 2015-11-05 at 13:56 +0100, Daniel Borkmann wrote:
>> On 11/05/2015 12:38 PM, Eric Dumazet wrote:
>
>>> If I am not mistaken, af_packet also lacks the ability to properly set
>>> skb->protocol
>>>
>>> I noticed this using trafgen on a bonding device, when I did my SYNFLOOD
>>> tests for TCP listener rewrite.
>>>
>>> The bonding hash function might uses flow dissector, but as this flow
>>> dissection depends on skb->protocol, all the traffic is directed on a
>>> single slave.
>>
>> Right, if I see this correctly, when you trigger the flushing of TX_RING
>> via sendmsg(), one can hand over a sockaddr_ll, where we infer sll_protocol
>> and tag every skb's skb->protocol with that in tpacket_fill_skb() for the
>> current flushing run. Otherwise, we use the po->num specified at socket
>> creation / bind time for everything (trafgen case).
>>
>> If needed on a per skb basis, perhaps we could map some tpacket_hdr{,2}
>> member that is not used from TX_RING side (perhaps union on tp_snaplen)?
>
> If po->num is 0 (as in trafgen case), we could also get the proto from
> Ethernet header provided by the user.
>
> The skb_probe_transport_header() call from tpacket_fill_skb() is useless
> in the current kernel.
>
> Let say an application wants to mix IPv6 and IPv4 packets, using a
> single TX ring....

Sorry for the late answer.

For the skb->protocol issue, perhaps something like this. Also noticed that
we should rather do the vlan check when we have the actual linear data from
the ring slot, the current way seems buggy if I see this correctly. Both
patches squashed below.

Thanks,
Daniel

  net/packet/af_packet.c | 27 ++++++++++++++-------------
  1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 691660b..8415ebd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2399,8 +2399,22 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
  	} else {
  		data = ph.raw + po->tp_hdrlen - sizeof(struct sockaddr_ll);
  	}
+
  	to_write = tp_len;

+	/* If skb->protocol is still 0, try to infer it. */
+	if (!skb->protocol && tp_len >= sizeof(struct ethhdr))
+		skb->protocol = ((struct ethhdr *)data)->h_proto;
+	if (tp_len > dev->mtu + dev->hard_header_len) {
+		/* Earlier code assumed this would be a VLAN pkt,
+		 * double-check this now that we have the actual
+		 * (linear) packet data at hand.
+		 */
+		if (unlikely(((struct ethhdr *)data)->h_proto !=
+			     htons(ETH_P_8021Q)))
+			return -EMSGSIZE;
+	}
+
  	if (sock->type == SOCK_DGRAM) {
  		err = dev_hard_header(skb, dev, ntohs(proto), addr,
  				NULL, tp_len);
@@ -2524,19 +2538,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
  		}
  		tp_len = tpacket_fill_skb(po, skb, ph, dev, size_max, proto,
  					  addr, hlen);
-		if (likely(tp_len >= 0) &&
-		    tp_len > dev->mtu + dev->hard_header_len) {
-			struct ethhdr *ehdr;
-			/* Earlier code assumed this would be a VLAN pkt,
-			 * double-check this now that we have the actual
-			 * packet in hand.
-			 */
-
-			skb_reset_mac_header(skb);
-			ehdr = eth_hdr(skb);
-			if (ehdr->h_proto != htons(ETH_P_8021Q))
-				tp_len = -EMSGSIZE;
-		}
  		if (unlikely(tp_len < 0)) {
  			if (po->tp_loss) {
  				__packet_set_status(po, ph,
-- 
1.9.3


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html