re: mwifiex: add cfg80211 handlers add/del_virtual_intf

re: mwifiex: add cfg80211 handlers add/del_virtual_intf

[Dan Carpenter]

Hi Yogesh,

The Smatch static checker complains that 93a1df48d22429 "mwifiex: add
cfg80211 handlers add/del_virtual_intf" introduces a potential NULL
dereference.

@@ -985,9 +817,20 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
                       atomic_read(&adapter->cmd_pending));
        }

-       /* Remove interface */
-       for (i = 0; i < adapter->priv_num; i++)
-               mwifiex_remove_interface(adapter, i);
+       for (i = 0; i < adapter->priv_num; i++) {
+               priv = adapter->priv[i];
+
+               if (!priv)
+                       continue;
+
+               rtnl_lock();
+               mwifiex_del_virtual_intf(priv->wdev->wiphy, priv->netdev);
+               rtnl_unlock();
+       }
+
+       wiphy_unregister(priv->wdev->wiphy);
                         ^^^^^^^^^^
Can priv be NULL here?  How do we know that the last element of
adapter->priv[]; is non-NULL when we have to check the others?

regards,
dan carpenter

RE: mwifiex: add cfg80211 handlers add/del_virtual_intf

[Bing Zhao]

Hi Dan,

Thanks for your comment.

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, October 04, 2011 11:08 PM
> To: Yogesh Powar
> Cc: Bing Zhao; linux-wireless@vger.kernel.org
> Subject: re: mwifiex: add cfg80211 handlers add/del_virtual_intf
> 
> Hi Yogesh,
> 
> The Smatch static checker complains that 93a1df48d22429 "mwifiex: add
> cfg80211 handlers add/del_virtual_intf" introduces a potential NULL
> dereference.
> 
> @@ -985,9 +817,20 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
>                        atomic_read(&adapter->cmd_pending));
>         }
> 
> -       /* Remove interface */
> -       for (i = 0; i < adapter->priv_num; i++)
> -               mwifiex_remove_interface(adapter, i);
> +       for (i = 0; i < adapter->priv_num; i++) {
> +               priv = adapter->priv[i];
> +
> +               if (!priv)
> +                       continue;
> +
> +               rtnl_lock();
> +               mwifiex_del_virtual_intf(priv->wdev->wiphy, priv->netdev);
> +               rtnl_unlock();
> +       }
> +
> +       wiphy_unregister(priv->wdev->wiphy);
>                          ^^^^^^^^^^
> Can priv be NULL here?  How do we know that the last element of
> adapter->priv[]; is non-NULL when we have to check the others?

That's a good catch.

Since the first element of priv[] cannot be NULL. How about this fix?

+       priv = adapter->priv[0];
+       BUG_ON(!priv);
+
        wiphy_unregister(priv->wdev->wiphy);

Thanks,
Bing

> 
> regards,
> dan carpenter

Re: mwifiex: add cfg80211 handlers add/del_virtual_intf

[Dan Carpenter]

On Wed, Oct 05, 2011 at 12:08:36PM -0700, Bing Zhao wrote:
> Since the first element of priv[] cannot be NULL. How about this fix?
> 
> +       priv = adapter->priv[0];
> +       BUG_ON(!priv);
> +
>         wiphy_unregister(priv->wdev->wiphy);
> 

I don't know the code at all, so I'll trust you on that.  :)

But please don't add the BUG_ON().  Calls to BUG_ON() make the code
messier, and they are more painful than needed for the user.  If we
don't have the BUG_ON() then we get an Oops and the driver will die,
but we can still can close our documents and reboot the system.

Debugging NULL dereference bugs is normally super easy.  Just compile
with CONFIG_DEBUG_INFO=y, run gdb on the .ko file and type:
"list *(function_name+0x63)" at the gdb prompt.  Done.

regards,
dan carpenter

RE: mwifiex: add cfg80211 handlers add/del_virtual_intf

[Bing Zhao]

Hi Dan,

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, October 05, 2011 12:52 PM
> To: Bing Zhao
> Cc: Yogesh Powar; linux-wireless@vger.kernel.org
> Subject: Re: mwifiex: add cfg80211 handlers add/del_virtual_intf
> 
> On Wed, Oct 05, 2011 at 12:08:36PM -0700, Bing Zhao wrote:
> > Since the first element of priv[] cannot be NULL. How about this fix?
> >
> > +       priv = adapter->priv[0];
> > +       BUG_ON(!priv);
> > +
> >         wiphy_unregister(priv->wdev->wiphy);
> >
> 
> I don't know the code at all, so I'll trust you on that.  :)
> 
> But please don't add the BUG_ON().  Calls to BUG_ON() make the code
> messier, and they are more painful than needed for the user.  If we
> don't have the BUG_ON() then we get an Oops and the driver will die,
> but we can still can close our documents and reboot the system.
> 
> Debugging NULL dereference bugs is normally super easy.  Just compile
> with CONFIG_DEBUG_INFO=y, run gdb on the .ko file and type:
> "list *(function_name+0x63)" at the gdb prompt.  Done.

Thanks for the information.

Regards,
Bing

> 
> regards,
> dan carpenter