Re: iwlwifi: question on failure path in iwl_txq_gen2

[Ben Greear <greearb@xxxxxxxxxxxxxxx>]

On 10/11/24 11:51, Ben Greear wrote:
> Hello,
>
> We've been trying to track down what may be a double-free or similar issue
> in iwlwifi.
>
> While looking through the transmit path, I believe that an skb reference
> might can be use-after-freed if tfd cannot be allocated.

Nulling out the skb doesn't fix the use-after-free, or at least not all of it.

And I realize now that the cmd-index doesn't need to be messed with in
the error branch.

I'll post a patch to null out the pointers when I get a chance, as I think
it at least makes things a bit cleaner.

Thanks,
Ben

> I am guessing we should NULL out the pointers in that case.  Should we also
> decrement the cmd_index?
>
>
> int iwl_txq_gen2_tx(struct iwl_trans *trans, struct sk_buff *skb,
>              struct iwl_device_tx_cmd *dev_cmd, int txq_id)
> {
>      struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
>      struct iwl_cmd_meta *out_meta;
>      struct iwl_txq *txq = trans_pcie->txqs.txq[txq_id];
>      u16 cmd_len;
>      int idx;
>      void *tfd;
>
>      if (WARN_ONCE(txq_id >= IWL_MAX_TVQM_QUEUES,
>                "queue %d out of range", txq_id))
>          return -EINVAL;
>
>      if (WARN_ONCE(!test_bit(txq_id, trans_pcie->txqs.queue_used),
>                "TX on unused queue %d\n", txq_id))
>          return -EINVAL;
>
>      if (skb_is_nonlinear(skb) &&
>          skb_shinfo(skb)->nr_frags > IWL_TRANS_PCIE_MAX_FRAGS(trans_pcie) &&
>          __skb_linearize(skb))
>          return -ENOMEM;
>
>      spin_lock(&txq->lock);
>
>      if (iwl_txq_space(trans, txq) < txq->high_mark) {
>          iwl_txq_stop(trans, txq);
>
>          /* don't put the packet on the ring, if there is no room */
>          if (unlikely(iwl_txq_space(trans, txq) < 3)) {
>              struct iwl_device_tx_cmd **dev_cmd_ptr;
>
>              dev_cmd_ptr = (void *)((u8 *)skb->cb +
>                             trans_pcie->txqs.dev_cmd_offs);
>
>              *dev_cmd_ptr = dev_cmd;
>              __skb_queue_tail(&txq->overflow_q, skb);
>              spin_unlock(&txq->lock);
>              return 0;
>          }
>      }
>
>      idx = iwl_txq_get_cmd_index(txq, txq->write_ptr);
>
>      /* Set up driver data for this TFD */
>      txq->entries[idx].skb = skb;
>      txq->entries[idx].cmd = dev_cmd;
>
>      dev_cmd->hdr.sequence =
>          cpu_to_le16((u16)(QUEUE_TO_SEQ(txq_id) |
>                  INDEX_TO_SEQ(idx)));
>
>      /* Set up first empty entry in queue's array of Tx/cmd buffers */
>      out_meta = &txq->entries[idx].meta;
>      memset(out_meta, 0, sizeof(*out_meta));
>
>      tfd = iwl_txq_gen2_build_tfd(trans, txq, dev_cmd, skb, out_meta);
>      if (!tfd) {
>          txq->entries[idx].skb = NULL;
>          txq->entries[idx].cmd = NULL;
>    ####  Upstream code does not have these two lines above ####
>          spin_unlock(&txq->lock);
>          return -1;
>      }
>
> Thanks,
> Ben