[bug report] Potential order bug in 'net/xfrm/xfrm_state.c', primarily in 'xfrm_state_walk_done()'

[bug report] Potential order bug in 'net/xfrm/xfrm_state.c', primarily in 'xfrm_state_walk_done()'

[Ginger]

Dear Linux kernel maintainers,

My research-based static analyzer found a potential orderbug within
the 'net/xfrm' subsystem, more specifically, in
'net/xfrm/xfrm_state.c'.

Kernel version: long-term kernel v6.18.9

Potential concurrent triggering executions:
T0:
xfrm_state_walk_done
    --> kfree(walk->filter); [t0]
    --> list_del(&walk->all); [t3]

T1:
xfrm_state_walk
   --> list_for_each_entry_from(x, &net->xfrm.state_all, all) [t1]
   --> __xfrm_state_filter_match(state, walk->filter) [t2]

In T0, 'walk->filter' is free before the parent 'walk' gets removed
from the global list. Thus, it is possible for T1 to still get the
same 'walk' by iterating through the list linked via 'walk->all' and
access the 'filter' field, which has already been freed.
The concurrent buggy order is t0 -> t1 -> t2 -> t3.

Thank you for your time and consideration.

Sincerely,
Ginger

Re: [bug report] Potential order bug in 'net/xfrm/xfrm_state.c', primarily in 'xfrm_state_walk_done()'

[Florian Westphal]

Ginger <ginger.jzllee@gmail.com> wrote:
> Potential concurrent triggering executions:
> T0:
> xfrm_state_walk_done
>     --> kfree(walk->filter); [t0]
>     --> list_del(&walk->all); [t3]

list_del() uses same spinlock as iterator.

> T1:
> xfrm_state_walk

2652 int xfrm_state_walk(struct net *net, struct xfrm_state_walk *walk,
2653                     int (*func)(struct xfrm_state *, int, void*),
2654                     void *data)
2655 {
[..]
2663         spin_lock_bh(&net->xfrm.xfrm_state_lock);
2668         list_for_each_entry_from(x, &net->xfrm.state_all, all) {
2669                 if (x->state == XFRM_STATE_DEAD)
2670                         continue;

... and walker has STATE_DEAD, no? So I don't see how UaF is possible.

Even if parallel invocation (pfkey+netlink?) is possible, then we have:

T0: walk_done() -> free filter -> blocks on spinlock for list_del
T1: list_for_each ... -> walker is valid memory, checks x->state -> SKIP
to next entry

(or list_del already finished, but then _walk() is blocked on
spinlock).