[B.A.T.M.A.N.] [PATCH maint] batman-adv: Initialize memory for station_info

[B.A.T.M.A.N.] [PATCH maint] batman-adv: Initialize memory for station_info

[Sven Eckelmann]

cfg80211_get_station is not initializing the memory given as parameter
sinfo. The caller has to handle it. Otherwise the filled parameter may be
set incorrectly and thus uninitialized memory is used to identify the
throughput to an neighbor.

Fixes: 5c3245172c01 ("batman-adv: ELP - compute the metric based on the estimated throughput")
Reported-by: Thomas Lauer <holminateur@gmail.com>
Reported-by: Marcel Schmidt <ff.z-casparistrasse@mailbox.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---

Cc: Thomas Lauer <holminateur@gmail.com>
Cc: Marcel Schmidt <ff.z-casparistrasse@mailbox.org>

 net/batman-adv/bat_v_elp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index 71c20c1d..5f931475 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -102,6 +102,7 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh)
 		if (!real_netdev)
 			goto default_throughput;
 
+		memset(&sinfo, 0, sizeof(sinfo));
 		ret = cfg80211_get_station(real_netdev, neigh->addr, &sinfo);
 
 		dev_put(real_netdev);
-- 
2.11.0

Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Initialize memory for station_info

[Sven Eckelmann]

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

Hi Antonio,

On Dienstag, 5. Juni 2018 20:31:31 CEST Sven Eckelmann wrote:
> cfg80211_get_station is not initializing the memory given as parameter
> sinfo. The caller has to handle it. Otherwise the filled parameter may be
> set incorrectly and thus uninitialized memory is used to identify the
> throughput to an neighbor.

Hm, have to rewrite the commit message to something better. Maybe to something 
like:

batadv_v_elp_get_throughput is calling cfg80211_get_station with a pointer
(sinfo) to some uninitialized memory on the stack. But most of the
implementations behind cfg80211_get_station will not initialize sinfo to
zero before manipulating it. For example, the member
&struct station_info.filled is often only modified by using a read (of
possibly uninitialized/random memory), an OR operation and then a write of
the new value back to the original memory address. A caller without a
preinitialized &struct station_info.filled can then no longer decide which
parts of sinfo were filled in by cfg80211_get_station.

The caller of cfg80211_get_station must therefore take care that sinfo (or
at least sinfo.filled) is initialized to zero. Otherwise, the caller may
tries to read information which was not filled in and is therefore also
uninitialized. In batadv_v_elp_get_throughput's case, an invalid "random"
expected throughput may be saved for this neighbor and thus the B.A.T.M.A.N
V algorithm may switch to non-optimal neighbors for certain destinations.

> Fixes: 5c3245172c01 ("batman-adv: ELP - compute the metric based on the estimated throughput")
> Reported-by: Thomas Lauer <holminateur@gmail.com>
> Reported-by: Marcel Schmidt <ff.z-casparistrasse@mailbox.org>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> 
> Cc: Thomas Lauer <holminateur@gmail.com>
> Cc: Marcel Schmidt <ff.z-casparistrasse@mailbox.org>

Here some more information why I thought that this could be the reason (still 
has to be tested by the reporters):

* sinfo is usually allocated with alloc helper that initializes everything 
  to 0 [1,2]
* filled is set to 0 by functions which have sinfo on stack and call the 
  (internal) functionality [3,4]
