diff mbox

[net] geneve: avoid use-after-free of skb->data

Message ID 027c88dd060f5ca4535cb346db125829b2181a88.1480675406.git.sd@queasysnail.net
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Sabrina Dubroca Dec. 2, 2016, 3:49 p.m. UTC 
geneve{,6}_build_skb can end up doing a pskb_expand_head(), which
makes the ip_hdr(skb) reference we stashed earlier stale. Since it's
only needed as an argument to ip_tunnel_ecn_encap(), move this
directly in the function call.

Fixes: 08399efc6319 ("geneve: ensure ECN info is handled properly in all tx/rx paths")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/geneve.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

John W. Linville Dec. 2, 2016, 6:33 p.m. UTC | #1 
On Fri, Dec 02, 2016 at 04:49:29PM +0100, Sabrina Dubroca wrote:
> geneve{,6}_build_skb can end up doing a pskb_expand_head(), which
> makes the ip_hdr(skb) reference we stashed earlier stale. Since it's
> only needed as an argument to ip_tunnel_ecn_encap(), move this
> directly in the function call.
> 
> Fixes: 08399efc6319 ("geneve: ensure ECN info is handled properly in all tx/rx paths")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: John W. Linville <linville@tuxdriver.com>
David Miller Dec. 2, 2016, 7:09 p.m. UTC | #2 
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Fri,  2 Dec 2016 16:49:29 +0100

> geneve{,6}_build_skb can end up doing a pskb_expand_head(), which
> makes the ip_hdr(skb) reference we stashed earlier stale. Since it's
> only needed as an argument to ip_tunnel_ecn_encap(), move this
> directly in the function call.
> 
> Fixes: 08399efc6319 ("geneve: ensure ECN info is handled properly in all tx/rx paths")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Applied and queued up for -stable, thanks.

This bug happens so many times that I think it might be time for
a debugging mode for pskb_expand_head() that unconditionally
reallocates the skb->data buffer regardless of whether it's
necessary or not and somehow unmaps the previous buffer to
force a trap on stale pointers.

Better ideas welcome, of course :)
Sabrina Dubroca Dec. 3, 2016, 12:33 a.m. UTC | #3 
2016-12-02, 14:09:25 -0500, David Miller wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Fri,  2 Dec 2016 16:49:29 +0100
> 
> > geneve{,6}_build_skb can end up doing a pskb_expand_head(), which
> > makes the ip_hdr(skb) reference we stashed earlier stale. Since it's
> > only needed as an argument to ip_tunnel_ecn_encap(), move this
> > directly in the function call.
> > 
> > Fixes: 08399efc6319 ("geneve: ensure ECN info is handled properly in all tx/rx paths")
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> Applied and queued up for -stable, thanks.
> 
> This bug happens so many times that I think it might be time for
> a debugging mode for pskb_expand_head() that unconditionally
> reallocates the skb->data buffer regardless of whether it's
> necessary or not and somehow unmaps the previous buffer to
> force a trap on stale pointers.

The problem with that is you'd need to enable the "debugging mode" in
all wrappers, so that they don't bypass the actual call to
pskb_expand_head(). And that still leaves all the direct calls to
pskb_expand_head() that are guarded by some kind of check (just two
random hits without even looking very hard:
net/core/pktgen.c:process_ipsec, net/ipv4/ip_gre.c:gre_fb_xmit).

Then I think we could just rely on KASAN (that's how I noticed this
bug).


> Better ideas welcome, of course :)

May not be better ;)  but at least another idea:

I'd like to try something based on static analysis. We'd need a way to
tag cached pointers to skb->data (via ip_hdr() or whatever), and
propagate the notion that pskb_expand_head() makes these cached
pointers stale through layers of function calls.  I don't know how
feasible this is with the tools we have.
David Miller Dec. 4, 2016, 4:11 a.m. UTC | #4 
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Sat, 3 Dec 2016 01:33:26 +0100

> I'd like to try something based on static analysis. We'd need a way to
> tag cached pointers to skb->data (via ip_hdr() or whatever), and
> propagate the notion that pskb_expand_head() makes these cached
> pointers stale through layers of function calls.  I don't know how
> feasible this is with the tools we have.

Perhaps create helpers that have some special attribute attached to
them like "skb_volatile" or whatever.  ip_hdr() et al would go through
them.

Then the static analysis tool is told that pskb_expand_head() "kills"
all skb_volatile obtained values, and it could basically mark all such
variables as uninitialized.
diff mbox

Patch

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 532695c8209b..bab65647b80f 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -859,7 +859,6 @@  static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	struct geneve_dev *geneve = netdev_priv(dev);
 	struct geneve_sock *gs4;
 	struct rtable *rt = NULL;
-	const struct iphdr *iip; /* interior IP header */
 	int err = -EINVAL;
 	struct flowi4 fl4;
 	__u8 tos, ttl;
@@ -890,8 +889,6 @@  static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	skb_reset_mac_header(skb);
 
-	iip = ip_hdr(skb);
-
 	if (info) {
 		const struct ip_tunnel_key *key = &info->key;
 		u8 *opts = NULL;
@@ -911,7 +908,7 @@  static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		if (unlikely(err))
 			goto tx_error;
 
-		tos = ip_tunnel_ecn_encap(key->tos, iip, skb);
+		tos = ip_tunnel_ecn_encap(key->tos, ip_hdr(skb), skb);
 		ttl = key->ttl;
 		df = key->tun_flags & TUNNEL_DONT_FRAGMENT ? htons(IP_DF) : 0;
 	} else {
@@ -920,7 +917,7 @@  static netdev_tx_t geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		if (unlikely(err))
 			goto tx_error;
 
-		tos = ip_tunnel_ecn_encap(fl4.flowi4_tos, iip, skb);
+		tos = ip_tunnel_ecn_encap(fl4.flowi4_tos, ip_hdr(skb), skb);
 		ttl = geneve->ttl;
 		if (!ttl && IN_MULTICAST(ntohl(fl4.daddr)))
 			ttl = 1;
@@ -952,7 +949,6 @@  static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 {
 	struct geneve_dev *geneve = netdev_priv(dev);
 	struct dst_entry *dst = NULL;
-	const struct iphdr *iip; /* interior IP header */
 	struct geneve_sock *gs6;
 	int err = -EINVAL;
 	struct flowi6 fl6;
@@ -982,8 +978,6 @@  static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	skb_reset_mac_header(skb);
 
-	iip = ip_hdr(skb);
-
 	if (info) {
 		const struct ip_tunnel_key *key = &info->key;
 		u8 *opts = NULL;
@@ -1004,7 +998,7 @@  static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 		if (unlikely(err))
 			goto tx_error;
 
-		prio = ip_tunnel_ecn_encap(key->tos, iip, skb);
+		prio = ip_tunnel_ecn_encap(key->tos, ip_hdr(skb), skb);
 		ttl = key->ttl;
 		label = info->key.label;
 	} else {
@@ -1014,7 +1008,7 @@  static netdev_tx_t geneve6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 			goto tx_error;
 
 		prio = ip_tunnel_ecn_encap(ip6_tclass(fl6.flowlabel),
-					   iip, skb);
+					   ip_hdr(skb), skb);
 		ttl = geneve->ttl;
 		if (!ttl && ipv6_addr_is_multicast(&fl6.daddr))
 			ttl = 1;