1. 19 May, 2017 7 commits
    • Brian Norris's avatar
      mwifiex: don't leak stashed beacon buffer on reset · 6eb2d002
      Brian Norris authored
      When removing or resetting an mwifiex device, we don't remember to free
      the saved beacon buffer. Use the (somewhat misleadingly-named)
      mwifiex_free_priv() helper to handle this.
      
      Noticed by kmemleak during tests:
      
      echo 1 > /sys/bus/pci/devices/.../reset
      
      unreferenced object 0xffffffc09d034a00 (size 256):
      ...
        backtrace:
          [<ffffffc0003cdce4>] create_object+0x228/0x3c4
          [<ffffffc000c0b9d8>] kmemleak_alloc+0x54/0x88
          [<ffffffc0003c0848>] __kmalloc+0x1cc/0x2dc
          [<ffffffbffc1500c4>] mwifiex_save_curr_bcn+0x80/0x308 [mwifiex]
          [<ffffffbffc1516b8>] mwifiex_ret_802_11_associate+0x4ec/0x5fc [mwifiex]
          [<ffffffbffc15da90>] mwifiex_process_sta_cmdresp+0xaf8/0x1fa4 [mwifiex]
          [<ffffffbffc1411e0>] mwifiex_process_cmdresp+0x40c/0x510 [mwifiex]
          [<ffffffbffc13b8f4>] mwifiex_main_process+0x4a4/0xb00 [mwifiex]
          [<ffffffbffc13bf84>] mwifiex_main_work_queue+0x34/0x40 [mwifiex]
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      6eb2d002
    • Brian Norris's avatar
      mwifiex: don't drop lock between list-retrieval / list-deletion · 0f13acf0
      Brian Norris authored
      mwifiex_exec_next_cmd() seems to have a classic TOCTOU race, where we
      drop the list lock in between retrieving the next command and deleting
      it from the list. This potentially leaves room for someone else to also
      retrieve / steal this node from the list (e.g.,
      mwifiex_cancel_all_pending_cmd()).
      
      Let's keep holding the lock while we do our 'ps_state' sanity checks.
      There should be no harm in continuing to hold this lock for a bit more.
      
      Noticed only by code inspection.
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      0f13acf0
    • Douglas Anderson's avatar
      mwifiex: Add locking to mwifiex_11n_delba · 09bdb650
      Douglas Anderson authored
      The mwifiex_11n_delba() function walked the rx_reorder_tbl_ptr without
      holding the lock, which was an obvious violation.
      
      Grab the lock.
      
      NOTE: we hold the lock while calling mwifiex_send_delba().  There's also
      several callers in 11n_rxreorder.c that hold the lock and the comments
      in the struct sound just like very other list/lock pair -- as if the
      lock should definitely be help for all operations like this.
      Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      09bdb650
    • Douglas Anderson's avatar
      mwifiex: Don't release cmd_pending_q_lock while iterating · 90ad0be8
      Douglas Anderson authored
      Just like in the previous patch ("mwifiex: Don't release
      tx_ba_stream_tbl_lock while iterating"), in
      mwifiex_cancel_all_pending_cmd() we were itearting over a list protected
      by a spinlock.  Again, it is not safe to release the spinlock while
      iterating.  Don't do it.
      
      Luckily in this case there should be no need to release the spinlock.
      This is evidenced by:
      
      1. The only function called while the spinlock was released was
         mwifiex_recycle_cmd_node()
      2. Aside from atomic functions (which are safe to call), the only
         function called by mwifiex_recycle_cmd_node() was
         mwifiex_insert_cmd_to_free_q().
      3. It can be seen in mwifiex_cancel_pending_scan_cmd() that it's OK to
         call mwifiex_insert_cmd_to_free_q() while holding a different
         spinlock (scan_pending_q_lock), so in general holding a spinlock
         should be OK.
      4. It doesn't appear that mwifiex_insert_cmd_to_free_q() has any
         interaction with the cmd_pending_q_lock
      
      No known bugs are fixed with this change, but as with other similar
      changes this could fix random list corruption.
      Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      90ad0be8
    • Douglas Anderson's avatar
      mwifiex: Don't release tx_ba_stream_tbl_lock while iterating · e0b636e5
      Douglas Anderson authored
      Despite the macro list_for_each_entry_safe() having the word "safe" in
      the name, it's still not actually safe to release the list spinlock
      while iterating over the list.  The "safe" in the macro name actually
      only means that it's safe to delete the current entry while iterating
      over the list.
      
      Releasing the spinlock while iterating over the list means that someone
      else could come in and adjust the list while we don't have the
      spinlock.  If they do that it can totally mix up our iteration and fully
      corrupt the list.  Later iterating over a corrupted list while holding a
      spinlock and having IRQs off can cause all sorts of hard to debug
      problems.
      
      As evidenced by the other call to
      mwifiex_11n_delete_tx_ba_stream_tbl_entry() in
      mwifiex_11n_delete_all_tx_ba_stream_tbl(), it's actually safe to skip
      the spinlock release.  Let's do that.
      
      No known problems are fixed by this patch, but it could fix all sorts of
      weird problems and it should be very safe.
      Signed-off-by: default avatarDouglas Anderson <dianders@chromium.org>
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      e0b636e5
    • Brian Norris's avatar
      mwifiex: fixup error cases in mwifiex_add_virtual_intf() · 8535107a
      Brian Norris authored
      If we fail to add an interface in mwifiex_add_virtual_intf(), we might
      hit a BUG_ON() in the networking code, because we didn't tear things
      down properly. Among the problems:
      
       (a) when failing to allocate workqueues, we fail to unregister the
           netdev before calling free_netdev()
       (b) even if we do try to unregister the netdev, we're still holding the
           rtnl lock, so the device never properly unregistered; we'll be at
           state NETREG_UNREGISTERING, and then hit free_netdev()'s:
      	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
       (c) we're allocating some dependent resources (e.g., DFS workqueues)
           after we've registered the interface; this may or may not cause
           problems, but it's good practice to allocate these before registering
       (d) we're not even trying to unwind anything when mwifiex_send_cmd() or
           mwifiex_sta_init_cmd() fail
      
      To fix these issues, let's:
      
       * add a stacked set of error handling labels, to keep error handling
         consistent and properly ordered (resolving (a) and (d))
       * move the workqueue allocations before the registration (to resolve
         (c); also resolves (b) by avoiding error cases where we have to
         unregister)
      
      [Incidentally, it's pretty easy to interrupt the alloc_workqueue() in,
      e.g., the following:
      
        iw phy phy0 interface add mlan0 type station
      
      by sending it SIGTERM.]
      
      This bugfix covers commits like commit 7d652034 ("mwifiex: channel
      switch support for mwifiex"), but parts of this bug exist all the way
      back to the introduction of dynamic interface handling in commit
      93a1df48 ("mwifiex: add cfg80211 handlers add/del_virtual_intf").
      
      Cc: <stable@vger.kernel.org>
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      8535107a
    • Brian Norris's avatar
      mwifiex: pcie: de-duplicate buffer allocation code · d41bf5c1
      Brian Norris authored
      This code was duplicated as part of the PCIe FLR code added to this
      driver. Let's de-duplicate it to:
      
       * make things easier to read (mwifiex_pcie_free_buffers() now has a
         corresponding mwifiex_pcie_alloc_buffers())
       * reduce likelihood of bugs
       * make error logging equally verbose
       * save lines of code!
      
      Also drop some of the commentary that isn't really needed.
      Signed-off-by: default avatarBrian Norris <briannorris@chromium.org>
      Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
      d41bf5c1
  2. 18 May, 2017 33 commits