Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c

[Kees Cook <keescook@xxxxxxxxxxxx>]

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies