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
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)
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
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_ */
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_ */
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
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
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Linus Lüssing -
Linus Lüssing -
Marek Lindner -
Sven Eckelmann