Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
From: Kees Cook
Date: Thu Sep 27 2018 - 11:43:46 EST
On Thu, Sep 27, 2018 at 7:24 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletnieks@xxxxxx 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@xxxxxxxxx>
>> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@xxxxxx>
>> ---
>> 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