Patchwork NULL pointer dereference in veth_stats_one

login
register
mail settings
Submitter Eric Dumazet
Date Jan. 4, 2013, 4:17 p.m.
Message ID <1357316231.1678.1528.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/209488/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Jan. 4, 2013, 4:17 p.m.
From: Eric Dumazet <edumazet@google.com>

On Fri, 2013-01-04 at 07:45 -0800, Eric Dumazet wrote:
> On Fri, 2013-01-04 at 10:59 +0000, Tom Parkin wrote:
> > Hi list,
> > 
> > I recently tripped over a NULL pointer dereference in the veth driver.
> > I'm running a 3.8.0_rc1 (updated from net-next git tree this morning)
> > on an Athlon 64 X2 machine running a 32 bit kernel.  To trigger the
> > oops I simply created a veth interface as follows:
> > 
> >         ip link add name ve0 type veth peer name ve1
> > 
> > I did a little digging in the git history and I note that veth
> > statistics changed a little with commit 2681128f0ced8aa4.  I tried
> > reverting that commit in my tree, which made the oops go away again.
> > 
> > Thanks,
> > Tom
> 
> Thanks Tom, I'll fix this.
> 

Oh well, a last minute change again...

I was fooled by veth_get_ethtool_stats() doing the priv->peer->ifindex
deref without checking.

Here is the fix, thanks !

[PATCH net-next] veth: avoid a NULL deref in veth_stats_one

commit 2681128f0ced8a (veth: extend device features) added a NULL deref
in veth_stats_one(), as veth_get_stats64() was not testing if the peer
device was setup or not.

At init time, we call dev_get_stats() before veth pair is fully setup.

[  178.854758]  [<ffffffffa00f5677>] veth_get_stats64+0x47/0x70 [veth]
[  178.861013]  [<ffffffff814f0a2d>] dev_get_stats+0x6d/0x130
[  178.866486]  [<ffffffff81504efc>] rtnl_fill_ifinfo+0x47c/0x930
[  178.872299]  [<ffffffff81505b93>] rtmsg_ifinfo+0x83/0x100
[  178.877678]  [<ffffffff81505cc6>] rtnl_configure_link+0x76/0xa0
[  178.883580]  [<ffffffffa00f52fa>] veth_newlink+0x16a/0x350 [veth]
[  178.889654]  [<ffffffff815061cc>] rtnl_newlink+0x4dc/0x5e0
[  178.895128]  [<ffffffff81505e1e>] ? rtnl_newlink+0x12e/0x5e0
[  178.900769]  [<ffffffff8150587d>] rtnetlink_rcv_msg+0x11d/0x310
[  178.906669]  [<ffffffff81505760>] ? __rtnl_unlock+0x20/0x20
[  178.912225]  [<ffffffff81521f89>] netlink_rcv_skb+0xa9/0xd0
[  178.917779]  [<ffffffff81502d55>] rtnetlink_rcv+0x25/0x40
[  178.923159]  [<ffffffff815218d1>] netlink_unicast+0x1b1/0x230
[  178.928887]  [<ffffffff81521c4e>] netlink_sendmsg+0x2fe/0x3b0
[  178.934615]  [<ffffffff814dbe22>] sock_sendmsg+0xd2/0xf0

So we must check if peer was setup in veth_get_stats64()

