Hello David,
although we are extremely late in the release cycle we have 4 fixes which would really be worth merging before releasing linux-4.6.
As you can read in the git tag below, each of them can lead to a kernel crash or to an unstable system.
We came up with several fixes after having tested our new B.A.T.M.A.N. V code at the Wireless Battle Mesh in Porto (PT) at the beginning of the month, however, what I am sending here is the minimum subset that we though being extremely important to avoid easy kernel crashes. The change footprint is also rather small.
Please pull or let me know if you rather prefer to get this through net-next.
If you decide to pull, you will hit some conflicts when merging net into net-next, but I can send you some instructions to ease the process.
Thanks a lot! Antonio
The following changes since commit b91506586206140154b0b44cccf88c8cc0a4dca5:
Merge branch 'xgene-fixes' (2016-05-13 21:12:07 -0400)
are available in the git repository at:
git://git.open-mesh.org/linux-merge.git tags/batman-adv-fix-for-davem
for you to fetch changes up to 6b892c1cb0805acee5d4ddd9e7878ed076c1b7c7:
batman-adv: Fix refcnt leak in batadv_v_neigh_* (2016-05-14 15:51:39 +0800)
---------------------------------------------------------------- During the Wireless Battle Mesh v9 in Porto (PT) at the beginning of May, we managed to uncover and fix some important bugs in our new B.A.T.M.A.N. V algorithm. These are the most critical fixes we came up with aimed to avoid easy kernel crashes: - avoid potential crash due to NULL pointer dereference in B.A.T.M.A.N. V routine when a neigh_ifinfo object is not found, by Sven Eckelmann - avoid crash due to double kref_put on neigh_node object in B.A.T.M.A.N. V routine leading to use-after-free, by Sven Eckelmann (this crash can be always replicated) - avoid use-after-free of skb when counting outgoing bytes, by Florian Westphal - fix neigh_ifinfo object reference counting imbalance when using B.A.T.M.A.N. V, by Sven Eckelmann. Such imbalance may lead to the impossibility of releasing the related netdev object on shutdown.
---------------------------------------------------------------- Florian Westphal (1): batman-adv: fix skb deref after free
Sven Eckelmann (3): batman-adv: Avoid nullptr derefence in batadv_v_neigh_is_sob batman-adv: Fix double neigh_node_put in batadv_v_ogm_route_update batman-adv: Fix refcnt leak in batadv_v_neigh_*
net/batman-adv/bat_v.c | 30 ++++++++++++++++++++++++++---- net/batman-adv/bat_v_ogm.c | 4 +++- net/batman-adv/routing.c | 4 +++- 3 files changed, 32 insertions(+), 6 deletions(-)
From: Florian Westphal fw@strlen.de
batadv_send_skb_to_orig() calls dev_queue_xmit() so we can't use skb->len.
Fixes: 953324776d6d ("batman-adv: network coding - buffer unicast packets before forward") Signed-off-by: Florian Westphal fw@strlen.de Reviewed-by: Sven Eckelmann sven@narfation.org Signed-off-by: Marek Lindner mareklindner@neomailbox.ch Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/routing.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index b781bf753250..0c0c30e101a1 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -601,6 +601,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, struct batadv_unicast_packet *unicast_packet; struct ethhdr *ethhdr = eth_hdr(skb); int res, hdr_len, ret = NET_RX_DROP; + unsigned int len;
unicast_packet = (struct batadv_unicast_packet *)skb->data;
@@ -641,6 +642,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, if (hdr_len > 0) batadv_skb_set_priority(skb, hdr_len);
+ len = skb->len; res = batadv_send_skb_to_orig(skb, orig_node, recv_if);
/* translate transmit result into receive result */ @@ -648,7 +650,7 @@ static int batadv_route_unicast_packet(struct sk_buff *skb, /* skb was transmitted and consumed */ batadv_inc_counter(bat_priv, BATADV_CNT_FORWARD); batadv_add_counter(bat_priv, BATADV_CNT_FORWARD_BYTES, - skb->len + ETH_HLEN); + len + ETH_HLEN);
ret = NET_RX_SUCCESS; } else if (res == NET_XMIT_POLICED) {
From: Sven Eckelmann sven@narfation.org
batadv_neigh_ifinfo_get can return NULL when it cannot find (even when only temporarily) anymore the neigh_ifinfo in the list neigh->ifinfo_list. This has to be checked to avoid kernel Oopses when the ifinfo is dereferenced.
This a situation which isn't expected but is already handled by functions like batadv_v_neigh_cmp. The same kind of warning is therefore used before the function returns without dereferencing the pointers.
Fixes: 9786906022eb ("batman-adv: B.A.T.M.A.N. V - implement neighbor comparison API calls") Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Marek Lindner mareklindner@neomailbox.ch Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/bat_v.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/net/batman-adv/bat_v.c b/net/batman-adv/bat_v.c index 4026f198a734..e81ad4b8e5c8 100644 --- a/net/batman-adv/bat_v.c +++ b/net/batman-adv/bat_v.c @@ -27,6 +27,7 @@ #include <linux/rculist.h> #include <linux/rcupdate.h> #include <linux/seq_file.h> +#include <linux/stddef.h> #include <linux/types.h> #include <linux/workqueue.h>
@@ -277,6 +278,9 @@ static bool batadv_v_neigh_is_sob(struct batadv_neigh_node *neigh1, ifinfo1 = batadv_neigh_ifinfo_get(neigh1, if_outgoing1); ifinfo2 = batadv_neigh_ifinfo_get(neigh2, if_outgoing2);
+ if (WARN_ON(!ifinfo1 || !ifinfo2)) + return false; + threshold = ifinfo1->bat_v.throughput / 4; threshold = ifinfo1->bat_v.throughput - threshold;
From: Sven Eckelmann sven@narfation.org
The router is put down twice when it was non-NULL and either orig_ifinfo is NULL afterwards or batman-adv receives a packet with the same sequence number. This will end up in a use-after-free when the batadv_neigh_node is removed because the reference counter ended up too early at 0.
Fixes: 9323158ef9f4 ("batman-adv: OGMv2 - implement originators logic") Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/bat_v_ogm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c index d9bcbe6e7d65..91df28a100f9 100644 --- a/net/batman-adv/bat_v_ogm.c +++ b/net/batman-adv/bat_v_ogm.c @@ -529,8 +529,10 @@ static void batadv_v_ogm_route_update(struct batadv_priv *bat_priv, goto out; }
- if (router) + if (router) { batadv_neigh_node_put(router); + router = NULL; + }
/* Update routes, and check if the OGM is from the best next hop */ batadv_v_ogm_orig_update(bat_priv, orig_node, neigh_node, ogm2,
From: Sven Eckelmann sven@narfation.org
The functions batadv_neigh_ifinfo_get increase the reference counter of the batadv_neigh_ifinfo. These have to be reduced again when the reference is not used anymore to correctly free the objects.
Fixes: 9786906022eb ("batman-adv: B.A.T.M.A.N. V - implement neighbor comparison API calls") Signed-off-by: Sven Eckelmann sven@narfation.org Signed-off-by: Marek Lindner mareklindner@neomailbox.ch Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/bat_v.c | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/net/batman-adv/bat_v.c b/net/batman-adv/bat_v.c index e81ad4b8e5c8..7fd477583eb0 100644 --- a/net/batman-adv/bat_v.c +++ b/net/batman-adv/bat_v.c @@ -257,14 +257,23 @@ static int batadv_v_neigh_cmp(struct batadv_neigh_node *neigh1, struct batadv_hard_iface *if_outgoing2) { struct batadv_neigh_ifinfo *ifinfo1, *ifinfo2; + int ret = 0;
ifinfo1 = batadv_neigh_ifinfo_get(neigh1, if_outgoing1); + if (WARN_ON(!ifinfo1)) + goto err_ifinfo1; + ifinfo2 = batadv_neigh_ifinfo_get(neigh2, if_outgoing2); + if (WARN_ON(!ifinfo2)) + goto err_ifinfo2;
- if (WARN_ON(!ifinfo1 || !ifinfo2)) - return 0; + ret = ifinfo1->bat_v.throughput - ifinfo2->bat_v.throughput;
- return ifinfo1->bat_v.throughput - ifinfo2->bat_v.throughput; + batadv_neigh_ifinfo_put(ifinfo2); +err_ifinfo2: + batadv_neigh_ifinfo_put(ifinfo1); +err_ifinfo1: + return ret; }
static bool batadv_v_neigh_is_sob(struct batadv_neigh_node *neigh1, @@ -274,17 +283,26 @@ static bool batadv_v_neigh_is_sob(struct batadv_neigh_node *neigh1, { struct batadv_neigh_ifinfo *ifinfo1, *ifinfo2; u32 threshold; + bool ret = false;
ifinfo1 = batadv_neigh_ifinfo_get(neigh1, if_outgoing1); - ifinfo2 = batadv_neigh_ifinfo_get(neigh2, if_outgoing2); + if (WARN_ON(!ifinfo1)) + goto err_ifinfo1;
- if (WARN_ON(!ifinfo1 || !ifinfo2)) - return false; + ifinfo2 = batadv_neigh_ifinfo_get(neigh2, if_outgoing2); + if (WARN_ON(!ifinfo2)) + goto err_ifinfo2;
threshold = ifinfo1->bat_v.throughput / 4; threshold = ifinfo1->bat_v.throughput - threshold;
- return ifinfo2->bat_v.throughput > threshold; + ret = ifinfo2->bat_v.throughput > threshold; + + batadv_neigh_ifinfo_put(ifinfo2); +err_ifinfo2: + batadv_neigh_ifinfo_put(ifinfo1); +err_ifinfo1: + return ret; }
static struct batadv_algo_ops batadv_batman_v __read_mostly = {
From: Antonio Quartulli a@unstable.cc Date: Sun, 15 May 2016 23:11:29 +0800
Please pull or let me know if you rather prefer to get this through net-next.
Since the merge window is open, please send me this via net-next.
Thank you.
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
David Miller