Message ID | 1543561202-614-1-git-send-email-wen.yang99@zte.com.cn |
---|---|
State | Rejected, archived |
Delegated to: | Simon Wunderlich |
Headers | show |
Series | batman-adv: fix null pointer dereference in batadv_gw_election | expand |
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
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); }