Reported-by: Tom Parkin <tparkin@katalix.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/veth.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 4, 2013, 6:17 p.m.
On Fri, 2013-01-04 at 08:17 -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> On Fri, 2013-01-04 at 07:45 -0800, Eric Dumazet wrote:
> > On Fri, 2013-01-04 at 10:59 +0000, Tom Parkin wrote:
> > > Hi list,
> > > 
> > > I recently tripped over a NULL pointer dereference in the veth driver.
> > > I'm running a 3.8.0_rc1 (updated from net-next git tree this morning)
> > > on an Athlon 64 X2 machine running a 32 bit kernel.  To trigger the
> > > oops I simply created a veth interface as follows:
> > > 
> > >         ip link add name ve0 type veth peer name ve1
> > > 
> > > I did a little digging in the git history and I note that veth
> > > statistics changed a little with commit 2681128f0ced8aa4.  I tried
> > > reverting that commit in my tree, which made the oops go away again.
> > > 
> > > Thanks,
> > > Tom
> > 
> > Thanks Tom, I'll fix this.
> > 
> 
> Oh well, a last minute change again...
> 
> I was fooled by veth_get_ethtool_stats() doing the priv->peer->ifindex
> deref without checking.
[...]
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -162,15 +162,18 @@ static struct rtnl_link_stats64 *veth_get_stats64(struct net_device *dev,
>  						  struct rtnl_link_stats64 *tot)
>  {
>  	struct veth_priv *priv = netdev_priv(dev);
> +	struct net_device *peer = priv->peer;
>  	struct pcpu_vstats one;
>  
>  	tot->tx_dropped = veth_stats_one(&one, dev);
>  	tot->tx_bytes = one.bytes;
>  	tot->tx_packets = one.packets;
>  
> -	tot->rx_dropped = veth_stats_one(&one, priv->peer);
> -	tot->rx_bytes = one.bytes;
> -	tot->rx_packets = one.packets;
> +	if (peer) {

This possibly needs some memory barriers to properly synchronise with
veth_newlink().  But can you not move initialisation of the peer
pointers before registration of the devices in veth_newlink(), so that
veth_get_stats64() cannot be called before they are initialised?

Ben.

> +		tot->rx_dropped = veth_stats_one(&one, peer);
> +		tot->rx_bytes = one.bytes;
> +		tot->rx_packets = one.packets;
> +	}
>  
>  	return tot;
>  }
Eric Dumazet - Jan. 4, 2013, 7:23 p.m.
On Fri, 2013-01-04 at 18:17 +0000, Ben Hutchings wrote:

> This possibly needs some memory barriers to properly synchronise with
> veth_newlink().  But can you not move initialisation of the peer
> pointers before registration of the devices in veth_newlink(), so that
> veth_get_stats64() cannot be called before they are initialised?

The ->peer pointer cannot change once set. ( its never cleared )

So the problem would not be in veth_newlink(), but might be in
veth_dellink()

It seems we would have a problem in veth_get_ethtool_stats() already...

More generally, what prevents a get_stats() being called while a
dellink() (-> veth_dev_free() -> free_percpu()) is done ?

(Same thing is done for tunnel/dummy stats percpu data)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 4, 2013, 8:25 p.m.
On Fri, 2013-01-04 at 11:23 -0800, Eric Dumazet wrote:
> On Fri, 2013-01-04 at 18:17 +0000, Ben Hutchings wrote:
> 
> > This possibly needs some memory barriers to properly synchronise with
> > veth_newlink().  But can you not move initialisation of the peer
> > pointers before registration of the devices in veth_newlink(), so that
> > veth_get_stats64() cannot be called before they are initialised?
> 
> The ->peer pointer cannot change once set. ( its never cleared )

We may still need an explicit barrier for data-dependency.

> So the problem would not be in veth_newlink(), but might be in
> veth_dellink()

A lot of things are done in between the unregister_netdevice_queue() and
the actual deletion which are probably sufficient to flush out any calls
to dev_get_stats().  But to make sure, I think we would need some small
amount of shared state that isn't freed until both devices are.

> It seems we would have a problem in veth_get_ethtool_stats() already...

That should be OK because both ethtool operations and the whole process
of interface deletion are serialised by the RTNL lock.

> More generally, what prevents a get_stats() being called while a
> dellink() (-> veth_dev_free() -> free_percpu()) is done ?

Anything calling dev_get_stats() must have a counted or RCU reference to
the device, and netdev_run_todo() waits for those to go away.  For
mutually referencing devices we want a kind of weak reference and we
have no good way to implement those.

Ben.

> (Same thing is done for tunnel/dummy stats percpu data)

Patch

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8b2e112..bd57213 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -162,15 +162,18 @@  static struct rtnl_link_stats64 *veth_get_stats64(struct net_device *dev,
 						  struct rtnl_link_stats64 *tot)
 {
 	struct veth_priv *priv = netdev_priv(dev);
+	struct net_device *peer = priv->peer;
 	struct pcpu_vstats one;
 
 	tot->tx_dropped = veth_stats_one(&one, dev);
 	tot->tx_bytes = one.bytes;
 	tot->tx_packets = one.packets;
 
-	tot->rx_dropped = veth_stats_one(&one, priv->peer);
-	tot->rx_bytes = one.bytes;
-	tot->rx_packets = one.packets;
+	if (peer) {
+		tot->rx_dropped = veth_stats_one(&one, peer);
+		tot->rx_bytes = one.bytes;
+		tot->rx_packets = one.packets;
+	}
 
 	return tot;
 }