[ 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
On Tue, Mar 17, 2020 at 2:18 AM Dan Carpenter <[email protected]> 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
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
On Wed, Mar 18, 2020 at 7:25 AM Ganapathi Bhat <[email protected]> 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