Re: mac80211: MLO link removal frees link RX stats percpu without RCU grace

[Johannes Berg]

Hi,

> I think there is a use after free on the MLO link removal path in
> net/mac80211/sta_info.c. The link RX stats percpu buffer is freed while a
> concurrent RX softirq can still write to it. I would appreciate it if you
> could take a look.
> 
> sta_remove_link() frees the link stats and defers only the container:
> 
> 	sta_info_free_link(&alloc->info);
> 	kfree_rcu(alloc, rcu_head);
> 
> sta_info_free_link() does the free right away:
> 
> 	free_percpu(link_sta->pcpu_rx_stats);

Funny, I was looking at this code just yesterday for other reasons.

> So the container waits for a grace period but the percpu stats are
> reclaimed at once. The RX fast path runs in softirq under rcu_read_lock
> only. It resolves link_sta early and writes the percpu stats later:
> 
> 	stats = this_cpu_ptr(link_sta->pcpu_rx_stats);
> 	stats->last_signal = status->signal;
> 
> A reader that resolved link_sta before the removal NULLed it keeps the
> pointer. The container is still alive from the kfree_rcu, so the read of
> link_sta->pcpu_rx_stats works. But the percpu block it points to is
> already freed. This needs uses_rss. That is when pcpu_rx_stats is
> allocated. The trigger is an MLO link removed over the air through a
> Multi-Link Reconfiguration element.
> 
> The full STA teardown does this safely. __sta_info_destroy calls
> synchronize_net() before sta_info_free() frees the deflink stats. The MLO
> link removal path has no such barrier. That path was added in
> cb71f1d136a6 ("wifi: mac80211: add sta link addition/removal").
> 
> I do not have WiFi 7 hardware. This is from reading the code. A small test
> that frees the stats buffer and writes it through the live container trips
> KASAN with a slab use after free.
> 
> Does this look like a real use after free to you?

The above analysis doesn't look _incorrect_ to me, though I think
between uses_rss, sta_pre_rcu remove and how drivers tend to work
(though I can mostly speak for iwlwifi, and ath1[12]k also have RSS),
it's going to be pretty fairly difficult (if not impossible?) to hit in
practice.

The only (relevant, real RCU) reader is rx.c, which requires fast_rx too
(not all that relevant I think). During STA removal, both are removed
together and we always have to have both, so I think it's mostly OK.
When the driver passes the STA it could still be an issue though, I
think?

During link switch it could be more of an issue, but then it's MLO and
we require the driver to tell us where it's receiving, and it wouldn't
receive on a link that was just disabled, so I don't think you'll hit it
in practice here either.

> Is the right fix to
> defer the percpu free to RCU, like the container already is? I am happy to
> send a patch once you confirm.

I think it'd be better to combine them and free the link with data after
RCU, rather than bumping each to RCU separately?

johannes