linux-kernel - [RFC] wifi: mwifiex: Asking for some light on this, please :)

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>] [thread-next>] [day] [month] [year] [list]

Message-ID: <6e3fddfe-2d31-72fa-ac2b-3b7fa4e6fe08@embeddedor.com>
Date:   Tue, 15 Aug 2023 18:52:06 -0600
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Brian Norris <briannorris@...omium.org>,
        Kalle Valo <kvalo@...nel.org>
Cc:     linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-hardening@...r.kernel.org
Subject: [RFC] wifi: mwifiex: Asking for some light on this, please :)

Hi all,

While working on flex-array transformations I ran into the following
implementation:

drivers/net/wireless/marvell/mwifiex/fw.h:775:
struct mwifiex_ie_types_rxba_sync {
	struct mwifiex_ie_types_header header;
	u8 mac[ETH_ALEN];
	u8 tid;
	u8 reserved;
	__le16 seq_num;
	__le16 bitmap_len;
	u8 bitmap[1];
} __packed;

`bitmap` is currently being used as a fake-flex array and we should
transform it into a proper flexible-array member.

However, while doing that, I noticed something in the following function
that's not clear to me and I wanted to ask you for feedback:

drivers/net/wireless/marvell/mwifiex/11n_rxreorder.c:907:
void mwifiex_11n_rxba_sync_event(struct mwifiex_private *priv,
				 u8 *event_buf, u16 len)
{
	struct mwifiex_ie_types_rxba_sync *tlv_rxba = (void *)event_buf;
	u16 tlv_type, tlv_len;
	struct mwifiex_rx_reorder_tbl *rx_reor_tbl_ptr;
	u8 i, j;
	u16 seq_num, tlv_seq_num, tlv_bitmap_len;
	int tlv_buf_left = len;
	int ret;
	u8 *tmp;

	mwifiex_dbg_dump(priv->adapter, EVT_D, "RXBA_SYNC event:",
			 event_buf, len);
	while (tlv_buf_left >= sizeof(*tlv_rxba)) {
		tlv_type = le16_to_cpu(tlv_rxba->header.type);
		tlv_len  = le16_to_cpu(tlv_rxba->header.len);
		if (tlv_type != TLV_TYPE_RXBA_SYNC) {
			mwifiex_dbg(priv->adapter, ERROR,
				    "Wrong TLV id=0x%x\n", tlv_type);
			return;
		}

		tlv_seq_num = le16_to_cpu(tlv_rxba->seq_num);
		tlv_bitmap_len = le16_to_cpu(tlv_rxba->bitmap_len);
		mwifiex_dbg(priv->adapter, INFO,
			    "%pM tid=%d seq_num=%d bitmap_len=%d\n",
			    tlv_rxba->mac, tlv_rxba->tid, tlv_seq_num,
			    tlv_bitmap_len);

		rx_reor_tbl_ptr =
			mwifiex_11n_get_rx_reorder_tbl(priv, tlv_rxba->tid,
						       tlv_rxba->mac);
		if (!rx_reor_tbl_ptr) {
			mwifiex_dbg(priv->adapter, ERROR,
				    "Can not find rx_reorder_tbl!");
			return;
		}

		for (i = 0; i < tlv_bitmap_len; i++) {
			for (j = 0 ; j < 8; j++) {
				if (tlv_rxba->bitmap[i] & (1 << j)) {
					seq_num = (MAX_TID_VALUE - 1) &
						(tlv_seq_num + i * 8 + j);

					mwifiex_dbg(priv->adapter, ERROR,
						    "drop packet,seq=%d\n",
						    seq_num);

					ret = mwifiex_11n_rx_reorder_pkt
					(priv, seq_num, tlv_rxba->tid,
					 tlv_rxba->mac, 0, NULL);

					if (ret)
						mwifiex_dbg(priv->adapter,
							    ERROR,
							    "Fail to drop packet");
				}
			}
		}

		tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);
		tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
		
What's the relation between tlv_len, sizeof(*tlv_rxba) and tlv_bitmap_len?

Isn't `sizeof(*tlv_rxba) + tlv_len` and `tlv_len + sizeof(*tlv_rxba)`
double-counting some fields in `struct mwifiex_ie_types_rxba_sync`?

Shouldn't this be something like this, instead (before the flex-array
transformation, of course):

-               tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_len);
-               tmp = (u8 *)tlv_rxba + tlv_len + sizeof(*tlv_rxba);
+               tlv_buf_left -= (sizeof(*tlv_rxba) + tlv_bitmap_len - 1);
+               tmp = (u8 *)tlv_rxba + tlv_bitmap_len + sizeof(*tlv_rxba - 1);


		tlv_rxba = (struct mwifiex_ie_types_rxba_sync *)tmp;
	}
}

Thanks in advance for any feedback!

--
Gustavo