| 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
| ||
|
Date: Wed, 20 Nov 2013 00:08:23 +0100
From: Daniel Borkmann <dborkman@...hat.com>
To: davem@...emloft.net
Cc: netdev@...r.kernel.org,
Salam Noureddine <noureddine@...stanetworks.com>,
Ben Greear <greearb@...delatech.com>
Subject: [PATCH net] packet: fix use after free race in send path when dev is released
Salam reported a use after free bug in PF_PACKET that occurs when
we're sending out frames on a socket bound device and suddenly the
netdevice is being unregistered. It appears that commit 827d9780
introduced a possible race condition between {t,}packet_snd and
packet_notifier. In the case of a bound socket, packet_notifier can
drop the last reference to the net_device and {t,}packet_snd might
end up sending a packet over a freed net_device.
To avoid reverting 827d9780 entirely, we could make use of po->running
member that gets reset when we're calling __unregister_prot_hook() in
packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
notification. Plus, we still need to hold ref to the netdev, so
that we can assure it won't be released while we're in send path.
We try to avoid holding the bind lock all over send code as this
would likely make it worse when multiple threads want to send on
the same socket (in mmap code we're holding the pg_vec_lock mutex
all over the send code; we should try to get rid of that at some
point in time!). This at least buys us the possibility that we don't
have to walk over the entire dev list in dev_get_by_index() that we
would need to do as before 827d9780, which can just hurt as well
with many devices registered.
Fixes: 827d978037d7 ("af-packet: Use existing netdev reference for bound sockets.")
Signed-off-by: Daniel Borkmann <dborkman@...hat.com>
Signed-off-by: Salam Noureddine <noureddine@...stanetworks.com>
Tested-by: Salam Noureddine <noureddine@...stanetworks.com>
Cc: Ben Greear <greearb@...delatech.com>
---
net/packet/af_packet.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2e8286b..385b2cc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1045,6 +1045,19 @@ static void *__prb_previous_block(struct packet_sock *po,
return prb_lookup_block(po, rb, previous, status);
}
+static struct net_device *dev_get_packet_sock(struct packet_sock *po)
+{
+ struct net_device *dev;
+
+ spin_lock(&po->bind_lock);
+ dev = po->prot_hook.dev;
+ if (dev)
+ dev_hold(dev);
+ spin_unlock(&po->bind_lock);
+
+ return dev;
+}
+
static void *packet_previous_rx_frame(struct packet_sock *po,
struct packet_ring_buffer *rb,
int status)
@@ -2057,7 +2070,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
struct sk_buff *skb;
struct net_device *dev;
__be16 proto;
- bool need_rls_dev = false;
int err, reserve = 0;
void *ph;
struct sockaddr_ll *saddr = (struct sockaddr_ll *)msg->msg_name;
@@ -2070,7 +2082,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
mutex_lock(&po->pg_vec_lock);
if (saddr == NULL) {
- dev = po->prot_hook.dev;
+ dev = dev_get_packet_sock(po);
proto = po->num;
addr = NULL;
} else {
@@ -2084,7 +2096,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
proto = saddr->sll_protocol;
addr = saddr->sll_addr;
dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
- need_rls_dev = true;
}
err = -ENXIO;
@@ -2094,7 +2105,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
reserve = dev->hard_header_len;
err = -ENETDOWN;
- if (unlikely(!(dev->flags & IFF_UP)))
+ if (unlikely(!(dev->flags & IFF_UP) || !po->running))
goto out_put;
size_max = po->tx_ring.frame_size
@@ -2173,8 +2184,7 @@ out_status:
__packet_set_status(po, ph, status);
kfree_skb(skb);
out_put:
- if (need_rls_dev)
- dev_put(dev);
+ dev_put(dev);
out:
mutex_unlock(&po->pg_vec_lock);
return err;
@@ -2212,7 +2222,6 @@ static int packet_snd(struct socket *sock,
struct sk_buff *skb;
struct net_device *dev;
__be16 proto;
- bool need_rls_dev = false;
unsigned char *addr;
int err, reserve = 0;
struct virtio_net_hdr vnet_hdr = { 0 };
@@ -2228,7 +2237,7 @@ static int packet_snd(struct socket *sock,
*/
if (saddr == NULL) {
- dev = po->prot_hook.dev;
+ dev = dev_get_packet_sock(po);
proto = po->num;
addr = NULL;
} else {
@@ -2240,7 +2249,6 @@ static int packet_snd(struct socket *sock,
proto = saddr->sll_protocol;
addr = saddr->sll_addr;
dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
- need_rls_dev = true;
}
err = -ENXIO;
@@ -2250,7 +2258,7 @@ static int packet_snd(struct socket *sock,
reserve = dev->hard_header_len;
err = -ENETDOWN;
- if (!(dev->flags & IFF_UP))
+ if (unlikely(!(dev->flags & IFF_UP) || !po->running))
goto out_unlock;
if (po->has_vnet_hdr) {
@@ -2386,15 +2394,14 @@ static int packet_snd(struct socket *sock,
if (err > 0 && (err = net_xmit_errno(err)) != 0)
goto out_unlock;
- if (need_rls_dev)
- dev_put(dev);
+ dev_put(dev);
return len;
out_free:
kfree_skb(skb);
out_unlock:
- if (dev && need_rls_dev)
+ if (dev)
dev_put(dev);
out:
return err;
--
1.8.3.1
--
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