Re: [PATCH net v2] mac802154: fix data race and NULL deref on local->assoc_dev

[Kaiwen Shi]

Hi Miquèl,

> Can you justify the choice of a spinlock vs. mutex here? This is an open
> question, not a request for changes.

Both users run in process context (the response handler runs from a
workqueue), so either lock is legal. I used a spinlock because the
critical sections are tiny and never sleep: caching a __le64 address and
flipping the associating bit, or validating a frame, storing a short
address plus a status byte, and completing the waiter. The lock is never
held across wait_for_completion_killable_timeout(), so a sleeping lock
would add nothing.

I also looked at reusing wpan_dev->association_lock rather than adding a
lock. It is not held on either of the two paths involved here
(mac802154_perform_association() and mac802154_process_association_resp()),
and pan.c has several lockdep_assert_held() on it, so widening its scope
to the response path looked like a bigger change than this fix should
carry. Happy to revisit if you would rather see one lock covering both.

> Shouldn't we also make sure that accessing assoc_status/assoc_addr is
> serialized? Typically, I believe the IS_ASSOCIATING bit should be
> cleared earlier in mac802154_perform_association(), just after the
> wait_for_completion call returns ...

You are right. The success path reads assoc_status/assoc_addr after the
wait returns but before clearing the bit, so a second (e.g. malicious)
response accepted in that window can still overwrite them mid-read.

With a debug delay inserted between the wait returning and the result
being read, and a peer that keeps answering the same request with an
increasing short address, the association path reads 0x08f0 right after
the wait and 0x0903 after the delay; 38 further responses were accepted
in between. v3 clears the associating bit and snapshots both fields under
assoc_lock right after the wait returns, and in the same test no response
is accepted in that window and both reads return the same address.

This is also what the scan path already does: mac802154_scan_cleanup_locked()
clears IEEE802154_IS_SCANNING first, "to prevent any further use of the
scan request", and only then replaces and frees the request.

A v3 follows.

Thanks,
Kaiwen