Re: [PATCH bpf] bpf: refresh seg6local SRH pointer after skb pull

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The fix for the dangling SRH pointer is conditionally compiled under `CONFIG_IPV6_SEG6_BPF`, leaving the use-after-free vulnerability unpatched when IPv6 is compiled as a module (`CONFIG_IPV6=m`).
- [Critical] Early error returns in `bpf_lwt_seg6_store_bytes`, `bpf_lwt_seg6_adjust_srh`, and `bpf_lwt_seg6_action` leave `srh_state->srh` dangling if the SKB head was reallocated before the failure, causing the exact same use-after-free that the patch attempts to fix for `bpf_skb_pull_data`.
- [High] `bpf_lwt_seg6_store_bytes`, `bpf_lwt_seg6_adjust_srh`, and `bpf_update_srh_state` use `ipv6_find_hdr` without `IP6_FH_F_SKIP_RH`, correctly identifying the wrong routing header if multiple are present.
--

commit 681db76be957fb9fd9b5dfbb7a3c0b06da4f90b7
Author: Weiming Shi <bestswngs@gmail.com>

bpf: refresh seg6local SRH pointer after skb pull

An LWT_SEG6LOCAL program can invalidate its cached SRH with
bpf_lwt_seg6_adjust_srh() and then call bpf_skb_pull_data(). The latter
may reallocate skb->head, leaving the per-CPU SRH pointer dangling.
Post-program SRH validation then writes through that pointer.

Closes: https://lore.kernel.org/all/GCy0KRM2IcQGoJQTjJEU9D0maBxXzEDHuQpq@bugs.sh/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e7535523..e61f9e9226b10 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7162,6 +7162,32 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
>  	.arg2_type	= ARG_ANYTHING,
>  	.arg3_type	= ARG_ANYTHING,
>  };

[Severity: High]
This isn't a bug introduced by this patch, but bpf_lwt_seg6_store_bytes(),
bpf_lwt_seg6_adjust_srh(), and bpf_update_srh_state() use ipv6_find_hdr()
without IP6_FH_F_SKIP_RH, which can correctly identify the wrong routing
header if multiple are present.

If a remote packet arrives with an exhausted Routing Header (e.g., Type 0)
followed by an active SRH, seg6_get_srh() initially correctly skips the
exhausted header using IP6_FH_F_SKIP_RH.

However, when the BPF program calls bpf_lwt_seg6_store_bytes() or
bpf_lwt_seg6_adjust_srh(), they recalculate the pointer using
ipv6_find_hdr() without IP6_FH_F_SKIP_RH, which stops at the exhausted
first header:

net/core/filter.c:bpf_lwt_seg6_store_bytes() {
    ...
    if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
        return -EINVAL;
    ...
}

This reassigns srh_state->srh to the wrong header. Later,
seg6_bpf_has_valid_srh() overwrites the hdrlen of the exhausted header and
validates against it, which can cause packet corruption and SRH validation
bypass.

Should we update these functions to use IP6_FH_F_SKIP_RH when finding the
routing header to ensure we match the correct active SRH?

> +
> +BPF_CALL_2(bpf_lwt_seg6_pull_data, struct sk_buff *, skb, u32, len)
> +{
> +	struct seg6_bpf_srh_state *srh_state =
> +		this_cpu_ptr(&seg6_bpf_srh_states);
> +	unsigned int srhoff;
> +	int ret;
> +
> +	lockdep_assert_held(&srh_state->bh_lock);
> +	if (!srh_state->srh)
> +		return ____bpf_skb_pull_data(skb, len);
> +
> +	srhoff = (unsigned char *)srh_state->srh - skb->data;
> +	ret = ____bpf_skb_pull_data(skb, len);
> +	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> +
> +	return ret;
> +}

