[PATCH bpf] xdp: fix cpumap redirect SKB creation bug

[PATCH bpf] xdp: fix cpumap redirect SKB creation bug

[Jesper Dangaard Brouer]

We want to avoid leaking pointer info from xdp_frame (that is placed in
top of frame) like commit 6dfb970d3dbd ("xdp: avoid leaking info stored in
frame data on page reuse"), and followup commit 97e19cce05e5 ("bpf:
reserve xdp_frame size in xdp headroom") that reserve this headroom.

These changes also affected how cpumap constructed SKBs, as xdpf->headroom
size changed, the skb data starting point were in-effect shifted with 32
bytes (sizeof xdp_frame). This was still okay, as the cpumap frame_size
calculation also included xdpf->headroom which were reduced by same amount.

A bug was introduced in commit 77ea5f4cbe20 ("bpf/cpumap: make sure
frame_size for build_skb is aligned if headroom isn't"), where the
xdpf->headroom became part of the SKB_DATA_ALIGN rounding up. This
round-up to find the frame_size is in principle still correct as it does
not exceed the 2048 bytes frame_size (which is max for ixgbe and i40e),
but the 32 bytes offset of pkt_data_start puts this over the 2048 bytes
limit. This cause skb_shared_info to spill into next frame. It is a little
hard to trigger, as the SKB need to use above 15 skb_shinfo->frags[] as
far as I calculate. This does happen in practise for TCP streams when
skb_try_coalesce() kicks in.

KASAN can be used to detect these wrong memory accesses, I've seen:
 BUG: KASAN: use-after-free in skb_try_coalesce+0x3cb/0x760
 BUG: KASAN: wild-memory-access in skb_release_data+0xe2/0x250

Driver veth also construct a SKB from xdp_frame in this way, but is not
affected, as it doesn't reserve/deduct the room (used by xdp_frame) from
the SKB headroom. Instead is clears the pointers via xdp_scrub_frame(),
and allows SKB to use this area.

The fix in this patch is to do like veth and instead allow SKB to (re)use
the area occupied by xdp_frame, by clearing via xdp_scrub_frame().  (This
does kill the idea of the SKB being able to access (mem) info from this
area, but I guess it was a bad idea anyhow, and it was already killed by
the veth changes.)

Fixes: 77ea5f4cbe20 ("bpf/cpumap: make sure frame_size for build_skb is aligned if headroom isn't")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
This patch is stable material for v5.0 see:
  $ git describe --contains 77ea5f4cbe20
  v5.0-rc1~129^2~15^2~1

 kernel/bpf/cpumap.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 8974b3755670..3c18260403dd 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -162,10 +162,14 @@ static void cpu_map_kthread_stop(struct work_struct *work)
 static struct sk_buff *cpu_map_build_skb(struct bpf_cpu_map_entry *rcpu,
 					 struct xdp_frame *xdpf)
 {
+	unsigned int hard_start_headroom;
 	unsigned int frame_size;
 	void *pkt_data_start;
 	struct sk_buff *skb;
 
+	/* Part of headroom was reserved to xdpf */
+	hard_start_headroom = sizeof(struct xdp_frame) +  xdpf->headroom;
+
 	/* build_skb need to place skb_shared_info after SKB end, and
 	 * also want to know the memory "truesize".  Thus, need to
 	 * know the memory frame size backing xdp_buff.
@@ -183,15 +187,15 @@ static struct sk_buff *cpu_map_build_skb(struct bpf_cpu_map_entry *rcpu,
 	 * is not at a fixed memory location, with mixed length
 	 * packets, which is bad for cache-line hotness.
 	 */
-	frame_size = SKB_DATA_ALIGN(xdpf->len + xdpf->headroom) +
+	frame_size = SKB_DATA_ALIGN(xdpf->len + hard_start_headroom) +
 		SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	pkt_data_start = xdpf->data - xdpf->headroom;
+	pkt_data_start = xdpf->data - hard_start_headroom;
 	skb = build_skb(pkt_data_start, frame_size);
 	if (!skb)
 		return NULL;
 
-	skb_reserve(skb, xdpf->headroom);
+	skb_reserve(skb, hard_start_headroom);
 	__skb_put(skb, xdpf->len);
 	if (xdpf->metasize)
 		skb_metadata_set(skb, xdpf->metasize);
@@ -205,6 +209,9 @@ static struct sk_buff *cpu_map_build_skb(struct bpf_cpu_map_entry *rcpu,
 	 * - RX ring dev queue index	(skb_record_rx_queue)
 	 */
 
+	/* Allow SKB to reuse area used by xdp_frame */
+	xdp_scrub_frame(xdpf);
+
 	return skb;
 }
 

Re: [PATCH bpf] xdp: fix cpumap redirect SKB creation bug

[Alexei Starovoitov]

On Fri, Mar 29, 2019 at 2:18 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> We want to avoid leaking pointer info from xdp_frame (that is placed in
> top of frame) like commit 6dfb970d3dbd ("xdp: avoid leaking info stored in
> frame data on page reuse"), and followup commit 97e19cce05e5 ("bpf:
> reserve xdp_frame size in xdp headroom") that reserve this headroom.
>
> These changes also affected how cpumap constructed SKBs, as xdpf->headroom
> size changed, the skb data starting point were in-effect shifted with 32
> bytes (sizeof xdp_frame). This was still okay, as the cpumap frame_size
> calculation also included xdpf->headroom which were reduced by same amount.
>
> A bug was introduced in commit 77ea5f4cbe20 ("bpf/cpumap: make sure
> frame_size for build_skb is aligned if headroom isn't"), where the
> xdpf->headroom became part of the SKB_DATA_ALIGN rounding up. This
> round-up to find the frame_size is in principle still correct as it does
> not exceed the 2048 bytes frame_size (which is max for ixgbe and i40e),
> but the 32 bytes offset of pkt_data_start puts this over the 2048 bytes
> limit. This cause skb_shared_info to spill into next frame. It is a little
> hard to trigger, as the SKB need to use above 15 skb_shinfo->frags[] as
> far as I calculate. This does happen in practise for TCP streams when
> skb_try_coalesce() kicks in.
>
> KASAN can be used to detect these wrong memory accesses, I've seen:
>  BUG: KASAN: use-after-free in skb_try_coalesce+0x3cb/0x760
>  BUG: KASAN: wild-memory-access in skb_release_data+0xe2/0x250
>
> Driver veth also construct a SKB from xdp_frame in this way, but is not
> affected, as it doesn't reserve/deduct the room (used by xdp_frame) from
> the SKB headroom. Instead is clears the pointers via xdp_scrub_frame(),
> and allows SKB to use this area.
>
> The fix in this patch is to do like veth and instead allow SKB to (re)use
> the area occupied by xdp_frame, by clearing via xdp_scrub_frame().  (This
> does kill the idea of the SKB being able to access (mem) info from this
> area, but I guess it was a bad idea anyhow, and it was already killed by
> the veth changes.)
>
> Fixes: 77ea5f4cbe20 ("bpf/cpumap: make sure frame_size for build_skb is aligned if headroom isn't")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Applied. Thanks