ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()

PATCH Applied Dan Carpenter 4 participants 9 messages Aug 19, 2013 – Aug 25, 2013

Subsystems (4)

Networking Network drivers Kernel janitors IPv4/IPv6

lore

Share & tools

lore

Dan Carpenter

author

2013-08-19 07:05 UTC #0 lore raw

PATCH 1/1 ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()

We need to move the derefernce after the IS_ERR() check.

Signed-off-by Dan Carpenter <[email protected]>

net/ipv4/ip_tunnel.c +4 -4

index a4d9126..24549b4 100644
@@ -854,14 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 
 	rtnl_lock();
 	itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
-	/* FB netdevice is special: we have one, and only one per netns.
-	 * Allowing to move it to another netns is clearly unsafe.
-	 */
-	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 	rtnl_unlock();
 
 	if (IS_ERR(itn->fb_tunnel_dev))
 		return PTR_ERR(itn->fb_tunnel_dev);
+	/* FB netdevice is special: we have one, and only one per netns.
+	 * Allowing to move it to another netns is clearly unsafe.
+	 */
+	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 
 	return 0;
 }

Nicolas Dichtel

Acked-by on v1 1/1

2013-08-19 07:33 UTC #1 lore raw

Quoted 3 lines · Dan Carpenter

We need to move the derefernce after the IS_ERR() check.
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Nicolas Dichtel <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2013-08-19 12:58 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 18 lines · Dan Carpenter

We need to move the derefernce after the IS_ERR() check.

Signed-off-by Dan Carpenter <[email protected]>

net/ipv4/ip_tunnel.c +0 -4

index a4d9126..24549b4 100644
@@ -854,14 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 
 	rtnl_lock();
 	itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
-	/* FB netdevice is special: we have one, and only one per netns.
-	 * Allowing to move it to another netns is clearly unsafe.
-	 */
-	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 	rtnl_unlock();

net/ipv4/ip_tunnel.c int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,

Quoted 10 lines

net/ipv4/ip_tunnel.c +4 -1

 	if (IS_ERR(itn->fb_tunnel_dev))
 		return PTR_ERR(itn->fb_tunnel_dev);
+	/* FB netdevice is special: we have one, and only one per netns.
+	 * Allowing to move it to another netns is clearly unsafe.
+	 */
+	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 
 	return 0;
 }
--

Please add to the changelog :

Bug was added in commit 6c742e714d8c2 ("ipip: add x-netns support")

I do not think this fix is safe.

"dev->features |= some_flag;" should be protected by rtnl

So I suggest using instead :

net/ipv4/ip_tunnel.c +3 -5

index a4d9126..830de3f 100644
@@ -857,13 +857,11 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 	/* FB netdevice is special: we have one, and only one per netns.
 	 * Allowing to move it to another netns is clearly unsafe.
 	 */
-	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+	if (!IS_ERR(itn->fb_tunnel_dev))
+		itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 	rtnl_unlock();
 
-	if (IS_ERR(itn->fb_tunnel_dev))
-		return PTR_ERR(itn->fb_tunnel_dev);
-
-	return 0;
+	return PTR_RET(itn->fb_tunnel_dev);
 }
 EXPORT_SYMBOL_GPL(ip_tunnel_init_net);

Dan Carpenter

author

2013-08-19 13:49 UTC #3 lore raw

Thanks for the review. I will send a v2 patch shortly.

regards,
dan carpenter

Dan Carpenter

author

2013-08-19 22:23 UTC #4 lore raw

PATCH v2 1/1 ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()

In reply to Eric Dumazet

The __ip_tunnel_create() function returns an ERR_PTR on error so we need to check for that before dereferencing. This bug was added in commit 6c742e714d8c2 ("ipip: add x-netns support").

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Eric Dumazet said v1 patch looked racy and suggested the v2 fix.

net/ipv4/ip_tunnel.c +3 -5

index a4d9126..830de3f 100644
@@ -857,13 +857,11 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 	/* FB netdevice is special: we have one, and only one per netns.
 	 * Allowing to move it to another netns is clearly unsafe.
 	 */
-	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+	if (!IS_ERR(itn->fb_tunnel_dev))
+		itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
 	rtnl_unlock();
 
-	if (IS_ERR(itn->fb_tunnel_dev))
-		return PTR_ERR(itn->fb_tunnel_dev);
-
-	return 0;
+	return PTR_RET(itn->fb_tunnel_dev);
 }
 EXPORT_SYMBOL_GPL(ip_tunnel_init_net);

David Miller

maintainer

2013-08-21 06:33 UTC #5 lore raw

From: Dan Carpenter <[email protected]>
Date: Tue, 20 Aug 2013 01:23:07 +0300

Quoted 8 lines

The __ip_tunnel_create() function returns an ERR_PTR on error so we need to check for that before dereferencing. This bug was added in commit 6c742e714d8c2 ("ipip: add x-netns support").
Signed-off-by Dan Carpenter <[email protected]>
---
v2: Eric Dumazet said v1 patch looked racy and suggested the v2 fix.

This doesn't apply cleanly, you'll need to respin it.

Dan Carpenter

author

2013-08-23 08:15 UTC #6 lore raw

PATCH 1/1 ipip: potential race in ip_tunnel_init_net()

Eric Dumazet says that my previous fix for an ERR_PTR dereference (ea857f28ab 'ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()') could be racy and suggests the following fix instead.

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Dan Carpenter <[email protected]>

net/ipv4/ip_tunnel.c +4 -6

index 24549b4..830de3f 100644
@@ -854,16 +854,14 @@ int ip_tunnel_init_net(struct net *net, int ip_tnl_net_id,
 
 	rtnl_lock();
 	itn->fb_tunnel_dev = __ip_tunnel_create(net, ops, &parms);
-	rtnl_unlock();
-
-	if (IS_ERR(itn->fb_tunnel_dev))
-		return PTR_ERR(itn->fb_tunnel_dev);
 	/* FB netdevice is special: we have one, and only one per netns.
 	 * Allowing to move it to another netns is clearly unsafe.
 	 */
-	itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+	if (!IS_ERR(itn->fb_tunnel_dev))
+		itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
+	rtnl_unlock();
 
-	return 0;
+	return PTR_RET(itn->fb_tunnel_dev);
 }
 EXPORT_SYMBOL_GPL(ip_tunnel_init_net);

David Miller

maintainer Applied

2013-08-25 22:40 UTC #7 lore raw

Re: [patch -next] ipip: potential race in ip_tunnel_init_net()

From: Dan Carpenter <[email protected]>
Date: Fri, 23 Aug 2013 11:15:37 +0300

Quoted 6 lines

Eric Dumazet says that my previous fix for an ERR_PTR dereference (ea857f28ab 'ipip: dereferencing an ERR_PTR in ip_tunnel_init_net()') could be racy and suggests the following fix instead.
Reported-by Eric Dumazet <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

Applied, thanks Dan.

David Miller

maintainer Applied

2013-08-20 22:08 UTC #8 lore raw

In reply to Dan Carpenter

From: Dan Carpenter <[email protected]>
Date: Mon, 19 Aug 2013 10:05:10 +0300

Quoted 3 lines

We need to move the derefernce after the IS_ERR() check.
Signed-off-by Dan Carpenter <[email protected]>

Applied.