[PATCH net-next 0/2] fix bonding neighbour setup handling

[PATCH net-next 0/2] fix bonding neighbour setup handling

[Veaceslav Falico]

Recent patches revealed an old bug, which was there for quite awhile. It's
related to vlan on top of bonding and ndo_neigh_setup(). When vlan device
is initiated, it calls its real_dev->ndo_neigh_setup(), and in case of
bonding - it will modify neigh_parms->neigh_setup to point to
bond_neigh_init, while neigh_parms are of vlan's dev.

This way, when neigh_parms->neigh_setup() of vlan's dev is called, the
bonding function will be called, which expects the dev to be struct
bonding, but will receive a vlan dev.

It was hidden before because of bond->first_slave usage. Now, with
Nikolay's conversion to list/RCU, first_slave is gone and we hit a null
pointer dereference when working with lists/slave.

First patch moves ndo_neigh_setup() in neigh_parms_alloc() to the bottom,
so that the ->dev will be available to the caller. It doesn't really change
anything, however is needed for the second patch.

Second patch makes bond_neigh_setup() (bond->ndo_neigh_setup()) check if
the neigh_parms are really from a bonding dev, and only modify the
neigh_setup in this case.

 drivers/net/bonding/bond_main.c |    8 +++++++-
 net/core/neighbour.c            |   10 ++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

[PATCH net-next 1/2] neighbour: populate neigh_parms on alloc before calling ndo_neigh_setup

[Veaceslav Falico]

dev->ndo_neigh_setup() might need some of the values of neigh_parms, so
populate them before calling it.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 net/core/neighbour.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index b7de821..576d46f 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1441,16 +1441,18 @@ struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
 		atomic_set(&p->refcnt, 1);
 		p->reachable_time =
 				neigh_rand_reach_time(p->base_reachable_time);
+		dev_hold(dev);
+		p->dev = dev;
+		write_pnet(&p->net, hold_net(net));
+		p->sysctl_table = NULL;
 
 		if (ops->ndo_neigh_setup && ops->ndo_neigh_setup(dev, p)) {
+			release_net(net);
+			dev_put(dev);
 			kfree(p);
 			return NULL;
 		}
 
-		dev_hold(dev);
-		p->dev = dev;
-		write_pnet(&p->net, hold_net(net));
-		p->sysctl_table = NULL;
 		write_lock_bh(&tbl->lock);
 		p->next		= tbl->parms.next;
 		tbl->parms.next = p;
-- 
1.7.1

[PATCH net-next 2/2] bonding: modify only neigh_parms owned by us

[Veaceslav Falico]

Otherwise, on neighbour creation, bond_neigh_init() will be called with a
foreign netdev.

Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 1d37a96..476df7d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3630,11 +3630,17 @@ static int bond_neigh_init(struct neighbour *n)
  * The bonding ndo_neigh_setup is called at init time beofre any
  * slave exists. So we must declare proxy setup function which will
  * be used at run time to resolve the actual slave neigh param setup.
+ *
+ * It's also called by master devices (such as vlans) to setup their
+ * underlying devices. In that case - do nothing, we're already set up from
+ * our init.
  */
 static int bond_neigh_setup(struct net_device *dev,
 			    struct neigh_parms *parms)
 {
-	parms->neigh_setup   = bond_neigh_init;
+	/* modify only our neigh_parms */
+	if (parms->dev == dev)
+		parms->neigh_setup = bond_neigh_init;
 
 	return 0;
 }
-- 
1.7.1

Re: [PATCH net-next 0/2] fix bonding neighbour setup handling

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Fri,  2 Aug 2013 19:07:37 +0200

> Recent patches revealed an old bug, which was there for quite awhile. It's
> related to vlan on top of bonding and ndo_neigh_setup(). When vlan device
> is initiated, it calls its real_dev->ndo_neigh_setup(), and in case of
> bonding - it will modify neigh_parms->neigh_setup to point to
> bond_neigh_init, while neigh_parms are of vlan's dev.
> 
> This way, when neigh_parms->neigh_setup() of vlan's dev is called, the
> bonding function will be called, which expects the dev to be struct
> bonding, but will receive a vlan dev.
> 
> It was hidden before because of bond->first_slave usage. Now, with
> Nikolay's conversion to list/RCU, first_slave is gone and we hit a null
> pointer dereference when working with lists/slave.
> 
> First patch moves ndo_neigh_setup() in neigh_parms_alloc() to the bottom,
> so that the ->dev will be available to the caller. It doesn't really change
> anything, however is needed for the second patch.
> 
> Second patch makes bond_neigh_setup() (bond->ndo_neigh_setup()) check if
> the neigh_parms are really from a bonding dev, and only modify the
> neigh_setup in this case.

Thanks for the detailed explanation of the situation, it made your patches
trivial to review.

Applied, thanks.

Re: [PATCH net-next 0/2] fix bonding neighbour setup handling

[Nikolay Aleksandrov]

On 08/03/2013 12:45 AM, David Miller wrote:
> 
> Thanks for the detailed explanation of the situation, it made your patches
> trivial to review.
> 
> Applied, thanks.
Hi all,
Vaeceslav thanks for fixing this.
Since the cat is out of the bag about this bug, as Vaeceslav discovered it
independently and wasn't aware that there's a CVE number pending because it
poses a security threat since the dereferenced first_slave pointer is
taken from the struct vlan_dev_priv's ingress_priority map array which is
user-controllable and any memory address can be dereferenced in that way,
and taking after that first_slave->dev->netdev_ops and calling a function
from the ops is making it even easier. Of course for that to happen the
user must have CAP_NET_ADMIN.
I've tested these patches and they apply cleanly on -net as well, so please
queue them for -net and stable.

Also apologies for the late reply but it wasn't up to me when to reveal this.

Thank you,
 Nik

Re: [PATCH net-next 0/2] fix bonding neighbour setup handling

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Mon, 05 Aug 2013 15:49:08 +0200

> Since the cat is out of the bag about this bug, as Vaeceslav discovered it
> independently and wasn't aware that there's a CVE number pending because it
> poses a security threat since the dereferenced first_slave pointer is
> taken from the struct vlan_dev_priv's ingress_priority map array which is
> user-controllable and any memory address can be dereferenced in that way,
> and taking after that first_slave->dev->netdev_ops and calling a function
> from the ops is making it even easier. Of course for that to happen the
> user must have CAP_NET_ADMIN.
> I've tested these patches and they apply cleanly on -net as well, so please
> queue them for -net and stable.

This is why I absolutely detest closed work on bugs, and prefer
everything be discussed and implemented openly here on this list,
without exceptions, and regardless of perceived "severity" of the bug.

Applied to net and queued up for -stable, thanks.