Re: [PATCH bpf-next v8 3/7] bpf: add bpf_icmp_send kfunc

[Mahe Tardy <mahe.tardy@gmail.com>]

On Mon, Jun 22, 2026 at 12:32:22PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] bpf_icmp_send() causes __icmp_send() to bypass broadcast checks and spoof the source address for TC ingress packets.
> --
> 
> commit 2acddb10f02b54c09816af3432bc928c87824069
> Author: Mahe Tardy <mahe.tardy@gmail.com>
> 
> bpf: add bpf_icmp_send kfunc
> 
> This commit adds the bpf_icmp_send kfunc to allow cgroup_skb and tc programs
> to send ICMP error messages in response to packets. It clones the original
> packet, optionally assigns a route using ip_route_reply_fill_dst for
> unrouted ingress packets, and invokes the standard icmp_send or icmpv6_send
> functions.
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> 
> [ ... ]
> 
> > @@ -12546,6 +12548,101 @@ __bpf_kfunc int bpf_xdp_pull_data(struct xdp_md *x, u32 len)
> > +__bpf_kfunc int bpf_icmp_send(struct __sk_buff *skb_ctx, int type, int code)
> > +{
> > +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> > +	struct sk_buff *nskb;
> > +	struct sock *sk;
> 
> [ ... ]
> 
> > +	switch (skb->protocol) {
> > +#if IS_ENABLED(CONFIG_INET)
> > +	case htons(ETH_P_IP):
> > +		if (type != ICMP_DEST_UNREACH)
> > +			return -EOPNOTSUPP;
> > +		if (code < 0 || code > NR_ICMP_UNREACH)
> > +			return -EINVAL;
> > +
> > +		nskb = skb_clone(skb, GFP_ATOMIC);
> > +		if (!nskb)
> > +			return -ENOMEM;
> > +
> > +		if (!pskb_network_may_pull(nskb, sizeof(struct iphdr))) {
> > +			kfree_skb(nskb);
> > +			return -EBADMSG;
> > +		}
> > +
> > +		if (!skb_dst(nskb) && ip_route_reply_fill_dst(nskb) < 0) {
>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> Does using ip_route_reply_fill_dst() here cause regressions with how
> __icmp_send() evaluates routing flags?
> 
> For TC ingress packets where skb_dst(nskb) is NULL, this attaches an output
> route. However, __icmp_send() expects the SKB to have an input route when
> evaluating rt_flags:
> 
> net/ipv4/icmp.c:__icmp_send() {
>         ...
>         if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
>                 goto out;
>         ...
>         saddr = iph->daddr;
>         if (!(rt->rt_flags & RTCF_LOCAL)) {
>                 ...
>                 saddr = 0;
>         }
>         ...
> }
> 
> Because the attached route is an output route, it lacks RTCF_BROADCAST,
> which might incorrectly allow ICMP replies to broadcast traffic.
> 
> Additionally, because RTCF_LOCAL is missing on the output route,
> __icmp_send() clears saddr. This causes icmp_route_lookup() to select the
> primary IP of the outgoing interface rather than the specific IP the packet
> was originally addressed to, violating RFC 1122 requirements for ICMP error
> source addresses.

Mmh that might be an actual issue that sashiko found here and could
simplify this patchset by getting rid of the netfilter helpers. Let me
investigate a little bit more.

> 
> > +			kfree_skb(nskb);
> > +			return -EHOSTUNREACH;
> > +		}
> > +
> > +		memset(IPCB(nskb), 0, sizeof(struct inet_skb_parm));
> > +
> > +		icmp_send(nskb, type, code, 0);
> > +		consume_skb(nskb);
> > +		break;
> > +#endif
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260622120515.137082-1-mahe.tardy@gmail.com?part=3