[Severity: Critical]
This is a pre-existing issue, but I noticed early error returns in
bpf_lwt_seg6_store_bytes(), bpf_lwt_seg6_adjust_srh(), and
bpf_lwt_seg6_action() leave srh_state->srh dangling if the SKB head was
reallocated before the failure, causing the exact same use-after-free
regression that this patch attempts to fix for bpf_skb_pull_data().

For example, in bpf_lwt_seg6_store_bytes():

net/core/filter.c:bpf_lwt_seg6_store_bytes() {
    ...
    if (unlikely(bpf_try_make_writable(skb, offset + len)))
        return -EFAULT;
    ...
}

If bpf_try_make_writable() successfully reallocates skb->head and frees the
old head, but then fails in a subsequent step such as an skb_clone()
fragment failure, it returns an error code immediately.

Because the early error paths do not update srh_state->srh, it remains
pointing to the freed old head. When the BPF program finishes,
seg6_bpf_has_valid_srh() accesses this freed memory.

A similar situation occurs in bpf_lwt_seg6_adjust_srh():

net/core/filter.c:bpf_lwt_seg6_adjust_srh() {
    ...
    if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
        return -EINVAL;
    ...
}

If the earlier skb_cow_head() successfully freed the old head, and then this
IPv6 header search fails, it returns early and leaves the SRH pointer dangling.

Also in bpf_lwt_seg6_action():

net/core/filter.c:bpf_lwt_seg6_action() {
    ...
    if (!pskb_pull(skb, hdroff))
        return -EBADMSG;
    ...
}

If pskb_pull() reallocates the head and fails, bpf_update_srh_state(skb) is
never called, leaving the SRH pointer dangling.

Should we also ensure the SRH pointer is updated on early error returns in
these functions, similarly to how it is handled for bpf_skb_pull_data() here?

> +
> +static const struct bpf_func_proto bpf_lwt_seg6_pull_data_proto = {
> +	.func		= bpf_lwt_seg6_pull_data,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_CTX,
> +	.arg2_type	= ARG_ANYTHING,
> +};
>  #endif /* CONFIG_IPV6_SEG6_BPF */

[ ... ]

> @@ -9052,6 +9078,8 @@ lwt_seg6local_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  		return &bpf_lwt_seg6_action_proto;
>  	case BPF_FUNC_lwt_seg6_adjust_srh:
>  		return &bpf_lwt_seg6_adjust_srh_proto;
> +	case BPF_FUNC_skb_pull_data:
> +		return &bpf_lwt_seg6_pull_data_proto;
>  #endif
>  	default:
>  		return lwt_out_func_proto(func_id, prog);

[Severity: Critical]
This is a pre-existing issue, but I noticed the fix for the dangling SRH
pointer is conditionally compiled under CONFIG_IPV6_SEG6_BPF, leaving the
use-after-free regression unpatched when IPv6 is compiled as a module
(CONFIG_IPV6=m).

When IPv6 is built as a module, CONFIG_IPV6_SEG6_BPF evaluates to 'n'.
BPF programs can still be attached via parse_nla_bpf() which unconditionally
supports BPF_PROG_TYPE_LWT_SEG6LOCAL. The verifier defaults
BPF_FUNC_skb_pull_data to the standard bpf_skb_pull_data_proto wrapper
instead of this newly introduced safe wrapper because of this fallback:

net/core/filter.c:lwt_seg6local_func_proto() {
    ...
#if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
    ...
#endif
    default:
        return lwt_out_func_proto(func_id, prog);
}

This standard wrapper can reallocate the SKB head without updating the
per-CPU srh_state->srh pointer. When the BPF program finishes,
input_action_end_bpf() in net/ipv6/seg6_local.c reads the dangling
pointer regardless of CONFIG_IPV6_SEG6_BPF:

net/ipv6/seg6_local.c:input_action_end_bpf() {
    ...
    if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
        goto drop;
    ...
}

Can we make sure the safe wrapper is also used when IPv6 is compiled as a
module to avoid this regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907192129.557377-2-bestswngs@gmail.com?part=1