linux-kernel - Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 27 Sep 2018 08:43:39 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Valdis Kletnieks <valdis.kletnieks@...edu>,
        Greg KH <gregkh@...uxfoundation.org>,
        John Whitmore <arigead@...il.com>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernelnewbies@...nelnewbies.org
Subject: Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c

On Thu, Sep 27, 2018 at 7:24 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletnieks@...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@...il.com>
>> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@...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