Re: [RFC bpf-next v3 2/3] bpf: Add PPPoE encap/decap kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] bpf: stale skb->cb data pointers on bpf_skb_pppoe_encap() and
  decap() error
- [High] bpf: Signed-off-by trailer uses handle instead of real full
  name
- [Medium] bpf: missing skb_postpush_rcsum() in bpf_skb_pppoe_encap()
  corrupts csum
--

commit 41cc0bc50e47c52d87a2f024362b196725052585
Author: ThisSeanZhang <thisseanzhang@gmail.com>

> bpf: Add PPPoE encap/decap kfuncs
>
> Add bpf_skb_pppoe_encap() and bpf_skb_pppoe_decap() kfuncs so that a
> TC BPF program can encapsulate an IPv4 or IPv6 packet into a PPPoE
> session frame, and decapsulate one again:
>
>         bpf_skb_pppoe_encap(skb, sid, flags);
>         bpf_skb_pppoe_decap(skb, flags);
>
> The kfuncs insert or remove the 8 byte PPPoE session header between
> the MAC header and the network header, and update the skb metadata to
> match: skb->protocol is set to ETH_P_PPP_SES on encapsulation, or
> restored from the PPP protocol field on decapsulation, mac_len is
> reset, and the GSO metadata is adjusted like bpf_skb_adjust_room()
> does (gso_size, gso_segs, SKB_GSO_DODGY).
>
> Both kfuncs accept BPF_F_ADJ_ROOM_FIXED_GSO to leave gso_size
> unchanged and only recompute gso_segs, reusing the existing flag
> instead of adding a new one; UDP GSO packets require it, as their
> gso_size is the datagram size. Unknown flags are rejected.
>
> Previously, a TC program could add or remove the header bytes
> manually, but the skb would keep a stale skb->protocol: forwarded GSO
> packets larger than the PPPoE MTU could not reach the PPPoE
> segmentation path registered for ETH_P_PPP_SES and could be dropped.
> bpf_skb_adjust_room() cannot be used for the decapsulation direction
> either, because it only accepts ETH_P_IP or ETH_P_IPV6 in
> skb->protocol.
>
> The encap kfunc fills in the PPPoE header itself from the session id:
> version 1, type 1, code 0, and the PPP protocol derived from
> skb->protocol, mirroring what pppoe_xmit() writes. The ethertype of
> the MAC header remains for the BPF program, like with the existing
> BPF_F_ADJ_ROOM_ENCAP_* flags. skb->encapsulation is deliberately left
> unset, like pppoe_xmit() does: PPPoE is not an IP tunnel, and setting
> it would narrow the device features to hw_enc_features in
> netif_skb_features(). The decap kfunc accepts only PPP_IP and PPP_IPV6
> payloads and requires a full inner L3 header to remain.
>
> The kfuncs are registered for SCHED_CLS and SCHED_ACT with the new
> KF_CHANGES_PKT flag, so the verifier invalidates packet pointers after
> a call, like it does for bpf_skb_adjust_room().
>
> PPPoE GRO/GSO support must be built in or loaded before this path is
> used; loading pppoe.ko is left to user space.
>
> Signed-off-by: ThisSeanZhang <thisseanzhang@gmail.com>

[Severity: High]
The Signed-off-by trailer and Author header use a single-word handle
("ThisSeanZhang") rather than a full real name, which does not meet the
Developer's Certificate of Origin requirements in
Documentation/process/submitting-patches.rst.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 9245009d9f1eb..3d9143552f0d1 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> +__bpf_kfunc int bpf_skb_pppoe_encap(struct __sk_buff *skb_ctx, __be16 sid,
> +				    u64 flags)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	struct pppoe_hdr *ph;
> +	u16 ppp_proto;
> +	u32 off;
> +	int ret;
> +
> +	if (flags & ~BPF_F_ADJ_ROOM_FIXED_GSO)
> +		return -EINVAL;
> +
> +	if (skb->protocol != htons(ETH_P_IP) &&
> +	    skb->protocol != htons(ETH_P_IPV6))
> +		return -ENOTSUPP;
> +
> +	if (skb->len + PPPOE_SES_HLEN > BPF_SKB_MAX_LEN && !skb_is_gso(skb))
> +		return -ENOTSUPP;
> +
> +	off = skb_mac_header_len(skb);
> +
> +	ret = bpf_skb_net_grow(skb, off, PPPOE_SES_HLEN, flags);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (skb->protocol == htons(ETH_P_IP))
> +		ppp_proto = PPP_IP;
> +	else
> +		ppp_proto = PPP_IPV6;
> +
> +	ph = (struct pppoe_hdr *)(skb->data + off);
> +	ph->ver = 1;
> +	ph->type = 1;
> +	ph->code = 0;
> +	ph->sid = sid;
> +	ph->length = htons(skb->len - off - sizeof(*ph));
> +	put_unaligned_be16(ppp_proto, (u8 *)ph + sizeof(*ph));
> +
> +	skb->protocol = htons(ETH_P_PPP_SES);
> +	skb_reset_mac_len(skb);
> +	if (skb_valid_dst(skb))
> +		skb_dst_drop(skb);
> +
> +	__skb_reset_checksum_unnecessary(skb);
> +	bpf_compute_data_pointers(skb);
> +
> +	return 0;
> +}

