netdev - re: rtw88: pci: define a mask for TX/RX BD indexes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Wed, 24 Jun 2020 11:45:48 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Yan-Hsuan Chuang <yhchuang@...ltek.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        netdev@...r.kernel.org
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: rtw88: pci: define a mask for TX/RX BD indexes

Hi,

static analysis with Coverity has detected a out of range write issue on
the assigment rx_ring->buf[i] = skb in rtw_pci_init_rx_ring. The commit
in question is:

commit a5697a65ecd109ce7f8e3661b89a5dffae73b512
Author: Yan-Hsuan Chuang <yhchuang@...ltek.com>
Date:   Thu Mar 12 16:08:51 2020 +0800

    rtw88: pci: define a mask for TX/RX BD indexes

Analysis is as follows:


    2. Condition len > 4095UL /* (int)sizeof (struct
rtw_pci_init_rx_ring::[unnamed type]) + (~0UL - (1UL << 0) + 1 & (~0UL
>> 64 - 1 - 11)) */, taking false branch.
    3. cond_at_most: Checking len > 4095UL implies that len may be up to
4095 on the false branch.

267        if (len > TRX_BD_IDX_MASK) {
268                rtw_err(rtwdev, "len %d exceeds maximum RX
entries\n", len);
269                return -EINVAL;
270        }
271
272        head = pci_zalloc_consistent(pdev, ring_sz, &dma);

   4. Condition !head, taking false branch.
273        if (!head) {
274                rtw_err(rtwdev, "failed to allocate rx ring\n");
275                return -ENOMEM;
276        }
277        rx_ring->r.head = head;
278

   5. Condition i < len, taking true branch.
   9. Condition i < len, taking true branch.
   10. cond_at_most: Checking i < len implies that i may be up to 4094
on the true branch.
279        for (i = 0; i < len; i++) {
280                skb = dev_alloc_skb(buf_sz);
   6. Condition !skb, taking false branch.
   11. Condition !skb, taking false branch.
281                if (!skb) {
282                        allocated = i;
283                        ret = -ENOMEM;
284                        goto err_out;
285                }
286
287                memset(skb->data, 0, buf_sz);
   CID (#1 of 1): Out-of-bounds write (OVERRUN)
   12. overrun-local: Overrunning array rx_ring->buf of 512 8-byte
elements at element index 4094 (byte offset 32759) using index i (which
evaluates to 4094).
288                rx_ring->buf[i] = skb;

The rx_ring->buf array is declared in
/drivers/net/wireless/realtek/rtw88/pci.h as:

struct sk_buff *buf[RTK_MAX_RX_DESC_NUM];

where RTK_MAX_RX_DESC_NUM is 512.

There are two places where the length check is incorrect:

rtw_pci_init_tx_ring():

       if (len > TRX_BD_IDX_MASK) {
               rtw_err(rtwdev, "len %d exceeds maximum TX entries\n", len);
               return -EINVAL;
       }

and rtw_pci_init_rx_ring():

       if (len > TRX_BD_IDX_MASK) {
               rtw_err(rtwdev, "len %d exceeds maximum RX entries\n", len);
               return -EINVAL;
       }

The length check should not be against TRX_BD_IDX_MASK but against the
number of elements in rx_ring->buf[].  Also the rx_ring->buf[] is only
512 items. Not sure if one or both of those are errors.

Colin