| Message ID | 1482932827-2035-2-git-send-email-andrei.otcheretianski@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Dec 28, 2016 at 03:47:07PM +0200, Andrei Otcheretianski wrote: > It was possible that nl80211_get_hw_feature_data() function would return > NULL when num_modes is not set to zero. This might result in a later crash > when accessing hw.modes. This may be reproduced with hwsim oom tests, for > example, dbus_connect_oom. > Fix that by zeroing num_modes if NULL is returned. I haven't been able to reproduce this.. Would you be able to identify the caller that does not check the returned pointer? There should be no places where *num_modes is used if NULL is returned.. > diff --git a/src/drivers/driver_nl80211_capa.c b/src/drivers/driver_nl80211_capa.c > @@ -1771,6 +1771,7 @@ nl80211_get_hw_feature_data(void *priv, u16 *num_modes, u16 *flags) > os_free(result.modes[i].rates); > } > os_free(result.modes); > + *num_modes = 0; > return NULL; > } > return wpa_driver_nl80211_postprocess_modes(result.modes, This does not look like a complete fix since the function can return NULL also if processing of NL80211_CMD_GET_WIPHY response fails. I'd assume this could potentially happen after having already incremented *num_modes. In any case, if this can really be hit with the current hostap.git snapshot, more appropriate fix would be to modify the caller that uses *num_modes if NULL is returned from get_hw_feature_data().
> -----Original Message----- > From: Jouni Malinen [mailto:j@w1.fi] > Sent: Friday, December 30, 2016 23:45 > To: Otcheretianski, Andrei <andrei.otcheretianski@intel.com> > Cc: hostap@lists.infradead.org > Subject: Re: [PATCH] nl80211: Zero num_modes if > nl80211_get_hw_feature_data() fails > > On Wed, Dec 28, 2016 at 03:47:07PM +0200, Andrei Otcheretianski wrote: > > It was possible that nl80211_get_hw_feature_data() function would > > return NULL when num_modes is not set to zero. This might result in a > > later crash when accessing hw.modes. This may be reproduced with hwsim > > oom tests, for example, dbus_connect_oom. > > Fix that by zeroing num_modes if NULL is returned. > > I haven't been able to reproduce this.. Would you be able to identify the > caller that does not check the returned pointer? There should be no places > where *num_modes is used if NULL is returned.. Here is my log from dbus_connect_oom: 1483359087.370642: WPA_TRACE: eloop SIGSEGV - START 1483359087.375423: [1]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant() [0x44c56e] 1483359087.375895: eloop_sigsegv_handler() ../src/utils/eloop.c:123 1483359087.389225: [2]: /lib/x86_64-linux-gnu/libc.so.6(+0x36d40) [0x7efeb2282d40] 1483359087.389904: [3]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(get_mode+0x5) [0x586925] 1483359087.390466: get_mode() wpa_supplicant.c:6858 1483359087.391223: [4]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(wpas_supp_op_class_ie+0xe5) [0x448ed5] 1483359087.391750: wpas_op_class_supported() op_classes.c:203 1483359087.392341: wpas_supp_op_class_ie() op_classes.c:291 1483359087.393186: [5]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant() [0x569ed3] 1483359087.393553: sme_send_authentication() sme.c:445 1483359087.394147: [6]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant() [0x57f9f3] 1483359087.394583: radio_start_next_work() wpa_supplicant.c:4462 1483359087.395122: [7]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(eloop_run+0x1bb) [0x44d4cb] 1483359087.395470: eloop_run() ../src/utils/eloop.c:1196 1483359087.396030: [8]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(wpa_supplicant_run+0x77) [0x580ec7] 1483359087.396502: wpa_supplicant_run() wpa_supplicant.c:5585 1483359087.396891: [9]: /home/tester/maintain/iwlwifi-hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(main+0x3a8) [0x43b7b8] 1483359087.397441: main() main.c:392 1483359087.397824: WPA_TRACE: eloop SIGSEGV - END > > > diff --git a/src/drivers/driver_nl80211_capa.c > > b/src/drivers/driver_nl80211_capa.c > > @@ -1771,6 +1771,7 @@ nl80211_get_hw_feature_data(void *priv, u16 > *num_modes, u16 *flags) > > os_free(result.modes[i].rates); > > } > > os_free(result.modes); > > + *num_modes = 0; > > return NULL; > > } > > return > wpa_driver_nl80211_postprocess_modes(result.modes, > > This does not look like a complete fix since the function can return NULL also > if processing of NL80211_CMD_GET_WIPHY response fails. I'd assume this > could potentially happen after having already incremented *num_modes. I don't see how it can happen without having result.failed == 1. > In any case, if this can really be hit with the current hostap.git snapshot, more > appropriate fix would be to modify the caller that uses *num_modes if NULL > is returned from get_hw_feature_data(). IMHO get_hw_feature_data() must be consistent and should set num_modes = 0 when it returns NULL, at least this is what can be understood from it's documentation "@num_modes: Variable for returning the number of returned modes" > > -- > Jouni Malinen PGP id EFC895FA
On Wed, Dec 28, 2016 at 03:47:07PM +0200, Andrei Otcheretianski wrote: > It was possible that nl80211_get_hw_feature_data() function would return > NULL when num_modes is not set to zero. This might result in a later crash > when accessing hw.modes. This may be reproduced with hwsim oom tests, for > example, dbus_connect_oom. > Fix that by zeroing num_modes if NULL is returned. Thanks, applied.
diff --git a/src/drivers/driver_nl80211_capa.c b/src/drivers/driver_nl80211_capa.c index 85706ef..1bea3ba 100644 --- a/src/drivers/driver_nl80211_capa.c +++ b/src/drivers/driver_nl80211_capa.c @@ -1771,6 +1771,7 @@ nl80211_get_hw_feature_data(void *priv, u16 *num_modes, u16 *flags) os_free(result.modes[i].rates); } os_free(result.modes); + *num_modes = 0; return NULL; } return wpa_driver_nl80211_postprocess_modes(result.modes,
It was possible that nl80211_get_hw_feature_data() function would return NULL when num_modes is not set to zero. This might result in a later crash when accessing hw.modes. This may be reproduced with hwsim oom tests, for example, dbus_connect_oom. Fix that by zeroing num_modes if NULL is returned. Signed-off-by: Andrei Otcheretianski <andrei.otcheretianski@intel.com> --- src/drivers/driver_nl80211_capa.c | 1 + 1 file changed, 1 insertion(+)