Re: [PATCH net] ipv4: fib_trie: prevent speculative out-of-bounds child access

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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).

- [High] The Spectre v1 mitigation in fib_table_lookup()
  (net/ipv4/fib_trie.c) is incomplete: the raw index is copied into…

Pre-existing issues:
- [High] fib_find_node() in net/ipv4/fib_trie.c has the same Spectre v1
  gadget the patch fixes in fib_table_lookup(), and the patch leaves it…

--

ipv4: fib_trie: prevent speculative out-of-bounds child access

This patch adds array_index_nospec() in fib_table_lookup(). It clamps the
child index before get_child_rcu() reads n->tnode[]. The goal is to stop a
mispredicted bounds check from loading past the child array with an
index derived from flp->daddr.

> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 248514dce0cdd..bd8a368c55026 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c

[ ... ]

> @@ -1474,6 +1475,7 @@ int fib_table_lookup(struct fib_table *tb, const struct flowi4 *flp,
>  			cindex = index;
>  		}
>  
> +		index = array_index_nospec(index, 1ul << n->bits);
>  		n = get_child_rcu(n, index);
>  		if (unlikely(!n))
>  			goto backtrace;

[Severity: High]
Does the clamp run too late to cover cindex? In the Step 1 loop, the raw
index is saved into cindex before the new array_index_nospec() line runs:

		if (n->slen > n->pos) {
			pn = n;
			cindex = index;
		}

		index = array_index_nospec(index, 1ul << n->bits);

Suppose the "index >= (1ul << n->bits)" check is mispredicted for an
out-of-bounds index and n->slen > n->pos. Then cindex and pn keep the
unmasked value. The clamped index then loads tnode[0].

Execution can reach backtrace with those values still in place in two
ways: tnode[0] is NULL, or Step 2 later fails prefix_mismatch(). cindex
is non-zero, so the while (!cindex) loop is skipped, and this runs:

			cindex &= cindex - 1;

			/* grab pointer for next child node */
			cptr = &pn->tnode[cindex];

That only clears the lowest set bit, so cindex stays out of bounds and
its high bits are still the ones the attacker chose. For example, 0x10003
becomes 0x10002 with bits = 4.

The slot is then loaded and used as a node pointer at the top of Step 2:

		while ((n = rcu_dereference(*cptr)) == NULL) {
		...
		if (unlikely(prefix_mismatch(key, n)) || (n->slen == n->pos))

Isn't this the same "load past the child array, then dereference the
value as a node pointer" sequence the commit message describes?

Would moving the array_index_nospec() line above the
"if (n->slen > n->pos)" block clamp both cindex and the index passed to
get_child_rcu()?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch.
fib_find_node() in the same file seems to have the same gadget, and this
patch leaves it as is:

net/ipv4/fib_trie.c:fib_find_node() {
	do {
		pn = n;
		n = get_child_rcu(n, index);

		if (!n)
			break;

		index = get_cindex(key, n);
		...
		if (index >= (1ul << n->bits)) {
			n = NULL;
			break;
		}

		/* keep searching until we find a perfect match leaf or NULL */
	} while (IS_TNODE(n));
	...
}

If that bounds check is mispredicted, IS_TNODE(n) is still true. The next
pass then calls get_child_rcu(n, index) with the unmasked index and
dereferences the loaded value through get_cindex().

The key is ntohl(cfg->fc_dst), set in fib_table_insert() and
fib_table_delete(). Both call fib_find_node(t, &tp, key), and they are
reached from RTM_NEWROUTE and RTM_DELROUTE. That needs CAP_NET_ADMIN over
the netns, which an unprivileged user can often get by creating a user
namespace.

The subject line says "prevent speculative out-of-bounds child access".
Should fib_find_node() get the same array_index_nospec() clamp before
its child load?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-fib-trie-nospec-v1-1-fa18726d485e%40nvidia.com