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

[sashiko-bot@kernel.org]

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.

> +			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