Re: [PATCH bpf v2] bpf: disallow bpf_skb_pull_data() for LWT_SEG6LOCAL

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] LWT-specific SRv6 helpers (bpf_lwt_seg6_action(), bpf_lwt_seg6_adjust_srh(), and bpf_lwt_seg6_store_bytes()) can leave srh_state->srh dangling if an error occurs after skb->head is reallocated, leading to a Use-After-Free when the BPF program finishes.
- [High] bpf_lwt_seg6_adjust_srh() corrupts cloned SKB data when shrinking the header (len < 0).
--

commit 432b32d6ccc70f500393dbc29ce68e92bc184b1c
Author: Weiming Shi <bestswngs@gmail.com>

bpf: disallow bpf_skb_pull_data() for LWT_SEG6LOCAL

An LWT_SEG6LOCAL program can invalidate its cached SRH with
bpf_lwt_seg6_adjust_srh() and then call bpf_skb_pull_data(), leaving the
per-CPU SRH pointer dangling. This commit disallows bpf_skb_pull_data() for
LWT_SEG6LOCAL programs so the verifier rejects this unsafe combination.

Link: https://lore.kernel.org/bpf/DL9COXZQXX4V.1FN45QO2Q77ZH@gmail.com/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8513167a858a8..2a84f9d011314 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -9044,6 +9044,8 @@ static const struct bpf_func_proto *
>  lwt_seg6local_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
>  	switch (func_id) {
> +	case BPF_FUNC_skb_pull_data:
> +		return NULL;
>  #if IS_ENABLED(CONFIG_IPV6_SEG6_BPF)
>  	case BPF_FUNC_lwt_seg6_store_bytes:
>  		return &bpf_lwt_seg6_store_bytes_proto;
>  	case BPF_FUNC_lwt_seg6_action:
>  		return &bpf_lwt_seg6_action_proto;

[Severity: High]
This is a pre-existing issue, but do these LWT-specific SRv6 helpers also
leave srh_state->srh dangling if an error occurs after skb->head is
reallocated?

For instance, in bpf_lwt_seg6_action(), if bpf_push_seg6_encap() fails, the
error path skips bpf_update_srh_state(), leaving the cached SRH pointer
dangling if the head was already reallocated:

net/core/filter.c:bpf_lwt_seg6_action() {
        ...
        err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
                                  param, param_len);
        if (!err)
                bpf_update_srh_state(skb);

        return err;
}

A similar pattern appears in bpf_lwt_seg6_adjust_srh() where an early return
on ipv6_find_hdr() failure misses updating srh_state->srh:

net/core/filter.c:bpf_lwt_seg6_adjust_srh() {
        ...
        if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
                return -EINVAL;
        srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
        ...
}

If the BPF program ignores the error and exits with BPF_OK, does this allow
input_action_end_bpf() to call seg6_bpf_has_valid_srh() and write to the
dangling pointer causing kernel memory corruption?

>  	case BPF_FUNC_lwt_seg6_adjust_srh:
>  		return &bpf_lwt_seg6_adjust_srh_proto;

[Severity: High]
This is another pre-existing issue, but does bpf_lwt_seg6_adjust_srh()
corrupt cloned SKB data when shrinking the header?

When len is less than 0, the code skips skb_cow_head() and directly
modifies the SKB via memmove():

net/core/filter.c:bpf_lwt_seg6_adjust_srh() {
        ...
        } else {
                ret = bpf_skb_net_hdr_pop(skb, offset, -1 * len);
        }
        ...
}

If an LWT_SEG6LOCAL BPF program is executed on a cloned SKB,
bpf_skb_net_hdr_pop() verifies the buffer via pskb_may_pull() which
returns 1 without uncloning because the target data is already within the
linear portion. 

Will bpf_skb_generic_pop() then use memmove() to modify skb->data,
corrupting the data buffer for other users of the cloned SKB?

>  #endif
>  	default:
>  		return lwt_out_func_proto(func_id, prog);
>  	}
>  }

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