* most implementations for cfg80211_get_station
  (&struct cfg80211_ops->get_station) [5] are *not* setting filled manually
  to a predefined value
  - default implementation [6] and its main helper [7] doesn't do that
  - ath6kl [8] doesn't do that
  - (ath10k [9] doesn't do that for statistics)
  - wil6210 [10] does it
  - brcmfmac does it for everything [11] except ibss [12]
  - (iwlwifi doesn't do that for the statistics [13])
  - libertas doesn't do that [14]
  - mwifiex does it [15]
  - quantenna doesn't do it [16,17]
  - (wlcore doesn't do it [18] for statistics)
  - rndis_wlan is doing it [19]
  - rtl8723bs does it [20]
  - (rtlwifi does it for statistics [21])
  - wilc1000 doesn't do it [22]
  - prism2 does it [23]
* nl80211 interface also does the memset [24]
* wext compat layer does it everywhere(?) [25]

But please check this again because a lot of the "research" was done in a
restaurant.

Kind regards,
	Sven

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/sta_info.c?id=5037be168f0e4ee910602935b1180291082d3aac#n558
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/sta_info.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1007
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/ethtool.c?id=5037be168f0e4ee910602935b1180291082d3aac#n109
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/ethtool.c?id=5037be168f0e4ee910602935b1180291082d3aac#n136
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/wireless/util.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1735
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/cfg.c?id=5037be168f0e4ee910602935b1180291082d3aac#n714
[7] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/mac80211/sta_info.c?id=5037be168f0e4ee910602935b1180291082d3aac#n2069
[8] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath6kl/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1774
[9] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/ath10k/mac.c?id=5037be168f0e4ee910602935b1180291082d3aac#n7676
[10] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ath/wil6210/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n303
[11] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n2489
[12] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n2574
[13] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n4174
[14] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/marvell/libertas/cfg.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1554
[15] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/marvell/mwifiex/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1352
[16] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n466
[17] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/quantenna/qtnfmac/commands.c?id=5037be168f0e4ee910602935b1180291082d3aac#n582
[18] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore//main.c?id=5037be168f0e4ee910602935b1180291082d3aac#n5705
[19] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/rndis_wlan.c?id=5037be168f0e4ee910602935b1180291082d3aac#n2477
[20] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1252
[21] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtlwifi/core.c?id=5037be168f0e4ee910602935b1180291082d3aac#n974
[22] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c?id=5037be168f0e4ee910602935b1180291082d3aac#n1162
[23] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/wlan-ng/cfg80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n267
[24] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/wireless/nl80211.c?id=5037be168f0e4ee910602935b1180291082d3aac#n4609
[25] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/wireless/wext-compat.c?id=5037be168f0e4ee910602935b1180291082d3aac

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

Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Initialize memory for station_info

[Antonio Quartulli]

[-- Attachment #1.1: Type: text/plain, Size: 2508 bytes --]

Hi,

On 06/06/18 06:01, Sven Eckelmann wrote:
> Hi Antonio,
> 
> On Dienstag, 5. Juni 2018 20:31:31 CEST Sven Eckelmann wrote:
>> cfg80211_get_station is not initializing the memory given as parameter
>> sinfo. The caller has to handle it. Otherwise the filled parameter may be
>> set incorrectly and thus uninitialized memory is used to identify the
>> throughput to an neighbor.
> 
> Hm, have to rewrite the commit message to something better. Maybe to something 
> like:
> 
> batadv_v_elp_get_throughput is calling cfg80211_get_station with a pointer
> (sinfo) to some uninitialized memory on the stack. But most of the
> implementations behind cfg80211_get_station will not initialize sinfo to
> zero before manipulating it. For example, the member
> &struct station_info.filled is often only modified by using a read (of
> possibly uninitialized/random memory), an OR operation and then a write of
> the new value back to the original memory address. A caller without a
> preinitialized &struct station_info.filled can then no longer decide which
> parts of sinfo were filled in by cfg80211_get_station.
> 
> The caller of cfg80211_get_station must therefore take care that sinfo (or
> at least sinfo.filled) is initialized to zero. Otherwise, the caller may
> tries to read information which was not filled in and is therefore also
> uninitialized. In batadv_v_elp_get_throughput's case, an invalid "random"
> expected throughput may be saved for this neighbor and thus the B.A.T.M.A.N
> V algorithm may switch to non-optimal neighbors for certain destinations.
> 
>> Fixes: 5c3245172c01 ("batman-adv: ELP - compute the metric based on the estimated throughput")
>> Reported-by: Thomas Lauer <holminateur@gmail.com>
>> Reported-by: Marcel Schmidt <ff.z-casparistrasse@mailbox.org>
>> Signed-off-by: Sven Eckelmann <sven@narfation.org>

This looks fairly reasonable and at a first glance I can confirm that
even ieee80211_get_station() does not clear it up before filling it.

However, what do you think about zero'ing the sinfo object directly in
cfg80211_get_station()?
I think it is safe to assume that no user should store anything in this
object before passing it to get_station().

At the same time future users of this API won't be able to hit the same
issue we just did.

(At the moment we are the only user of this function in the kernel,
therefore changing its behaviour now might still be an option)


Cheers,


-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Initialize memory for station_info

[Sven Eckelmann]

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

On Mittwoch, 6. Juni 2018 06:44:32 CEST Antonio Quartulli wrote:
[...]
> This looks fairly reasonable and at a first glance I can confirm that
> even ieee80211_get_station() does not clear it up before filling it.
> 
> However, what do you think about zero'ing the sinfo object directly in
> cfg80211_get_station()?
> I think it is safe to assume that no user should store anything in this
> object before passing it to get_station().

I understand your point. But there is the potential problem that such a change 
requires more discussion with the Johannes/other wireless guys - while the 
early B.A.T.M.A.N. V adopters still suffer under this problem. I will ask him 
in IRC first about his opinion but send a second version of the patch to the 
batman-adv mailing list. We can later decide which route we take.

Kind regards,
	Sven

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