Re: [PATCH bpf-next v4 1/6] net: move netfilter nf_reject_fill_skb_dst to core ipv4

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] NULL Pointer Dereference on `skb->dev` in exported routing helper.
- [High] Missing Header Linearization and Bounds Check causing Out-of-Bounds Read.
- [Low] Misleading function name implies read-only operation while mutating SKB state.
--

commit d9b5a7cd6924dc358237afb642ea1eb45a09213a
Author: Mahe Tardy <mahe.tardy@gmail.com>

net: move netfilter nf_reject_fill_skb_dst to core ipv4

This patch moves and renames nf_reject_fill_skb_dst to
ip_route_reply_fetch_dst in ipv4/route.c to allow reuse by BPF kfuncs.
It also inlines the nf_ip_route wrapper around ip_route_output_key.

[ ... ]

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index bc1296f0ea69b..7091ef9360737 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -2945,6 +2945,21 @@ struct rtable *ip_route_output_flow(struct net *net, struct flowi4 *flp4,
>  }
>  EXPORT_SYMBOL_GPL(ip_route_output_flow);
>  
> +int ip_route_reply_fetch_dst(struct sk_buff *skb)

This isn't a bug, but does the name ip_route_reply_fetch_dst imply a read-only
operation? Since it mutates the SKB by setting the destination via
skb_dst_set(), could this lead to callers accidentally overwriting and
leaking existing destinations?

Would a name like ip_route_reply_fill_skb_dst better reflect its side effects?

> +{
> +	struct rtable *rt;
> +	struct flowi4 fl4 = {
> +		.daddr = ip_hdr(skb)->saddr

Can this lead to an out-of-bounds read?

Now that this is exported for BPF kfuncs, it can be reached from early
hooks like TC ingress where the IPv4 header might not be linearized yet.

If a malformed packet contains less than sizeof(struct iphdr) bytes of
linear data, does accessing ->saddr require validation via
pskb_may_pull() or similar?

> +	};
> +
> +	rt = ip_route_output_key(dev_net(skb->dev), &fl4);

Can skb->dev be NULL here?

While safe in its original netfilter context, a BPF_PROG_TYPE_NETFILTER
program attached to the NF_INET_LOCAL_OUT hook will receive locally
generated SKBs before skb->dev is assigned (e.g., in __ip_local_out before
ip_output).

Does dev_net(skb->dev) need a check to prevent a kernel panic when called
from such contexts?

> +	if (IS_ERR(rt))
> +		return PTR_ERR(rt);
> +	skb_dst_set(skb, &rt->dst);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ip_route_reply_fetch_dst);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420105816.72168-1-mahe.tardy@gmail.com?part=1