[PATCH] batman-adv: fix null pointer dereference in batadv_gw_election

[PATCH] batman-adv: fix null pointer dereference in batadv_gw_election

[Wen Yang]

This patch fixes a possible null pointer dereference in
batadv_gw_election, detected by the semantic patch
deref_null.cocci, with the following warning:

./net/batman-adv/gateway_client.c:289:15-24: ERROR: next_gw is NULL but dereferenced.
./net/batman-adv/gateway_client.c:290:15-29: ERROR: next_gw is NULL but dereferenced.
./net/batman-adv/gateway_client.c:291:15-29: ERROR: next_gw is NULL but dereferenced.
./net/batman-adv/gateway_client.c:292:15-27: ERROR: next_gw is NULL but dereferenced.
./net/batman-adv/gateway_client.c:293:15-27: ERROR: next_gw is NULL but dereferenced.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Reviewed-by: Tan Hu <tan.hu@zte.com.cn>
---
 net/batman-adv/gateway_client.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index 140c61a..d80ef1c 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -284,14 +284,16 @@ void batadv_gw_election(struct batadv_priv *bat_priv)
 		batadv_throw_uevent(bat_priv, BATADV_UEV_GW, BATADV_UEV_ADD,
 				    gw_addr);
 	} else {
-		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
-			   "Changing route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, tq: %i)\n",
-			   next_gw->orig_node->orig,
-			   next_gw->bandwidth_down / 10,
-			   next_gw->bandwidth_down % 10,
-			   next_gw->bandwidth_up / 10,
-			   next_gw->bandwidth_up % 10,
-			   router_ifinfo->bat_iv.tq_avg);
+		if (next_gw) {
+			batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
+				   "Changing route to gateway %pM (bandwidth: %u.%u/%u.%u MBit, tq: %i)\n",
+				    next_gw->orig_node->orig,
+				    next_gw->bandwidth_down / 10,
+				    next_gw->bandwidth_down % 10,
+				    next_gw->bandwidth_up / 10,
+				    next_gw->bandwidth_up % 10,
+				    router_ifinfo->bat_iv.tq_avg);
+		}
 		batadv_throw_uevent(bat_priv, BATADV_UEV_GW, BATADV_UEV_CHANGE,
 				    gw_addr);
 	}
-- 
2.9.5

Re: [PATCH] batman-adv: fix null pointer dereference in batadv_gw_election

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

On Friday, 30 November 2018 15:00:02 CET Wen Yang wrote:
> This patch fixes a possible null pointer dereference in
> batadv_gw_election, detected by the semantic patch
> deref_null.cocci, with the following warning:
> 
> ./net/batman-adv/gateway_client.c:289:15-24: ERROR: next_gw is NULL but dereferenced.
> ./net/batman-adv/gateway_client.c:290:15-29: ERROR: next_gw is NULL but dereferenced.
> ./net/batman-adv/gateway_client.c:291:15-29: ERROR: next_gw is NULL but dereferenced.
> ./net/batman-adv/gateway_client.c:292:15-27: ERROR: next_gw is NULL but dereferenced.
> ./net/batman-adv/gateway_client.c:293:15-27: ERROR: next_gw is NULL but dereferenced.

This patch is seems to be nonsensical. next_gw cannot be NULL at this point 
(let us call this location in the code "4."). Let us go through the code

    // 1. when both are NULL then it would jump out of the the function.
	if (curr_gw == next_gw)
		goto out;

    [...]

    if (curr_gw && !next_gw) {
        [...]
        // 2. this handles the only valid case when next_gw is NULL
    }  else if (!curr_gw && next_gw) {
        // 3. here we know that next_gw is not NULL and curr_gw is NULL
        // we can therefore infer that 
        [...]
    } else {
        // 4. here you try to add an ugly patch to handle a non-existing 
        // next_gw == NULL case
        [...]
    }

Let us go through all possible combinations:

        curr_gw  next_gw
    I   0        0
    II  x        0
    III 0        y
    IV  x        y

For I:   we would leave the function even at 1. and never reach 4.
For II:  would be handled by 2. and thus never reach 4.
For III: would be handled by 3. and thus never reach 4.
For IV:  This can be handled by 1. (when x == y). Or otherwise it would be
         handled by 4. but is not the next_gw == NULL case.

Please correct me (in case I missed something) but it looks to me that this 
patch should be rejected.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]