Skip to contentSkip to conversation

wifi: mac80211_hwsim: reject undersized HWSIM_ATTR_TX_INFO

PATCH Superseded Ibrahim Hashimov 2 participants 2 messages Jul 11 – Jul 21

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 wifi: mac80211_hwsim: reject undersized HWSIM_ATTR_TX_INFO

hwsim_tx_info_frame_received_nl() casts the HWSIM_ATTR_TX_INFO attribute payload to a struct hwsim_tx_rate * and then unconditionally walks IEEE80211_TX_MAX_RATES (4) entries of it:

	tx_attempts = (struct hwsim_tx_rate *)nla_data(
		       info->attrs[HWSIM_ATTR_TX_INFO]);
	...
	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
		txi->status.rates[i].idx = tx_attempts[i].idx;
		txi->status.rates[i].count = tx_attempts[i].count;
	}

struct hwsim_tx_rate is {s8 idx; u8 count;} __packed, i.e. 2 bytes, so the loop reads a fixed 8 bytes out of the attribute regardless of its actual length.

The attribute's policy entry only bounds the payload from above:

	[HWSIM_ATTR_TX_INFO] = { .type = NLA_BINARY,
				 .len = IEEE80211_TX_MAX_RATES *
					sizeof(struct hwsim_tx_rate)},

For NLA_BINARY, nla_policy.len is a maximum, not a minimum - nla_validate() happily accepts a shorter attribute, including a zero-length one. The HWSIM_CMD_TX_INFO_FRAME op also sets GENL_DONT_VALIDATE_STRICT, so no implicit minimum-length enforcement kicks in either. The handler itself never calls nla_len() on HWSIM_ATTR_TX_INFO before dereferencing it.

The net effect: any process that has completed HWSIM_CMD_REGISTER as the netgroup's wmediumd (CAP_NET_ADMIN at registration time, e.g. via an unprivileged user+net namespace) can send a HWSIM_CMD_TX_INFO_FRAME with a 0-length HWSIM_ATTR_TX_INFO and drive tx_attempts[0..3] to read up to 8 bytes past the end of the attribute payload.

This driver already validates NLA_BINARY payload length before use in two other handlers in this same file:

- hwsim_cloned_frame_received_nl() bounds-checks HWSIM_ATTR_FRAME before touching it:

	frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]);
	...
	if (frame_data_len < sizeof(struct ieee80211_hdr_3addr) ||
	    frame_data_len > IEEE80211_MAX_DATA_LEN)
		goto err;

- hwsim_new_radio_nl() bounds-checks HWSIM_ATTR_CIPHER_SUPPORT before use, rejecting a length that doesn't cleanly divide into whole elements:

	u32 len = nla_len(info->attrs[HWSIM_ATTR_CIPHER_SUPPORT]);
	...
	if (len % sizeof(u32)) {
		NL_SET_ERR_MSG_ATTR(...);
		return -EINVAL;
	}

hwsim_tx_info_frame_received_nl() is missing the equivalent check for HWSIM_ATTR_TX_INFO. Add a minimum-length check next to the existing attrs-presence check at the top of the function (before any pending skb is unlinked from data2->pending, to avoid holding/leaking that skb on the new error path), requiring at least IEEE80211_TX_MAX_RATES * sizeof(struct hwsim_tx_rate) bytes - matching the exact quantity the loop below unconditionally reads. A too-short attribute now takes the existing goto out; / -EINVAL path instead of reading past the attribute payload.

On a v6.19 KASAN-enabled stand, the differential reproducer confirmed the unpatched handler accepts a 0-length HWSIM_ATTR_TX_INFO and runs the fixed-count read loop to completion (ack err = 0); KASAN did not report anything here, since the resulting 8-byte over-read stays inside the same skb's kmalloc/shinfo tailroom rather than crossing a slab redzone. The check added above makes the same 0-length attribute fail before that loop runs, taking the existing goto out / -EINVAL path instead.

Fixes 7882513bacb1 ("mac80211_hwsim driver support userspace frame tx/rx")
Assisted-by AuditCode-AI:2026.07
---
 drivers/net/wireless/virtual/mac80211_hwsim_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/wireless/virtual/mac80211_hwsim_main.c +11 -0
index 0dd8a6c85953..6ef7398ffaad 100644
@@ -6340,6 +6340,17 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
 	    !info->attrs[HWSIM_ATTR_TX_INFO])
 		goto out;
 
+	/*
+	 * HWSIM_ATTR_TX_INFO is NLA_BINARY with only a maximum length in
+	 * its policy entry, so nla_validate() accepts any shorter (incl.
+	 * zero-length) payload. The read loop below unconditionally walks
+	 * IEEE80211_TX_MAX_RATES entries, so reject undersized attributes
+	 * here before any data is touched.
+	 */
+	if (nla_len(info->attrs[HWSIM_ATTR_TX_INFO]) <
+	    IEEE80211_TX_MAX_RATES * sizeof(struct hwsim_tx_rate))
+		goto out;
+
 	src = (void *)nla_data(info->attrs[HWSIM_ATTR_ADDR_TRANSMITTER]);
 	hwsim_flags = nla_get_u32(info->attrs[HWSIM_ATTR_FLAGS]);
 	ret_skb_cookie = nla_get_u64(info->attrs[HWSIM_ATTR_COOKIE]);
Signature · 1 line
2.50.1 (Apple Git-155)
maintainer
#1 lore raw
Quoted 3 lines · Ibrahim Hashimov

hwsim_tx_info_frame_received_nl() casts the HWSIM_ATTR_TX_INFO attribute payload to a struct hwsim_tx_rate * and then unconditionally walks IEEE80211_TX_MAX_RATES (4) entries of it:

[snip]

Way too much text. Please edit manually, LLMs like to be super verbose.

Quoted 3 lines
	[HWSIM_ATTR_TX_INFO] = { .type = NLA_BINARY,
				 .len = IEEE80211_TX_MAX_RATES *
					sizeof(struct hwsim_tx_rate)},

So, you found the policy, why not fix that instead?

johannes