| 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
| ||
|
Message-ID: <CA+FuTSfkD4aMciGeqon0cLp=EJ7fWis56nGQsbku82ZC+=nNWg@mail.gmail.com>
Date: Tue, 10 Nov 2015 17:52:13 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: David Miller <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, tklauser@...tanz.ch,
Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 3/3] packet: fix tpacket_snd max frame and vlan handling
> if (sock->type == SOCK_DGRAM) {
> - err = dev_hard_header(skb, dev, ntohs(proto), addr,
> - NULL, tp_len);
> + /* In DGRAM sockets, we expect struct sockaddr_ll was filled
> + * via struct msghdr, so we have dest mac and skb->protocol.
> + * Otherwise there's not too much useful things we can do in
> + * this flush run.
> + */
> + err = dev_hard_header(skb, dev, ntohs(skb->protocol), addr,
> + NULL, tp_len);
This change is not really necessary.
> if (unlikely(err < 0))
> return -EINVAL;
> - } else if (dev->hard_header_len) {
Why remove the check on hard_header_len?
> - if (ll_header_truncated(dev, tp_len))
> - return -EINVAL;
> + } else {
> + /* If skb->protocol is still 0, try to infer/guess it. Might
> + * not be fully reliable in the sense that a user could still
> + * change/race data afterwards, but on the other hand the proto
The race goes away when probing it after the copy in skb_store_bits.
Then it is also certain that tp_len is long enough to hold the entire
link layer header.
> + * can be set arbitrarily anyways. We only need to take care
> + * in case of extra large VLAN frames.
> + */
> + if (!skb->protocol && tp_len >= ETH_HLEN)
> + skb->protocol = ((struct ethhdr *)data)->h_proto;
Packet sockets are not restricted to link layer of type Ethernet.
There are a few other points in this file that also cast mac header
to eth_hdr(skb).
>
> - skb_push(skb, dev->hard_header_len);
> - err = skb_store_bits(skb, 0, data,
> - dev->hard_header_len);
> - if (unlikely(err))
> - return err;
> + /* Copy Ethernet header in case we need to deal with extra
> + * VLAN space as otherwise data could change underneath us.
> + * The caller already accomodated for enough linear room.
> + */
> + if (dev->hard_header_len || tp_len > dev->mtu + reserve) {
> + int hdr_len = dev->hard_header_len;
> +
> + if (hdr_len < ETH_HLEN)
> + hdr_len = ETH_HLEN;
> + if (unlikely(ll_header_truncated(hdr_len, tp_len)))
> + return -EINVAL;
> +
> + skb_push(skb, hdr_len);
> + err = skb_store_bits(skb, 0, data, hdr_len);
> + if (unlikely(err))
> + return err;
>
> - data += dev->hard_header_len;
> - to_write -= dev->hard_header_len;
> + data += hdr_len;
> + to_write -= hdr_len;
> + }
> }
>
> offset = offset_in_page(data);
> @@ -2447,6 +2471,20 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
> len = ((to_write > len_max) ? len_max : to_write);
> }
>
> + /* Check for full frame with extra VLAN space. We can probe
> + * here on the linear header, if necessary. Earlier code
> + * assumed this would be a VLAN pkt, double-check this now
> + * that we have the actual packet and protocol at hand.
> + */
> + if (tp_len > dev->mtu + reserve) {
> + if (skb->protocol != htons(ETH_P_8021Q))
> + return -EMSGSIZE;
> +
> + skb_reset_mac_header(skb);
> + if (eth_hdr(skb)->h_proto != htons(ETH_P_8021Q))
> + return -EMSGSIZE;
> + }
> +
> skb_probe_transport_header(skb, 0);
>
> return tp_len;
> @@ -2493,12 +2531,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> if (unlikely(!(dev->flags & IFF_UP)))
> goto out_put;
>
> - reserve = dev->hard_header_len + VLAN_HLEN;
> - size_max = po->tx_ring.frame_size
> - - (po->tp_hdrlen - sizeof(struct sockaddr_ll));
> -
> - if (size_max > dev->mtu + reserve)
> - size_max = dev->mtu + reserve;
> + if (po->sk.sk_socket->type == SOCK_RAW)
> + reserve = dev->hard_header_len;
> + size_max = po->tx_ring.frame_size - (po->tp_hdrlen -
> + sizeof(struct sockaddr_ll));
> + if (size_max > dev->mtu + reserve + VLAN_HLEN)
> + size_max = dev->mtu + reserve + VLAN_HLEN;
>
> do {
> ph = packet_current_frame(po, &po->tx_ring,
> @@ -2523,20 +2561,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> goto out_status;
> }
> 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;
> - }
> + addr, hlen, reserve);
> if (unlikely(tp_len < 0)) {
> if (po->tp_loss) {
> __packet_set_status(po, ph,
> @@ -2754,7 +2779,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> if (unlikely(offset < 0))
> goto out_free;
> } else {
> - if (ll_header_truncated(dev, len))
> + if (unlikely(ll_header_truncated(dev->hard_header_len, len)))
> goto out_free;
> }
>
> --
> 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
Powered by blists - more mailing lists