[bug report] wireless: mwifiex: initial commit for Marvell mwifiex driver

[bug report] wireless: mwifiex: initial commit for Marvell mwifiex driver

[Dan Carpenter]

[ This is old, but maybe the driver is still really actively maintained
  so maybe someone knows the answer.  - dan ]

Hello Marvell Developers,

The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for
Marvell mwifiex driver" from Mar 21, 2011, leads to the following
static checker warning:

	drivers/net/wireless/marvell/mwifiex/11n.c:505 mwifiex_11n_delete_tx_ba_stream_tbl_entry()
	error: we previously assumed 'tx_ba_tsr_tbl' could be null (see line 498)

drivers/net/wireless/marvell/mwifiex/11n.c
   472  /*
   473   * This function checks if the given pointer is valid entry of
   474   * Tx BA Stream table.
   475   */
   476  static int mwifiex_is_tx_ba_stream_ptr_valid(struct mwifiex_private *priv,
   477                                  struct mwifiex_tx_ba_stream_tbl *tx_tbl_ptr)
                                                                         ^^^^^^^^^^
This is always NULL.

   478  {
   479          struct mwifiex_tx_ba_stream_tbl *tx_ba_tsr_tbl;
   480  
   481          list_for_each_entry(tx_ba_tsr_tbl, &priv->tx_ba_stream_tbl_ptr, list) {
   482                  if (tx_ba_tsr_tbl == tx_tbl_ptr)
                            ^^^^^^^^^^^^^
tx_ba_tsr_tbl is the list iterator, which is never NULL so this will
never return true.

   483                          return true;
   484          }
   485  
   486          return false;
   487  }
   488  
   489  /*
   490   * This function deletes the given entry in Tx BA Stream table.
   491   *
   492   * The function also performs a validity check on the supplied
   493   * pointer before trying to delete.
   494   */
   495  void mwifiex_11n_delete_tx_ba_stream_tbl_entry(struct mwifiex_private *priv,
   496                                  struct mwifiex_tx_ba_stream_tbl *tx_ba_tsr_tbl)
   497  {
   498          if (!tx_ba_tsr_tbl &&
                     ^^^^^^^^^^^^^
Check for NULL

   499              mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl))
                                                            ^^^^^^^^^^^^^
Which is passed to here.  So maybe the NULL check is reversed?

   500                  return;
   501  
   502          mwifiex_dbg(priv->adapter, INFO,
   503                      "info: tx_ba_tsr_tbl %p\n", tx_ba_tsr_tbl);
   504  
   505          list_del(&tx_ba_tsr_tbl->list);
                          ^^^^^^^^^^^^^^^^^^^
Unchecked NULL dereference

   506  
   507          kfree(tx_ba_tsr_tbl);
   508  }

regards,
dan carpenter

Re: [bug report] wireless: mwifiex: initial commit for Marvell mwifiex driver

[Brian Norris]

On Tue, Mar 17, 2020 at 2:18 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> [ This is old, but maybe the driver is still really actively maintained
>   so maybe someone knows the answer.  - dan ]

I'm not sure what your definition of "active" is :) I also don't claim
to know "the" answer, but I'll provide one:

>    499              mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl))
>                                                             ^^^^^^^^^^^^^
> Which is passed to here.  So maybe the NULL check is reversed?

Maybe, but it also looks like this validity check has always been dead
code, and only serves a redundant purpose (to check that the list
entry is *really* part of the list that we're trying to delete from?).
That sounds like we should just delete
mwifiex_is_tx_ba_stream_ptr_valid().

Brian

RE: [EXT] Re: [bug report] wireless: mwifiex: initial commit for Marvell mwifiex driver

[Ganapathi Bhat]

Hi Dan/Brian

>    495  void mwifiex_11n_delete_tx_ba_stream_tbl_entry(struct
> mwifiex_private *priv,
>    496                                  struct mwifiex_tx_ba_stream_tbl *tx_ba_tsr_tbl)
>    497  {
>    498          if (!tx_ba_tsr_tbl &&
>                      ^^^^^^^^^^^^^
> Check for NULL
> 
>    499              mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl))
>                                                             ^^^^^^^^^^^^^ Which is passed to here.  So
> maybe the NULL check is reversed?

I think, it should have been like below: 

if (!tx_ba_tsr_tbl || !mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl)) . . . 


Regards,
Ganapathi

Re: [EXT] Re: [bug report] wireless: mwifiex: initial commit for Marvell mwifiex driver

[Brian Norris]

On Wed, Mar 18, 2020 at 7:25 AM Ganapathi Bhat <ganapathi.bhat@nxp.com> wrote:
> > maybe the NULL check is reversed?
>
> I think, it should have been like below:
>
> if (!tx_ba_tsr_tbl || !mwifiex_is_tx_ba_stream_ptr_valid(priv, tx_ba_tsr_tbl)) . . .

Ah, of course.

But I think my point still stands:
It's currently dead code, and even if it were correctly-written, it
would be redundant and unnecessary. So we should just remove it.

Brian