Re: [PATCH net 03/10] netfilter: ip6t_rpfilter: reject routes without inet6_dev

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: ip6t_rpfilter: reject routes without inet6_dev

This adds a NULL check on rt6i_idev right after the ip6_route_lookup()
call in rpfilter_lookup_reverse6(), so routes that survived in the FIB
after their nexthop device lost its inet6_dev are skipped. It protects
the later rt6i_idev->dev dereferences in the same function.

Closes: https://lore.kernel.org/all/VtWUkE8QzJt5CroTj2V2v3ZQ0gwbXZ7nq7I3@bugs.sh/

> diff --git a/net/ipv6/netfilter/ip6t_rpfilter.c b/net/ipv6/netfilter/ip6t_rpfilter.c
> index 67c87a88cde4f..b5def30c3127e 100644
> --- a/net/ipv6/netfilter/ip6t_rpfilter.c
> +++ b/net/ipv6/netfilter/ip6t_rpfilter.c
> @@ -61,7 +61,7 @@ static bool rpfilter_lookup_reverse6(struct net *net, const struct sk_buff *skb,
>  		fl6.flowi6_oif = dev->ifindex;
>  
>  	rt = (void *)ip6_route_lookup(net, &fl6, skb, lookup_flags);
> -	if (rt->dst.error)
> +	if (rt->dst.error || !rt->rt6i_idev)
>  		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but if rt6i_idev can be NULL on
an error-free route, does the same dereference remain unguarded in
validate_ipv6_net_dev() in drivers/infiniband/core/cma.c?

drivers/infiniband/core/cma.c:validate_ipv6_net_dev() {
	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
					 &src_addr->sin6_addr, net_dev->ifindex,
					 NULL, strict);
	bool ret;

	if (!rt)
		return false;

	ret = rt->rt6i_idev->dev == net_dev;
	...
}

The !rt test there looks equivalent to the rt->dst.error test this patch
just found insufficient, since rt6_lookup() only returns NULL for the
error case:

net/ipv6/route.c:rt6_lookup() {
	dst = fib6_rule_lookup(net, &fl6, skb, flags, ip6_pol_route_lookup);
	if (dst->error == 0)
		return dst_rt6_info(dst);

	dst_release(dst);

	return NULL;
}

So the route class described in this commit message, an error-free cache
route whose rt6i_idev was left NULL by ip6_rt_copy_init():

net/ipv6/route.c:ip6_rt_copy_init() {
	...
	rt->rt6i_idev = dev ? in6_dev_get(dev) : NULL;
	...
}

would pass the !rt check and then be dereferenced when validating an
incoming RoCE CM request via validate_net_dev() -> validate_ipv6_net_dev().

The other in-tree readers of the field already treat NULL as legal:

net/ipv6/fib6_rules.c:fib6_rule_suppress() {
	...
	if (rt->rt6i_idev)
		dev = rt->rt6i_idev->dev;
	...
}

net/ipv6/icmp.c:icmp6_send() {
	...
		if (rt6 && rt6->rt6i_idev)
			dev = rt6->rt6i_idev->dev;
	...
}

Would a follow-up patch adding the same guard to cma.c make sense? Note
that reaching the cma.c site is not unprivileged end-to-end the way the
rpfilter path is: it needs the NULL-idev route to exist in the namespace
of the RoCE netdev, which normally means CAP_NET_ADMIN in init_net plus an
RDMA-capable device. Nothing later in this series touches
drivers/infiniband/core/cma.c or net/ipv6/route.c, so the call site is
still unguarded at the end of the series.