[Severity: Medium]
Does writing the PPPoE header without calling
skb_postpush_rcsum(skb, ph, PPPOE_SES_HLEN) leave skb->csum stale on
CHECKSUM_COMPLETE packets?

bpf_skb_net_grow() -> bpf_skb_net_hdr_push() -> bpf_skb_generic_push()
zeroes the pushed 8 bytes and skips skb_postpush_rcsum() because zeroed
bytes do not change skb->csum, and __skb_reset_checksum_unnecessary() only
clears CHECKSUM_UNNECESSARY.  Writing non-zero PPPoE header fields at
skb_network_header(skb) without updating skb->csum corrupts the checksum on
ingress packets with hardware CHECKSUM_COMPLETE (and fails
BPF_PROG_TEST_RUN with BPF_F_TEST_SKB_CHECKSUM_COMPLETE), whereas
bpf_skb_pppoe_decap() and pppoe_rcv() subtract the 8-byte header from
skb->csum via skb_postpull_rcsum().

[ ... ]

> +__bpf_kfunc int bpf_skb_pppoe_decap(struct __sk_buff *skb_ctx, u64 flags)
> +{
> +	struct sk_buff *skb = (struct sk_buff *)skb_ctx;
> +	__be16 inner_proto;
> +	u32 inner_len, off;
> +	u16 ppp_proto;
> +	int ret;
> +
> +	if (flags & ~BPF_F_ADJ_ROOM_FIXED_GSO)
> +		return -EINVAL;
> +
> +	if (skb->protocol != htons(ETH_P_PPP_SES))
> +		return -ENOTSUPP;
> +
> +	off = skb_mac_header_len(skb);
> +
> +	if (skb->len < off + PPPOE_SES_HLEN)
> +		return -EINVAL;
> +
> +	if (!pskb_may_pull(skb, off + PPPOE_SES_HLEN))
> +		return -ENOMEM;
> +
> +	ppp_proto = get_unaligned_be16(skb->data + off +
> +				       sizeof(struct pppoe_hdr));
> +	switch (ppp_proto) {
> +	case PPP_IP:
> +		inner_proto = htons(ETH_P_IP);
> +		inner_len = sizeof(struct iphdr);
> +		break;
> +	case PPP_IPV6:
> +		inner_proto = htons(ETH_P_IPV6);
> +		inner_len = sizeof(struct ipv6hdr);
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	/* A full inner L3 header must remain after decapsulation. */
> +	if (skb->len - off - PPPOE_SES_HLEN < inner_len)
> +		return -EINVAL;
> +
> +	ret = bpf_skb_net_shrink(skb, off, PPPOE_SES_HLEN, flags);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
Can error returns in bpf_skb_pppoe_decap() and bpf_skb_pppoe_encap() leave
stale data_end and data_meta pointers in skb->cb after skb->head is
reallocated?

In bpf_skb_pppoe_decap(), pskb_may_pull() can reallocate skb->head via
pskb_expand_head() and free the old buffer when pulling bytes from
nonlinear fragments or expanding a cloned skb.  If ppp_proto is then
rejected as unsupported (for example, a PPP LCP/IPCP control frame),
inner_len is not met, or bpf_skb_net_shrink() fails, the function returns
without calling bpf_compute_data_pointers(skb).  Similarly, in
bpf_skb_pppoe_encap(), bpf_skb_net_grow() can reallocate skb->head in
skb_cow_head() and shift skb->data in bpf_skb_net_hdr_push() before failing
in pskb_may_pull() or skb_linearize().

Because TC programs read ctx->data from skb->data (now pointing to the new
buffer) while reading ctx->data_end and ctx->data_meta from skb->cb (still
pointing into the old freed buffer), a program that continues inspecting
the packet after an error return can pass verifier bounds checks and
trigger a kernel heap use-after-free or out-of-bounds access.  Calling
bpf_compute_data_pointers(skb) on the error paths after any potential
reallocation, as bpf_skb_adjust_room() does unconditionally, avoids leaving
dangling pointers.

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