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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] The patch's Spectre-v1 hardening leaves out
  fib_trie_get_next() in net/ipv4/fib_trie.c, which is the depth-first…

--

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

This patch adds array_index_nospec() sanitization of the child index
before get_child_rcu() in fib_table_lookup(), fib_find_node() and both
loops of leaf_walk_rcu(). It closes a Spectre-v1 gadget where a
mispredicted bounds check lets an out-of-bounds tnode[] slot be
transiently used as a node pointer.

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

[ ... ]

> @@ -1787,6 +1794,8 @@ static struct key_vector *leaf_walk_rcu(struct key_vector **tn, t_key key)
>  			continue;
>  		}
>  
> +		cindex = array_index_nospec(cindex, 1ul << pn->bits);
> +
>  		/* grab the next available node */
>  		n = get_child_rcu(pn, cindex++);
>  		if (!n)

[Severity: Medium]

The commit message says "leaf_walk_rcu() has a similar checked cindex
before get_child_rcu(), and the traversed state can be influenced by
userspace actions". Should fib_trie_get_next() get the same treatment?

It has the same shape as this sibling walk in leaf_walk_rcu(). The only
bound is the loop condition:

net/ipv4/fib_trie.c:fib_trie_get_next() {
    ...
	while (!IS_TRIE(pn)) {
		while (cindex < child_length(pn)) {
			struct key_vector *n = get_child_rcu(pn, cindex++);

			if (!n)
				continue;

			if (IS_LEAF(n)) {
    ...
		/* Current node exhausted, pop back up */
		pkey = pn->key;
		pn = node_parent_rcu(pn);
		cindex = get_index(pkey, pn) + 1;
    ...
}

Suppose the cindex < child_length(pn) check is mispredicted on the last
iteration. Could pn->tnode[child_length(pn)] then be loaded transiently
and dereferenced as a node pointer by IS_LEAF(n)? This hunk sanitizes
the same one-past-the-end case in leaf_walk_rcu().

fib_trie_get_next() is reachable from the 0444 /proc/net/fib_trie and
/proc/net/fib_triestat files, via fib_trie_seq_ops and
fib_triestat_seq_show()->trie_collect_stats(). Any local user can drive
it, and an unprivileged user and net namespace can shape the trie
contents.

Here the out-of-bounds index is always one past the end, so an attacker
can't choose it. That makes exploitation speculative. Would something
like this keep the hardening consistent?

		while (cindex < child_length(pn)) {
			struct key_vector *n;

			cindex = array_index_nospec(cindex, child_length(pn));
			n = get_child_rcu(pn, cindex++);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010-fib-trie-nospec-v2-1-30319b4db8d0%40nvidia.com