Re: [PATCH bpf-next v9 1/5] 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] Missing `skb_valid_dst()` check for IPv6 allows type confusion with metadata dst entries, leading to an out-of-bounds read and kernel crash.
--

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

bpf: add bpf_icmp_send kfunc

This commit introduces a new bpf_icmp_send kfunc to allow BPF programs to
generate ICMP error messages. It clones the original skb and invokes the
underlying IPv4 or IPv6 ICMP send routines.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2e96b4b847ce1..f3aa494ed1053 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> +__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 ||
> +		    code == ICMP_FRAG_NEEDED) /* needs a valid next-hop MTU */
> +			return -EINVAL;
> +
> +		/* icmp_send requires a rtable; test-run synthetic skbs lack one. */
> +		if (!skb_valid_dst(skb))
> +			return -ENETUNREACH;
> +
> +		nskb = skb_clone(skb, GFP_ATOMIC);

[ ... ]

> +#if IS_ENABLED(CONFIG_IPV6)
> +	case htons(ETH_P_IPV6):
> +		if (type != ICMPV6_DEST_UNREACH)
> +			return -EOPNOTSUPP;
> +		if (code < 0 || code > ICMPV6_REJECT_ROUTE)
> +			return -EINVAL;
> +

[Severity: High]
Should there be a check for skb_valid_dst(skb) here before cloning and
sending the IPv6 ICMP message?

It looks like the IPv4 branch validates the dst to prevent routing table
lookups on synthetic or metadata-dst packets, but the IPv6 branch does not.

If an IPv6 packet with a metadata_dst (e.g., from LWT or a tunnel) is
passed to bpf_icmp_send(), could icmpv6_send() process the invalid dst and
cast it to a struct rt6_info in icmp6_dev()? Dereferencing rt6->rt6i_idev
might then read out of bounds from the metadata_dst struct and cause a
kernel crash.

> +		nskb = skb_clone(skb, GFP_ATOMIC);
> +		if (!nskb)
> +			return -ENOMEM;
> +
> +		memset(IP6CB(nskb), 0, sizeof(*IP6CB(nskb)));
> +		icmpv6_send(nskb, type, code, 0);
> +		consume_skb(nskb);
> +		break;
> +#endif

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