[PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
John notes that if the kzalloc of ieee->pHTInfo fails, we fail to call ieee80211_networks_free(). In addition, that function has an un-needed check before kfree(). Reported-by: John Whitmore <arigead@gmail.com> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> --- diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c index 90a097f2cd4e..97ff0371b5bb 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c @@ -78,8 +78,6 @@ static inline int ieee80211_networks_allocate(struct ieee80211_device *ieee) static inline void ieee80211_networks_free(struct ieee80211_device *ieee) { - if (!ieee->networks) - return; kfree(ieee->networks); ieee->networks = NULL; } @@ -180,6 +178,7 @@ struct net_device *alloc_ieee80211(int sizeof_priv) return dev; failed: + ieee80211_networks_free(ieee); if (dev) free_netdev(dev);
On Wed, Sep 26, 2018 at 10:52 AM, <valdis.kletnieks@vt.edu> wrote:
John notes that if the kzalloc of ieee->pHTInfo fails, we fail to call ieee80211_networks_free(). In addition, that function has an un-needed check before kfree().
Reported-by: John Whitmore <arigead@gmail.com> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu>
Reviewed-by: Kees Cook <keescook@chromium.org> (And this seems to be the only case of this -- I don't see this code trivially copy/pasted in other 80211 stacks.) -Kees
--- diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c index 90a097f2cd4e..97ff0371b5bb 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c @@ -78,8 +78,6 @@ static inline int ieee80211_networks_allocate(struct ieee80211_device *ieee)
static inline void ieee80211_networks_free(struct ieee80211_device *ieee) { - if (!ieee->networks) - return; kfree(ieee->networks); ieee->networks = NULL; } @@ -180,6 +178,7 @@ struct net_device *alloc_ieee80211(int sizeof_priv) return dev;
failed: + ieee80211_networks_free(ieee); if (dev) free_netdev(dev);
-- Kees Cook Pixel Security
On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletnieks@vt.edu wrote:
John notes that if the kzalloc of ieee->pHTInfo fails, we fail to call ieee80211_networks_free(). In addition, that function has an un-needed check before kfree().
Reported-by: John Whitmore <arigead@gmail.com> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> --- diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c index 90a097f2cd4e..97ff0371b5bb 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c @@ -78,8 +78,6 @@ static inline int ieee80211_networks_allocate(struct ieee80211_device *ieee)
static inline void ieee80211_networks_free(struct ieee80211_device *ieee) { - if (!ieee->networks) - return; kfree(ieee->networks); ieee->networks = NULL; } @@ -180,6 +178,7 @@ struct net_device *alloc_ieee80211(int sizeof_priv) return dev;
failed: + ieee80211_networks_free(ieee); if (dev) free_netdev(dev);
When there is a "goto failed;" then it's called "one err style" error handling and we're just asking for bugs... In this case the bug is that we're not allowd to call ieee80211_networks_free() with a NULL "ieee" parameter. The right thing to do is to only call ieee80211_networks_free() if we know that ieee80211_networks_allocate() succeeded. regards, dan carpenter
On Thu, Sep 27, 2018 at 7:24 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletnieks@vt.edu wrote:
John notes that if the kzalloc of ieee->pHTInfo fails, we fail to call ieee80211_networks_free(). In addition, that function has an un-needed check before kfree().
Reported-by: John Whitmore <arigead@gmail.com> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> --- diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c index 90a097f2cd4e..97ff0371b5bb 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c @@ -78,8 +78,6 @@ static inline int ieee80211_networks_allocate(struct ieee80211_device *ieee)
static inline void ieee80211_networks_free(struct ieee80211_device *ieee) { - if (!ieee->networks) - return; kfree(ieee->networks); ieee->networks = NULL; } @@ -180,6 +178,7 @@ struct net_device *alloc_ieee80211(int sizeof_priv) return dev;
failed: + ieee80211_networks_free(ieee); if (dev) free_netdev(dev);
When there is a "goto failed;" then it's called "one err style" error handling and we're just asking for bugs... In this case the bug is that we're not allowd to call ieee80211_networks_free() with a NULL "ieee" parameter. The right thing to do is to only call ieee80211_networks_free() if we know that ieee80211_networks_allocate() succeeded.
Ah yeah, thanks for the extra eyes. Looks like it would need something more like this: diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c index 90a097f2cd4e..8d6a28af96dc 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_module.c @@ -78,7 +78,7 @@ static inline int ieee80211_networks_allocate(struct ieee80211_device *ieee) static inline void ieee80211_networks_free(struct ieee80211_device *ieee) { - if (!ieee->networks) + if (!ieee) return; kfree(ieee->networks); ieee->networks = NULL; @@ -97,7 +97,7 @@ static inline void ieee80211_networks_initialize(struct ieee80211_device *ieee) struct net_device *alloc_ieee80211(int sizeof_priv) { - struct ieee80211_device *ieee; + struct ieee80211_device *ieee = NULL; struct net_device *dev; int i, err; @@ -180,6 +180,7 @@ struct net_device *alloc_ieee80211(int sizeof_priv) return dev; failed: + ieee80211_networks_free(ieee); if (dev) free_netdev(dev); Valdis, can you respin the patch? -Kees -- Kees Cook Pixel Security
participants (3)
-
Dan Carpenter -
Kees Cook -
valdis.kletnieks＠vt.edu