Re: [PATCH net] ipv6: fix fib6 walker UAF on NLM_F_REPLACE

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] fib6_walkers_skip_rt() (net/ipv6/ip6_fib.c:95-113) is
  documented as moving walkers that rest on @rt to the next route.
- [Medium] In the NLM_F_REPLACE branch of fib6_add_rt2node()
  (net/ipv6/ip6_fib.c:1338-1342) the new route rt takes over iter's slot…

Pre-existing issues:
- [Medium] When a dump resumes and the sernum has changed,
  fib6_dump_table() (net/ipv6/ip6_fib.c:632-638) writes w->state =…

--

ipv6: fix fib6 walker UAF on NLM_F_REPLACE

This patch moves the walker adjustment out of fib6_del_route() into a new
helper, fib6_walkers_skip_rt(). The helper is also called from the
NLM_F_REPLACE branch of fib6_add_rt2node() for the replaced route and for
each removed ECMP sibling. The goal is to keep an RTM_GETROUTE dump from
resuming on a freed fib6_info when the fn_sernum check is bypassed.

> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 9ff761962b45c..a5699809fe049 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -89,6 +89,29 @@ static void fib6_walker_unlink(struct net *net, struct fib6_walker *w)

[ ... ]

> +/* Move the walkers resting on @rt to the next route, or up the tree if
> + * @rt is the last one.  Called with tb6_lock held.
> + */
> +static void fib6_walkers_skip_rt(struct net *net, struct fib6_info *rt)
> +{

[ ... ]

> +	read_lock(&net->ipv6.fib6_walker_lock);
> +	FOR_WALKERS(net, w) {
> +		if (w->state == FWS_C && w->leaf == rt) {
> +			pr_debug("walker %p adjusted by unlink of %p\n", w, rt);
> +			w->leaf = next;

[Severity: Medium]
Should w->skip_in_node be reset here too?

This moves the walker to a different route, but w->skip_in_node still
holds the number of entries already sent for the old route.
fib6_dump_node() keeps that count per w->leaf:

net/ipv6/ip6_fib.c:fib6_dump_node() {
    ...
		res = rt6_dump_route(rt, w->args, w->skip_in_node);
		if (res >= 0) {
			/* Frame is full, suspend walking */
			w->leaf = rt;
			...
			w->skip_in_node += res;
    ...
}

fib6_dump_table() clears it only on a sernum mismatch. When the sernum
matches, it does just this:

		} else
			w->skip = 0;

Suppose the dump paused partway through the exceptions of route R
(skip_in_node >= 1), and R is then removed through this helper. The
resumed dump calls rt6_dump_route() on R->fib6_next with the stale skip:

net/ipv6/route.c:rt6_dump_route() {
    ...
	if (filter->dump_routes) {
		if (skip) {
			skip--;
		} else {
			if (rt6_fill_node(net, arg->skb, rt, NULL, NULL, NULL,
    ...
}

That drops the next route's own RTM_NEWROUTE message, and the leftover
skip hides its first exceptions. In the FWS_U case, the first route of a
later node is dropped instead. NLM_F_DUMP_INTR is not set in either case.

With this patch, the same thing can happen for a replaced route and for
each removed ECMP sibling in fib6_add_rt2node(). That needs the resume to
go through the fn_sernum == -1 collision described in the commit message.

fib6_del() does not bump the root sernum, so the delete path behaved this
way on a normal resume even before the code was moved out of
fib6_del_route().

Would adding w->skip_in_node = 0 where w->leaf is reassigned make the
helper match its comment?

> +			if (!next)
> +				w->state = FWS_U;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but can
this store race with the lockless walker reset in fib6_dump_table()?

When the sernum has changed, fib6_dump_table() resets the walker before it
takes tb6_lock. The walker is still linked on net->ipv6.fib6_walkers at
that point:

net/ipv6/ip6_fib.c:fib6_dump_table() {
    ...
		if (cb->args[5] != sernum) {
			/* Begin at the root if the tree changed */
			cb->args[5] = sernum;
			w->state = FWS_INIT;
			w->node = w->root;
			w->skip = w->count;
			w->skip_in_node = 0;
		} else
			w->skip = 0;

		spin_lock_bh(&table->tb6_lock);
    ...
}

At the same time, this helper holds only tb6_lock and
read_lock(fib6_walker_lock), and it can still see w->state == FWS_C and
w->leaf == rt.

If its FWS_U store lands after the FWS_INIT store, fib6_walk_continue()
starts at w->root in state FWS_U:

		case FWS_U:
			if (fn == w->root)
				return 0;

The rest of the table is then silently left out of the dump, with no
NLM_F_DUMP_INTR.

The same race already existed with fib6_del_route() as the writer. This
patch adds the replace paths in fib6_add_rt2node() as more writers. I don't
see a use-after-free here, because FWS_INIT and FWS_U do not dereference
w->leaf.

[ ... ]

> @@ -1315,6 +1338,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct fib6_info *rt,
>  		fib6_info_hold(rt);
>  		rcu_assign_pointer(rt->fib6_node, fn);
>  		rt->fib6_next = iter->fib6_next;
> +		fib6_walkers_skip_rt(info->nl_net, iter);
>  		rcu_assign_pointer(*ins, rt);

[Severity: Medium]
Can a resumed dump skip the replacement route rt here?

In this branch, rt takes over iter's slot in the leaf list. The helper
treats that as a deletion. It moves walkers on iter to iter->fib6_next,
which comes after rt, or sets FWS_U if iter was the last route in the
node.

fib6_dump_node() leaves w->leaf on the route it hadn't finished. In the
common case, the pause happened at rt6_fill_node() of iter itself with
skip_in_node == 0, so nothing from iter has been sent yet.

Suppose the dump then resumes on the sernum-match path (the
fn_sernum == -1 case from the commit message). fib6_dump_node() starts at
w->leaf and never visits rt. The dump then contains neither the old route
nor its replacement for that prefix and metric, and NLM_F_DUMP_INTR is not
set.

On the sernum-mismatch path, the node is walked again from fn->leaf and rt
is dumped, so the two resume paths give different results.

The ECMP sibling loop further down looks fine, since those routes really
are removed.

For the replaced slot itself, would pointing walkers on iter at rt (and
clearing skip_in_node) keep the replacement in the dump?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009225142.47005-1-cenzhang%40linux.microsoft.com