Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
On Wed, Sep 26, 2018 at 10:52 AM, 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
> Signed-off-by: Valdis Kletnieks
Reviewed-by: Kees Cook
(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
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
On Thu, Sep 27, 2018 at 7:24 AM, Dan Carpenter wrote:
> On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletni...@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
>> Signed-off-by: Valdis Kletnieks
>> ---
>> 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@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] fix error handling in drivers/staging/rtl8192u/ieee80211/ieee80211_module.c
On Wed, Sep 26, 2018 at 01:52:17PM -0400, valdis.kletni...@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
> Signed-off-by: Valdis Kletnieks
> ---
> 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
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
[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
Signed-off-by: Valdis Kletnieks
---
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);
___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
