Re: [PATCH bpf v2] bpf: Fix non-linear SRH access in bpf_update_srh_state()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free and OOB access in bpf_lwt_seg6_store_bytes and bpf_lwt_seg6_adjust_srh
--

commit f61c52a826d2d54d2acaba3787ebc0a69bbfc5af
Author: Cen Zhang (Microsoft Security FORGE Labs) <cenzhang@linux.microsoft.com>

bpf: Fix non-linear SRH access in bpf_update_srh_state()

This commit fixes an out-of-bounds access in bpf_update_srh_state() by
replacing ipv6_find_hdr() with seg6_get_srh() to ensure the SRH is pulled
into linear memory. It also updates bpf_lwt_seg6_action() to correctly
handle skb->head reallocations.

Link: https://lore.kernel.org/bpf/20260901183151.16648-1-cenzhang@linux.microsoft.com/
Link: https://lore.kernel.org/bpf/CABFh=a5iLOEJdPhoaWUhLc0eEqAuhnd83_jJr9MVZZG6gSJAEw@mail.gmail.com/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 8513167a858a8..b037d70e5fe65 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[Severity: High]
This is a pre-existing issue, but does bpf_lwt_seg6_store_bytes() have a
use-after-free and out-of-bounds access risk similar to the one fixed here?

In bpf_lwt_seg6_store_bytes():

	if (unlikely(bpf_try_make_writable(skb, offset + len)))
		return -EFAULT;
	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
		return -EINVAL;
	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);

The call to bpf_try_make_writable() can reallocate skb->head. If the
subsequent call to ipv6_find_hdr() fails, the function returns without
updating srh_state->srh, leaving a dangling pointer to the freed skb->head.

Additionally, if ipv6_find_hdr() succeeds but locates the SRH in non-linear
data, the cache update uses skb->data + srhoff.

Could this result in an out-of-bounds pointer being cached, just like the
bug this patch fixes in bpf_update_srh_state()?

[Severity: High]
This is a pre-existing issue, but bpf_lwt_seg6_adjust_srh() appears to
suffer from similar memory safety issues.

In bpf_lwt_seg6_adjust_srh():

	if (len > 0) {
		ret = skb_cow_head(skb, len);
		if (unlikely(ret < 0))
			return ret;
		...
	} else {
		ret = bpf_skb_net_hdr_pop(skb, offset, -1 * len);
	}

	bpf_compute_data_pointers(skb);
	if (unlikely(ret < 0))
		return ret;
	...
	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
		return -EINVAL;
	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);

Operations like skb_cow_head() or bpf_skb_net_hdr_pop() can reallocate
skb->head. If they fail, returning early leaves srh_state->srh pointing to
freed memory. Similarly, if ipv6_find_hdr() fails, it returns early leaving
a dangling pointer.

If ipv6_find_hdr() succeeds, it also caches a potentially out-of-bounds
pointer from non-linear data.

Would it make sense to update both of these functions to safely handle
skb->head reallocations and use seg6_get_srh() instead of ipv6_find_hdr()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135324.60165-1-cenzhang@linux.microsoft.com?part=1