Re: [PATCH nf 1/2] netfilter: nft_fib_ipv6: handle routes via external nexthop

[Phil Sutter <phil@nwl.cc>]

Hi,

On Tue, May 19, 2026 at 12:14:30PM +0800, Jiayuan Chen wrote:
> fib6_info has a union:
> 
>     union {
>         struct list_head fib6_siblings;
>         struct list_head nh_list;
>     };
> 
> Old-style multipath (ip -6 route add ... nexthop ... nexthop ...) uses
> fib6_siblings.  External nexthop (ip -6 route add ... nhid N) uses
> nh_list, linked into &nh->f6i_list.
> 
> nft_fib6_info_nh_uses_dev() blindly walks &rt->fib6_siblings, causing
> an OOB read past the struct nexthop slab when rt->nh is set:
> 
>   ==================================================================
>   BUG: KASAN: slab-out-of-bounds in nft_fib6_eval+0x1362/0x16c0
>   Read of size 8 at addr ffff888103a099d0 by task ping/386
> 
>   CPU: 2 UID: 0 PID: 386 Comm: ping Not tainted 7.1.0-rc3+ #251 PREEMPT
>   Call Trace:
>    <IRQ>
>    dump_stack_lvl+0x76/0xa0
>    print_report+0xd1/0x5f0
>    kasan_report+0xe7/0x130
>    __asan_report_load8_noabort+0x14/0x30
>    nft_fib6_eval+0x1362/0x16c0
>    nft_do_chain+0x279/0x18c0
>    nft_do_chain_ipv6+0x1a8/0x230
>    nf_hook_slow+0xad/0x200
>    ipv6_rcv+0x152/0x380
>    __netif_receive_skb_one_core+0x118/0x1c0
>   ==================================================================
> 
> Branch by route shape: when rt->nh is set, walk via
> nexthop_for_each_fib6_nh() (also covers nh groups, which the original
> code missed); otherwise walk fib6_siblings, guarded by fib6_nsiblings.
> 
> Fixes: 1c32b24c234b ("netfilter: nft_fib_ipv6: switch to fib6_lookup")
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
>  net/ipv6/netfilter/nft_fib_ipv6.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
> index 8b2dba88ee96..a44919f46de9 100644
> --- a/net/ipv6/netfilter/nft_fib_ipv6.c
> +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
> @@ -160,16 +160,32 @@ static bool nft_fib6_info_nh_dev_match(const struct net_device *nh_dev,
>  	       l3mdev_master_ifindex_rcu(nh_dev) == dev->ifindex;
>  }
>  
> +static int nft_fib6_nh_match_dev_cb(struct fib6_nh *nh, void *arg)
> +{
> +	const struct net_device *dev = arg;
> +
> +	return nft_fib6_info_nh_dev_match(nh->fib_nh_dev, dev) ? 1 : 0;

Why the ternary here? The function returns bool, but the iterator merely
checks the value for 0 and caller returns the value as bool as well.

> +}
> +
>  static bool nft_fib6_info_nh_uses_dev(struct fib6_info *rt,
>  				      const struct net_device *dev)
>  {
>  	const struct net_device *nh_dev;
>  	struct fib6_info *iter;
>  
> +	/* External nexthop: fib6_siblings slot aliases nh_list, walk via nh. */
> +	if (rt->nh)
> +		return nexthop_for_each_fib6_nh(rt->nh,
> +						nft_fib6_nh_match_dev_cb,
> +						(void *)dev) != 0;

As above, the '!= 0' is not needed, is it?

> +
>  	nh_dev = fib6_info_nh_dev(rt);
>  	if (nft_fib6_info_nh_dev_match(nh_dev, dev))
>  		return true;
>  
> +	if (!rt->fib6_nsiblings)

Should this access using READ_ONCE() as per commit 31d7d67ba127 ("ipv6:
annotate data-races around rt->fib6_nsiblings")?

> +		return false;
> +
>  	list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {
>  		nh_dev = fib6_info_nh_dev(iter);

I thought about open-coding this to void the need for the callback
wrapper, but it's not worth it.

Cheers, Phil