re: mwl8k: differentiate between WMM queues and AMPDU queues

re: mwl8k: differentiate between WMM queues and AMPDU queues

[Dan Carpenter]

Hello Brian Cavagnolo,

The patch e600707b021e: "mwl8k: differentiate between WMM queues and
AMPDU queues" from Mar 17, 2011, leads to the following
static checker warning: "drivers/net/wireless/mwl8k.c:2456
mwl8k_cmd_get_hw_spec_sta()
	 error: buffer overflow 'cmd->tx_queue_ptrs' 4 <= 11"

drivers/net/wireless/mwl8k.c
    98  #define MWL8K_RX_QUEUES         1
    99  #define MWL8K_TX_WMM_QUEUES     4
   100  #define MWL8K_MAX_AMPDU_QUEUES  8
   101  #define MWL8K_MAX_TX_QUEUES     (MWL8K_TX_WMM_QUEUES + MWL8K_MAX_AMPDU_QUEUES)
   102  #define mwl8k_tx_queues(priv)   (MWL8K_TX_WMM_QUEUES + (priv)->num_ampdu_queues)
   103  

In theory mwl8k_tx_queues() is 4-12 as we can see below.

  2539                  priv->ap_macids_supported = 0x000000ff;
  2540                  priv->sta_macids_supported = 0x00000100;
  2541                  priv->num_ampdu_queues = le32_to_cpu(cmd->num_of_ampdu_queues);
  2542                  if (priv->num_ampdu_queues > MWL8K_MAX_AMPDU_QUEUES) {
  2543                          wiphy_warn(hw->wiphy, "fw reported %d ampdu queues"
  2544                                     " but we only support %d.\n",
  2545                                     priv->num_ampdu_queues,
  2546                                     MWL8K_MAX_AMPDU_QUEUES);
  2547                          priv->num_ampdu_queues = MWL8K_MAX_AMPDU_QUEUES;
  2548                  }

->num_ampdu_queues can go up to 8.


  2454          cmd->num_tx_queues = cpu_to_le32(mwl8k_tx_queues(priv));
  2455          for (i = 0; i < mwl8k_tx_queues(priv); i++)
  2456                  cmd->tx_queue_ptrs[i] = cpu_to_le32(priv->txq[i].txd_dma);

This code is super confusing because it is copied almost verbatim in two
places.  It took me a long time to figure that out.  Anyway,
->tx_queue_ptrs[] is an array with 4 elements.  If ->num_ampdu_queues is
anything besides zero then we are corrupting memory.  It's entirely
possible that ->num_ampdu_queues is zero here but I got a headache
trying to sort out the duplicate copies of this code.

A cleaner way to write this would be:

		for (i = 0; i < MWL8K_TX_WMM_QUEUES; i++)

or

		for (i = 0; i < ARRAY_SIZE(cmd->tx_queue_ptrs); i++)

That we don't have to follow all the callers and verify that adding
->num_ampdu_queues is a no-op.

priv->txq[] is 12 element array.

  2457          cmd->num_tx_desc_per_queue = cpu_to_le32(MWL8K_TX_DESCS);
  2458          cmd->total_rxd = cpu_to_le32(MWL8K_RX_DESCS);

regards,
dan carpenter