* [B.A.T.M.A.N.] Null pointer dereference, ticket #146
@ 2011-03-03 19:09 Linus Lüssing
2011-03-03 19:09 ` [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if Linus Lüssing
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Linus Lüssing @ 2011-03-03 19:09 UTC (permalink / raw)
To: b.a.t.m.a.n
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if
2011-03-03 19:09 [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Linus Lüssing
@ 2011-03-03 19:09 ` Linus Lüssing
2011-03-03 20:22 ` [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Sven Eckelmann
2011-03-03 20:39 ` [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if Sven Eckelmann
2 siblings, 0 replies; 9+ messages in thread
From: Linus Lüssing @ 2011-03-03 19:09 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] Null pointer dereference, ticket #146
2011-03-03 19:09 [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Linus Lüssing
2011-03-03 19:09 ` [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if Linus Lüssing
@ 2011-03-03 20:22 ` Sven Eckelmann
2011-03-03 20:39 ` [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if Sven Eckelmann
2 siblings, 0 replies; 9+ messages in thread
From: Sven Eckelmann @ 2011-03-03 20:22 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
[-- Attachment #1: Type: Text/Plain, Size: 987 bytes --]
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
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if
2011-03-03 19:09 [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Linus Lüssing
2011-03-03 19:09 ` [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if Linus Lüssing
2011-03-03 20:22 ` [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Sven Eckelmann
@ 2011-03-03 20:39 ` Sven Eckelmann
2011-03-03 21:08 ` [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device Sven Eckelmann
2 siblings, 1 reply; 9+ messages in thread
From: Sven Eckelmann @ 2011-03-03 20:39 UTC (permalink / raw)
To: b.a.t.m.a.n
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@ascom.ch>
Signed-off-by: Sven Eckelmann <sven@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
2011-03-03 20:39 ` [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if Sven Eckelmann
@ 2011-03-03 21:08 ` Sven Eckelmann
2011-03-04 20:55 ` Marek Lindner
2011-03-04 21:34 ` Marek Lindner
0 siblings, 2 replies; 9+ messages in thread
From: Sven Eckelmann @ 2011-03-03 21:08 UTC (permalink / raw)
To: b.a.t.m.a.n
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@ascom.ch>
Signed-off-by: Sven Eckelmann <sven@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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
2011-03-03 21:08 ` [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device Sven Eckelmann
@ 2011-03-04 20:55 ` Marek Lindner
2011-03-04 21:18 ` Linus Lüssing
2011-03-04 21:34 ` Marek Lindner
1 sibling, 1 reply; 9+ messages in thread
From: Marek Lindner @ 2011-03-04 20:55 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
2011-03-04 20:55 ` Marek Lindner
@ 2011-03-04 21:18 ` Linus Lüssing
2011-03-04 21:24 ` Marek Lindner
0 siblings, 1 reply; 9+ messages in thread
From: Linus Lüssing @ 2011-03-04 21:18 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Fri, Mar 04, 2011 at 09:55:12PM +0100, Marek Lindner wrote:
> 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
>
Yep, works as expected, thanks Sven!
"Can't create batman mesh interface interface %s:"
-> there's one 'interface' too much though
Cheers, Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
2011-03-04 21:18 ` Linus Lüssing
@ 2011-03-04 21:24 ` Marek Lindner
0 siblings, 0 replies; 9+ messages in thread
From: Marek Lindner @ 2011-03-04 21:24 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device
2011-03-03 21:08 ` [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device Sven Eckelmann
2011-03-04 20:55 ` Marek Lindner
@ 2011-03-04 21:34 ` Marek Lindner
1 sibling, 0 replies; 9+ messages in thread
From: Marek Lindner @ 2011-03-04 21:34 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-03-04 21:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-03 19:09 [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Linus Lüssing
2011-03-03 19:09 ` [B.A.T.M.A.N.] [PATCH] RFC: batman-adv: Fix null pointer deref. when adding hard-if as soft-if Linus Lüssing
2011-03-03 20:22 ` [B.A.T.M.A.N.] Null pointer dereference, ticket #146 Sven Eckelmann
2011-03-03 20:39 ` [B.A.T.M.A.N.] [PATCH] batman-adv: Fix null pointer deref. when adding hard-if as soft-if Sven Eckelmann
2011-03-03 21:08 ` [B.A.T.M.A.N.] [PATCHv2] batman-adv: Disallow regular interface as mesh device Sven Eckelmann
2011-03-04 20:55 ` Marek Lindner
2011-03-04 21:18 ` Linus Lüssing
2011-03-04 21:24 ` Marek Lindner
2011-03-04 21:34 ` Marek Lindner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox