Patchwork batman-adv: Fix NULL pointer dereference in DAT hash collision avoidance

login
register
mail settings
Submitter Pau Koning
Date Feb. 12, 2013, 10:18 a.m.
Message ID <1360664325-7323-1-git-send-email-paukoning@gmail.com>
Download mbox | patch
Permalink /patch/219775/
State Accepted
Delegated to: David Miller
Headers show

Comments

Pau Koning - Feb. 12, 2013, 10:18 a.m.
An entry in DAT with the hashed position of 0 can cause a NULL pointer
dereference when the first entry is checked by batadv_choose_next_candidate.
This first candidate automatically has the max value of 0 and the max_orig_node
of NULL. Not checking max_orig_node for NULL in batadv_is_orig_node_eligible
will lead to a NULL pointer dereference when checking for the lowest address.

This problem was added in 785ea1144182c341b8b85b0f8180291839d176a8
("batman-adv: Distributed ARP Table - create DHT helper functions").

Signed-off-by: Pau Koning <paukoning@gmail.com>
---
 net/batman-adv/distributed-arp-table.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
David Miller - Feb. 13, 2013, 6:36 p.m.
From: Pau Koning <paukoning@gmail.com>
Date: Tue, 12 Feb 2013 11:18:45 +0100

> An entry in DAT with the hashed position of 0 can cause a NULL pointer
> dereference when the first entry is checked by batadv_choose_next_candidate.
> This first candidate automatically has the max value of 0 and the max_orig_node
> of NULL. Not checking max_orig_node for NULL in batadv_is_orig_node_eligible
> will lead to a NULL pointer dereference when checking for the lowest address.
> 
> This problem was added in 785ea1144182c341b8b85b0f8180291839d176a8
> ("batman-adv: Distributed ARP Table - create DHT helper functions").
> 
> Signed-off-by: Pau Koning <paukoning@gmail.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antonio Quartulli - Feb. 15, 2013, 8:30 a.m.
On Tue, Feb 12, 2013 at 11:18:45AM +0100, Pau Koning wrote:
> An entry in DAT with the hashed position of 0 can cause a NULL pointer
> dereference when the first entry is checked by batadv_choose_next_candidate.
> This first candidate automatically has the max value of 0 and the max_orig_node
> of NULL. Not checking max_orig_node for NULL in batadv_is_orig_node_eligible
> will lead to a NULL pointer dereference when checking for the lowest address.
> 
> This problem was added in 785ea1144182c341b8b85b0f8180291839d176a8
> ("batman-adv: Distributed ARP Table - create DHT helper functions").
> 
> Signed-off-by: Pau Koning <paukoning@gmail.com>


Hello Pau,

thank you very much for this fix, this was not an easy one!

However, next time please CC our mailing list as well (get_maintainer.pl will
give you all the needed addresses), otherwise it may be the case that we
overlook such patches and:
1) we don't review it
2) we don't merge it into our repository (which is where the real development
   goes on).

Both 1) and 2) happened with this patch and, in my humble opinion, it is not a
good idea to merge such delicate fixes without having a reply from the
maintainers.

Therefore, please keep us in the loop when sending patches. It would be really
appreciated.


Regards,

Patch

diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index 0e05ad4..d54188a 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -439,7 +439,7 @@  static bool batadv_is_orig_node_eligible(struct batadv_dat_candidate *res,
 	/* this is an hash collision with the temporary selected node. Choose
 	 * the one with the lowest address
 	 */
-	if ((tmp_max == max) &&
+	if ((tmp_max == max) && max_orig_node &&
 	    (batadv_compare_eth(candidate->orig, max_orig_node->orig) > 0))
 		goto out;