Skip to contentSkip to conversation

wifi: mac80211: Replace strncpy() with strscpy_pad() in drv_switch_vif_chanctx tracepoint

PATCH In review Kees Cook 2 participants 3 messages Mar 23 mac80211

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 wifi: mac80211: Replace strncpy() with strscpy_pad() in drv_switch_vif_chanctx tracepoint

Replace the deprecated[1] strncpy() with strscpy_pad() for copying the interface name into a tracepoint entry.

The source "sdata->name" is a NUL-terminated char[IFNAMSIZ] buffer populated via NUL-guaranteeing paths: strscpy() in ieee80211_if_add(), snprintf() in ieee80211_add_virtual_monitor(), or memcpy() from ndev->name in ieee80211_if_add() and netdev_notify() (net/mac80211/iface.c). In the memcpy() cases, the source ndev->name is itself always NUL-terminated (populated via snprintf() or strscpy() in __dev_alloc_name() and dev_prep_valid_name() in net/core/dev.c).

The destination "local_vifs[i].vif.vif_name" is a char[IFNAMSIZ] field in struct trace_vif_entry, stored in a __dynamic_array within the trace ring buffer. Since ring buffer entries are not zeroed on allocation, strscpy_pad() is used to zero-fill trailing bytes and prevent exposing stale ring buffer contents to userspace readers of tracefs.

No behavioral change: since interface names are always at most 15 characters plus a NUL terminator, strscpy_pad() with size IFNAMSIZ (16) produces identical output to the original strncpy().

---
 net/mac80211/trace.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
net/mac80211/trace.h +2 -3
index c04d4547e8f4..3b139fb67d1b 100644
@@ -1796,9 +1796,8 @@ TRACE_EVENT(drv_switch_vif_chanctx,
 				SWITCH_ENTRY_ASSIGN(vif.vif_type, vif->type);
 				SWITCH_ENTRY_ASSIGN(vif.p2p, vif->p2p);
 				SWITCH_ENTRY_ASSIGN(link_id, link_conf->link_id);
-				strncpy(local_vifs[i].vif.vif_name,
-					sdata->name,
-					sizeof(local_vifs[i].vif.vif_name));
+				strscpy_pad(local_vifs[i].vif.vif_name,
+					    sdata->name);
 				SWITCH_ENTRY_ASSIGN(old_chandef.control_freq,
 						old_ctx->def.chan->center_freq);
 				SWITCH_ENTRY_ASSIGN(old_chandef.freq_offset,
Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 5 lines · Kees Cook

ring buffer. Since ring buffer entries are not zeroed on allocation,

No behavioral change: since interface names are always at most 15 characters plus a NUL terminator, strscpy_pad() with size IFNAMSIZ (16) produces identical output to the original strncpy().

This doesn't add up - if it's not zero-filled on allocation, then it *is* a (desired) behaviour change?

johannes

maintainer
#2 lore raw
Quoted 9 lines · Johannes Berg
Quoted 5 lines · Kees Cook

ring buffer. Since ring buffer entries are not zeroed on allocation,

No behavioral change: since interface names are always at most 15 characters plus a NUL terminator, strscpy_pad() with size IFNAMSIZ (16) produces identical output to the original strncpy().

This doesn't add up - if it's not zero-filled on allocation, then it *is* a (desired) behaviour change?

Err, no, never mind, it's str*n*cpy() now...

johannes