Re: [PATCH bpf-next v4 3/6] bpf: add bpf_icmp_send_unreach kfunc

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] Unchecked `skb->dev` leads to a guaranteed NULL pointer dereference and kernel panic.
- [High] Missing SKB header linearization and bounds validation before accessing IP/IPv6 headers.
- [High] Uninitialized SKB control block (`skb->cb`) causes memory corruption and info leaks via IP options parsing.
- [High] Ad-hoc synchronization with the per-CPU flag `bpf_icmp_send_in_progress` causes false-positive failures and bypasses recursion protection under preemption.
--

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

bpf: add bpf_icmp_send_unreach kfunc

This commit introduces the bpf_icmp_send_unreach kfunc to allow BPF programs
to send ICMP destination unreachable messages. It clones the original SKB to
ensure the packet can still proceed unmodified if desired, and includes a
per-CPU flag to prevent recursion.

> diff --git a/net/core/filter.c b/net/core/filter.c
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> +static DEFINE_PER_CPU(bool, bpf_icmp_send_in_progress);
> +
> +/**
> + * bpf_icmp_send_unreach - Send ICMP destination unreachable error

[ ... ]

> +__bpf_kfunc int bpf_icmp_send_unreach(struct __sk_buff *__skb, int code)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)__skb;
> +	struct sk_buff *nskb;
> +	bool *in_progress;
> +
> +	in_progress = this_cpu_ptr(&bpf_icmp_send_in_progress);
> +	if (*in_progress)
> +		return -EBUSY;

BPF programs can run in preemptible contexts, such as when running with
migrate_disable() and rcu_read_lock(). Is it possible for a task to be
preempted between checking and setting this flag, or while icmp_send() is
executing?

If another task runs on the same CPU while the flag is set, it might
incorrectly fail to send an ICMP message. Alternatively, could a race allow
the recursion protection to be bypassed entirely if tasks interleave their
reads and writes to this flag?

> +
> +	switch (skb->protocol) {
> +#if IS_ENABLED(CONFIG_INET)
> +	case htons(ETH_P_IP):
> +		if (code < 0 || code > NR_ICMP_UNREACH)
> +			return -EINVAL;
> +
> +		nskb = skb_clone(skb, GFP_ATOMIC);
> +		if (!nskb)
> +			return -ENOMEM;
> +
> +		if (!skb_dst(nskb) && ip_route_reply_fetch_dst(nskb) < 0) {

This kfunc is registered in bpf_kfunc_check_set_skb, exposing it to program
types like SOCKET_FILTER or SK_SKB. In these contexts, socket buffers
frequently have neither a destination entry (skb_dst is NULL) nor an
assigned network device (skb->dev is NULL).

When ip_route_reply_fetch_dst() executes, it calls dev_net(skb->dev). Does
this lead to a NULL pointer dereference if the network device is not set?

Additionally, BPF programs can arbitrarily modify the SKB length, and the
network header might reside in the non-linear paged area. Does this code
need to call pskb_network_may_pull() or similar to ensure the headers are
linearized?

ip_route_reply_fetch_dst() blindly dereferences ip_hdr(skb)->saddr, which
might result in out-of-bounds reads if the header is not safely in the
linear data area.

> +			kfree_skb(nskb);
> +			return -EHOSTUNREACH;
> +		}
> +
> +		*in_progress = true;
> +		icmp_send(nskb, ICMP_DEST_UNREACH, code, 0);

skb_clone() copies the original SKB's control block (skb->cb) verbatim. In
hooks like TC, this area contains subsystem-specific data.

icmp_send() casts this control block to struct inet_skb_parm via IPCB().
If the copied control block data happens to set opt.optlen > 0, will
__ip_options_echo() attempt to parse IP options using arbitrary offsets?

Does the new SKB's control block need to be zeroed before passing it to
the ICMP send functions to prevent reading uninitialized values?

> +		*in_progress = false;
> +		kfree_skb(nskb);
> +		break;
> +#endif
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case htons(ETH_P_IPV6):
> +		if (code < 0 || code > ICMPV6_REJECT_ROUTE)
> +			return -EINVAL;
> +
> +		nskb = skb_clone(skb, GFP_ATOMIC);
> +		if (!nskb)
> +			return -ENOMEM;
> +
> +		if (!skb_dst(nskb) && ip6_route_reply_fetch_dst(nskb) < 0) {

The same concerns about skb->dev being NULL and the need for IPv6 header
linearization apply here before calling ip6_route_reply_fetch_dst().

> +			kfree_skb(nskb);
> +			return -EHOSTUNREACH;
> +		}
> +
> +		*in_progress = true;
> +		icmpv6_send(nskb, ICMPV6_DEST_UNREACH, code, 0);

Similarly, does nskb->cb need to be zeroed before calling icmpv6_send()?

> +		*in_progress = false;
> +		kfree_skb(nskb);
> +		break;
> +#endif

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