3 Mar
2011
3 Mar
'11
7:09 p.m.
Hi everyone,
By accident I've typed in a mesh interface name for batman-adv which
already existed as a real interface. This produces a null pointer
dereference in orig_hash_add_if():
http://www.open-mesh.org/ticket/146
The attached patch shall illustrate the problem, but I'm not quite
satisfied with it. Although it seems to "fix" the problem and gets
rid of the call trace, it is probably still very racy. Does anyone
have an idea for a more sane but equally easy check to fix the issue?
Or is the only sane solution to hold an rcu-lock and compare the
hard_iface->soft_iface in hardif_enable_interface() with every
hard-iface->net_dev from the hardif_list?
Cheers, Linus
3 Mar
3 Mar
7:09 p.m.
New subject: [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if
When we are trying to create a batman soft-interface which already
exists as a common hard interface, batman than wrongly assumes that this
hard interface is a fully initialized soft interface. This leads to a
null pointer dereference on the first try of accessing for instance a
non-intialized orig_hash.
For every hard interface, there is no initialized orig_hash, therefore
this commit uses this criteria to abort creating a soft interface with
an already existing name.
---
hard-interface.c | 17 +++++++++++++++--
1 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index 95a35b6..53d6fce 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -282,6 +282,7 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
{
struct bat_priv *bat_priv;
struct batman_packet *batman_packet;
+ int ret;
if (hard_iface->if_status != IF_NOT_IN_USE)
goto out;
@@ -294,20 +295,32 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
if (!hard_iface->soft_iface) {
hard_iface->soft_iface = softif_create(iface_name);
- if (!hard_iface->soft_iface)
+ if (!hard_iface->soft_iface) {
+ ret = -ENOMEM;
goto err;
+ }
/* dev_get_by_name() increases the reference counter for us */
dev_hold(hard_iface->soft_iface);
}
bat_priv = netdev_priv(hard_iface->soft_iface);
+
+ if (!bat_priv->orig_hash) {
+ bat_err(hard_iface->soft_iface,
+ "Can't create soft interface %s: "
+ "already exists as non soft interface\n",
+ hard_iface->soft_iface->name);
+ ret = -EINVAL;
+ goto err;
+ }
hard_iface->packet_len = BAT_PACKET_LEN;
hard_iface->packet_buff = kmalloc(hard_iface->packet_len, GFP_ATOMIC);
if (!hard_iface->packet_buff) {
bat_err(hard_iface->soft_iface, "Can't add interface packet "
"(%s): out of memory\n", hard_iface->net_dev->name);
+ ret = -ENOMEM;
goto err;
}
@@ -370,7 +383,7 @@ out:
err:
hardif_free_ref(hard_iface);
- return -ENOMEM;
+ return ret;
}
void hardif_disable_interface(struct hard_iface *hard_iface)
--
1.7.2.3
8:22 p.m.
Linus Lüssing wrote:
Hi everyone,
By accident I've typed in a mesh interface name for batman-adv which
already existed as a real interface. This produces a null pointer
dereference in orig_hash_add_if():
http://www.open-mesh.org/ticket/146
The attached patch shall illustrate the problem, but I'm not quite
satisfied with it. Although it seems to "fix" the problem and gets
rid of the call trace, it is probably still very racy. Does anyone
have an idea for a more sane but equally easy check to fix the issue?
Or is the only sane solution to hold an rcu-lock and compare the
hard_iface->soft_iface in hardif_enable_interface() with every
hard-iface->net_dev from the hardif_list?
Cheers, Linus
Oh, it doesn't fix anything - it just works by accident. :)
You are just happy that bat_priv->orig_hash is still memory that is accessible
by us and is zero.
Let me suggest another patch (may take a while).
Best regards,
Sven
8:39 p.m.
New subject: [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if
When we are trying to create a batman soft-interface which already
exists as a common hard interface, batman wrongly assumes that this
hard interface is a fully initialized soft interface. This leads to a
null pointer dereference on the first try of accessing for instance a
non-intialized orig_hash.
Reported-by: Linus Lüssing <linus.luessing(a)ascom.ch>
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
batman-adv/hard-interface.c | 37 +++++++++++++++++++++++--------------
batman-adv/soft-interface.c | 13 +++++++++++++
batman-adv/soft-interface.h | 1 +
3 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index 95a35b6..947e647 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -71,21 +71,14 @@ static int is_valid_iface(struct net_device *net_dev)
{
if (net_dev->flags & IFF_LOOPBACK)
return 0;
-
if (net_dev->type != ARPHRD_ETHER)
return 0;
-
if (net_dev->addr_len != ETH_ALEN)
return 0;
/* no batman over batman */
-#ifdef HAVE_NET_DEVICE_OPS
- if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ if (softif_is_valid(net_dev))
return 0;
-#else
- if (net_dev->hard_start_xmit == interface_tx)
- return 0;
-#endif
/* Device is being bridged */
/* if (net_dev->priv_flags & IFF_BRIDGE_PORT)
@@ -282,6 +275,8 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
{
struct bat_priv *bat_priv;
struct batman_packet *batman_packet;
+ int ret;
+ struct net_device *soft_iface;
if (hard_iface->if_status != IF_NOT_IN_USE)
goto out;
@@ -289,18 +284,31 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
if (!atomic_inc_not_zero(&hard_iface->refcount))
goto out;
- hard_iface->soft_iface = dev_get_by_name(&init_net, iface_name);
+ soft_iface = dev_get_by_name(&init_net, iface_name);
- if (!hard_iface->soft_iface) {
- hard_iface->soft_iface = softif_create(iface_name);
+ if (!soft_iface) {
+ soft_iface = softif_create(iface_name);
- if (!hard_iface->soft_iface)
+ if (!soft_iface) {
+ ret = -ENOMEM;
goto err;
+ }
/* dev_get_by_name() increases the reference counter for us */
- dev_hold(hard_iface->soft_iface);
+ dev_hold(soft_iface);
}
+ if (!softif_is_valid(soft_iface)) {
+ pr_err("Can't create soft interface %s: "
+ "already exists as non soft interface\n",
+ soft_iface->name);
+ dev_put(soft_iface);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ hard_iface->soft_iface = soft_iface;
+
bat_priv = netdev_priv(hard_iface->soft_iface);
hard_iface->packet_len = BAT_PACKET_LEN;
hard_iface->packet_buff = kmalloc(hard_iface->packet_len, GFP_ATOMIC);
@@ -308,6 +316,7 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
if (!hard_iface->packet_buff) {
bat_err(hard_iface->soft_iface, "Can't add interface packet "
"(%s): out of memory\n", hard_iface->net_dev->name);
+ ret = -ENOMEM;
goto err;
}
@@ -370,7 +379,7 @@ out:
err:
hardif_free_ref(hard_iface);
- return -ENOMEM;
+ return ret;
}
void hardif_disable_interface(struct hard_iface *hard_iface)
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 6b514ec..9ed2614 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -622,6 +622,19 @@ void softif_destroy(struct net_device *soft_iface)
unregister_netdevice(soft_iface);
}
+int softif_is_valid(struct net_device *net_dev)
+{
+#ifdef HAVE_NET_DEVICE_OPS
+ if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ return 1;
+#else
+ if (net_dev->hard_start_xmit == interface_tx)
+ return 1;
+#endif
+
+ return 0;
+}
+
/* ethtool */
static int bat_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
{
diff --git a/batman-adv/soft-interface.h b/batman-adv/soft-interface.h
index 80a3607..4789b6f 100644
--- a/batman-adv/soft-interface.h
+++ b/batman-adv/soft-interface.h
@@ -31,5 +31,6 @@ void interface_rx(struct net_device *soft_iface,
int hdr_size);
struct net_device *softif_create(char *name);
void softif_destroy(struct net_device *soft_iface);
+int softif_is_valid(struct net_device *net_dev);
#endif /* _NET_BATMAN_ADV_SOFT_INTERFACE_H_ */
--
1.7.4.1
9:08 p.m.
New subject: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
When trying to associate a net_device with another net_device which
already exists, batman-adv assumes that this interface is a fully
initialized batman mesh interface without checking it. The behaviour
when accessing data behind netdev_priv of a random net_device is
undefined and potentially dangerous.
Reported-by: Linus Lüssing <linus.luessing(a)ascom.ch>
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
* Just smaller cleanups
* No functionality changes
batman-adv/hard-interface.c | 34 ++++++++++++++++++++++------------
batman-adv/soft-interface.c | 13 +++++++++++++
batman-adv/soft-interface.h | 1 +
3 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index 95a35b6..a5d88e5 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -79,13 +79,8 @@ static int is_valid_iface(struct net_device *net_dev)
return 0;
/* no batman over batman */
-#ifdef HAVE_NET_DEVICE_OPS
- if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ if (softif_is_valid(net_dev))
return 0;
-#else
- if (net_dev->hard_start_xmit == interface_tx)
- return 0;
-#endif
/* Device is being bridged */
/* if (net_dev->priv_flags & IFF_BRIDGE_PORT)
@@ -282,6 +277,8 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
{
struct bat_priv *bat_priv;
struct batman_packet *batman_packet;
+ struct net_device *soft_iface;
+ int ret;
if (hard_iface->if_status != IF_NOT_IN_USE)
goto out;
@@ -289,18 +286,30 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
if (!atomic_inc_not_zero(&hard_iface->refcount))
goto out;
- hard_iface->soft_iface = dev_get_by_name(&init_net, iface_name);
+ soft_iface = dev_get_by_name(&init_net, iface_name);
- if (!hard_iface->soft_iface) {
- hard_iface->soft_iface = softif_create(iface_name);
+ if (!soft_iface) {
+ soft_iface = softif_create(iface_name);
- if (!hard_iface->soft_iface)
+ if (!soft_iface) {
+ ret = -ENOMEM;
goto err;
+ }
/* dev_get_by_name() increases the reference counter for us */
- dev_hold(hard_iface->soft_iface);
+ dev_hold(soft_iface);
}
+ if (!softif_is_valid(soft_iface)) {
+ pr_err("Can't create batman mesh interface interface %s: "
+ "already exists as regular interface\n",
+ soft_iface->name);
+ dev_put(soft_iface);
+ ret = -EINVAL;
+ goto err;
+ }
+
+ hard_iface->soft_iface = soft_iface;
bat_priv = netdev_priv(hard_iface->soft_iface);
hard_iface->packet_len = BAT_PACKET_LEN;
hard_iface->packet_buff = kmalloc(hard_iface->packet_len, GFP_ATOMIC);
@@ -308,6 +317,7 @@ int hardif_enable_interface(struct hard_iface *hard_iface, char
*iface_name)
if (!hard_iface->packet_buff) {
bat_err(hard_iface->soft_iface, "Can't add interface packet "
"(%s): out of memory\n", hard_iface->net_dev->name);
+ ret = -ENOMEM;
goto err;
}
@@ -370,7 +380,7 @@ out:
err:
hardif_free_ref(hard_iface);
- return -ENOMEM;
+ return ret;
}
void hardif_disable_interface(struct hard_iface *hard_iface)
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 6b514ec..9ed2614 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -622,6 +622,19 @@ void softif_destroy(struct net_device *soft_iface)
unregister_netdevice(soft_iface);
}
+int softif_is_valid(struct net_device *net_dev)
+{
+#ifdef HAVE_NET_DEVICE_OPS
+ if (net_dev->netdev_ops->ndo_start_xmit == interface_tx)
+ return 1;
+#else
+ if (net_dev->hard_start_xmit == interface_tx)
+ return 1;
+#endif
+
+ return 0;
+}
+
/* ethtool */
static int bat_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
{
diff --git a/batman-adv/soft-interface.h b/batman-adv/soft-interface.h
index 80a3607..4789b6f 100644
--- a/batman-adv/soft-interface.h
+++ b/batman-adv/soft-interface.h
@@ -31,5 +31,6 @@ void interface_rx(struct net_device *soft_iface,
int hdr_size);
struct net_device *softif_create(char *name);
void softif_destroy(struct net_device *soft_iface);
+int softif_is_valid(struct net_device *net_dev);
#endif /* _NET_BATMAN_ADV_SOFT_INTERFACE_H_ */
--
1.7.4.1
4 Mar
4 Mar
8:55 p.m.
New subject: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
On Thursday 03 March 2011 22:08:39 Sven Eckelmann wrote:
When trying to associate a net_device with another
net_device which
already exists, batman-adv assumes that this interface is a fully
initialized batman mesh interface without checking it. The behaviour
when accessing data behind netdev_priv of a random net_device is
undefined and potentially dangerous.
Linus, can you confirm this patch fixes the problem for you ?
Thanks,
Marek
9:18 p.m.
New subject: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
On Fri, Mar 04, 2011 at 09:55:12PM +0100, Marek Lindner wrote:
On Thursday 03 March 2011 22:08:39 Sven Eckelmann
wrote:
Yep, works as expected, thanks Sven!
"Can't create batman mesh interface interface %s:"
-> there's one 'interface' too much though
Cheers, Linus
When trying to associate a net_device with
another net_device which
already exists, batman-adv assumes that this interface is a fully
initialized batman mesh interface without checking it. The behaviour
when accessing data behind netdev_priv of a random net_device is
undefined and potentially dangerous.
Linus, can you confirm this patch fixes the problem for you ?
Thanks,
Marek
9:24 p.m.
New subject: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
On Friday 04 March 2011 22:18:44 Linus Lüssing wrote:
Yep, works as expected, thanks Sven!
Ok.
"Can't create batman mesh interface interface
%s:"
-> there's one 'interface' too much though
I will fix that before committing the patch.
Cheers,
Marek
9:34 p.m.
New subject: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
On Thursday 03 March 2011 22:08:39 Sven Eckelmann wrote:
When trying to associate a net_device with another
net_device which
already exists, batman-adv assumes that this interface is a fully
initialized batman mesh interface without checking it. The behaviour
when accessing data behind netdev_priv of a random net_device is
undefined and potentially dangerous.
Applied in revision 1955.
Thanks,
Marek
4468
days inactive
4469
days old
b.a.t.m.a.n@lists.open-mesh.org
8 comments
4 participants
participants (4)
-
Linus Lüssing -
Linus Lüssing -
Marek Lindner -
Sven Eckelmann