[bpf-next,1/2] bpf, test_run: fix crashes due to XDP frame overwriting/corruption

Commit Message

Alexander Lobakin March 16, 2023, 5:50 p.m. UTC

syzbot and Ilya faced the splats when %XDP_PASS happens for bpf_test_run
after skb PP recycling was enabled for {__,}xdp_build_skb_from_frame():

BUG: kernel NULL pointer dereference, address: 0000000000000d28
RIP: 0010:memset_erms+0xd/0x20 arch/x86/lib/memset_64.S:66
[...]
Call Trace:
 <TASK>
 __finalize_skb_around net/core/skbuff.c:321 [inline]
 __build_skb_around+0x232/0x3a0 net/core/skbuff.c:379
 build_skb_around+0x32/0x290 net/core/skbuff.c:444
 __xdp_build_skb_from_frame+0x121/0x760 net/core/xdp.c:622
 xdp_recv_frames net/bpf/test_run.c:248 [inline]
 xdp_test_run_batch net/bpf/test_run.c:334 [inline]
 bpf_test_run_xdp_live+0x1289/0x1930 net/bpf/test_run.c:362
 bpf_prog_test_run_xdp+0xa05/0x14e0 net/bpf/test_run.c:1418
[...]

This happens due to that it calls xdp_scrub_frame(), which nullifies
xdpf->data. bpf_test_run code doesn't reinit the frame when the XDP
program doesn't adjust head or tail. Previously, %XDP_PASS meant the
page will be released from the pool and returned to the MM layer, but
now it does return to the Pool with the nullified xdpf->data, which
doesn't get reinitialized then.
So, in addition to checking whether the head and/or tail have been
adjusted, check also for a potential XDP frame corruption. xdpf->data
is 100% affected and also xdpf->flags is the field closest to the
metadata / frame start. Checking for these two should be enough for
non-extreme cases.

Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from XDP frames")
Reported-by: [e-mail]
Link: https://lore.kernel.org/bpf/[e-mail]
Reported-by: Ilya Leoshkevich <[e-mail]>
Link: https://lore.kernel.org/bpf/[e-mail]
Signed-off-by: Alexander Lobakin <[e-mail]>
---
 net/bpf/test_run.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Toke Høiland-Jørgensen March 16, 2023, 8:09 p.m. UTC | #1

Alexander Lobakin <[e-mail]> writes:

> syzbot and Ilya faced the splats when %XDP_PASS happens for bpf_test_run
> after skb PP recycling was enabled for {__,}xdp_build_skb_from_frame():
>
> BUG: kernel NULL pointer dereference, address: 0000000000000d28
> RIP: 0010:memset_erms+0xd/0x20 arch/x86/lib/memset_64.S:66
> [...]
> Call Trace:
>  <TASK>
>  __finalize_skb_around net/core/skbuff.c:321 [inline]
>  __build_skb_around+0x232/0x3a0 net/core/skbuff.c:379
>  build_skb_around+0x32/0x290 net/core/skbuff.c:444
>  __xdp_build_skb_from_frame+0x121/0x760 net/core/xdp.c:622
>  xdp_recv_frames net/bpf/test_run.c:248 [inline]
>  xdp_test_run_batch net/bpf/test_run.c:334 [inline]
>  bpf_test_run_xdp_live+0x1289/0x1930 net/bpf/test_run.c:362
>  bpf_prog_test_run_xdp+0xa05/0x14e0 net/bpf/test_run.c:1418
> [...]
>
> This happens due to that it calls xdp_scrub_frame(), which nullifies
> xdpf->data. bpf_test_run code doesn't reinit the frame when the XDP
> program doesn't adjust head or tail. Previously, %XDP_PASS meant the
> page will be released from the pool and returned to the MM layer, but
> now it does return to the Pool with the nullified xdpf->data, which
> doesn't get reinitialized then.
> So, in addition to checking whether the head and/or tail have been
> adjusted, check also for a potential XDP frame corruption. xdpf->data
> is 100% affected and also xdpf->flags is the field closest to the
> metadata / frame start. Checking for these two should be enough for
> non-extreme cases.
>
> Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from XDP frames")
> Reported-by: [e-mail]
> Link: https://lore.kernel.org/bpf/[e-mail]
> Reported-by: Ilya Leoshkevich <[e-mail]>
> Link: https://lore.kernel.org/bpf/[e-mail]
> Signed-off-by: Alexander Lobakin <[e-mail]>

Acked-by: Toke Høiland-Jørgensen <[e-mail]>

Ilya Leoshkevich March 16, 2023, 9:21 p.m. UTC | #2

On Thu, 2023-03-16 at 18:50 +0100, Alexander Lobakin wrote:
> syzbot and Ilya faced the splats when %XDP_PASS happens for
> bpf_test_run
> after skb PP recycling was enabled for
> {__,}xdp_build_skb_from_frame():
> 
> BUG: kernel NULL pointer dereference, address: 0000000000000d28
> RIP: 0010:memset_erms+0xd/0x20 arch/x86/lib/memset_64.S:66
> [...]
> Call Trace:
>  <TASK>
>  __finalize_skb_around net/core/skbuff.c:321 [inline]
>  __build_skb_around+0x232/0x3a0 net/core/skbuff.c:379
>  build_skb_around+0x32/0x290 net/core/skbuff.c:444
>  __xdp_build_skb_from_frame+0x121/0x760 net/core/xdp.c:622
>  xdp_recv_frames net/bpf/test_run.c:248 [inline]
>  xdp_test_run_batch net/bpf/test_run.c:334 [inline]
>  bpf_test_run_xdp_live+0x1289/0x1930 net/bpf/test_run.c:362
>  bpf_prog_test_run_xdp+0xa05/0x14e0 net/bpf/test_run.c:1418
> [...]
> 
> This happens due to that it calls xdp_scrub_frame(), which nullifies
> xdpf->data. bpf_test_run code doesn't reinit the frame when the XDP
> program doesn't adjust head or tail. Previously, %XDP_PASS meant the
> page will be released from the pool and returned to the MM layer, but
> now it does return to the Pool with the nullified xdpf->data, which
> doesn't get reinitialized then.
> So, in addition to checking whether the head and/or tail have been
> adjusted, check also for a potential XDP frame corruption. xdpf->data
> is 100% affected and also xdpf->flags is the field closest to the
> metadata / frame start. Checking for these two should be enough for
> non-extreme cases.
> 
> Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from
> XDP frames")
> Reported-by: [e-mail]
> Link:
> https://lore.kernel.org/bpf/[e-mail]
> Reported-by: Ilya Leoshkevich <[e-mail]>
> Link:
> https://lore.kernel.org/bpf/[e-mail]
> Signed-off-by: Alexander Lobakin <[e-mail]>
> ---
>  net/bpf/test_run.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 71226f68270d..8d6b31209bd6 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -208,6 +208,16 @@ static void xdp_test_run_teardown(struct
> xdp_test_data *xdp)
>         kfree(xdp->skbs);
>  }
>  
> +static bool frame_was_changed(const struct xdp_page_head *head)
> +{
> +       /* xdp_scrub_frame() zeroes the data pointer, flags is the
> last field,
> +        * i.e. has the highest chances to be overwritten. If those
> two are
> +        * untouched, it's most likely safe to skip the context
> reset.
> +        */
> +       return head->frm.data != head->orig_ctx.data ||
> +              head->frm.flags != head->orig_ctx.flags;
> +}
> +
>  static bool ctx_was_changed(struct xdp_page_head *head)
>  {
>         return head->orig_ctx.data != head->ctx.data ||
> @@ -217,7 +227,7 @@ static bool ctx_was_changed(struct xdp_page_head
> *head)
>  
>  static void reset_ctx(struct xdp_page_head *head)
>  {
> -       if (likely(!ctx_was_changed(head)))
> +       if (likely(!frame_was_changed(head) &&
> !ctx_was_changed(head)))
>                 return;
>  
>         head->ctx.data = head->orig_ctx.data;

With this test begins to work on s390x:

# ./test_progs -t xdp_do_redirect
IPv6: ADDRCONF(NETDEV_CHANGE): veth_dst: link becomes ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth_src: link becomes ready
#290     xdp_do_redirect:OK
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Thanks!

Tested-by: Ilya Leoshkevich <[e-mail]>

Patch

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 71226f68270d..8d6b31209bd6 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -208,6 +208,16 @@  static void xdp_test_run_teardown(struct xdp_test_data *xdp)
 	kfree(xdp->skbs);
 }
 
+static bool frame_was_changed(const struct xdp_page_head *head)
+{
+	/* xdp_scrub_frame() zeroes the data pointer, flags is the last field,
+	 * i.e. has the highest chances to be overwritten. If those two are
+	 * untouched, it's most likely safe to skip the context reset.
+	 */
+	return head->frm.data != head->orig_ctx.data ||
+	       head->frm.flags != head->orig_ctx.flags;
+}
+
 static bool ctx_was_changed(struct xdp_page_head *head)
 {
 	return head->orig_ctx.data != head->ctx.data ||
@@ -217,7 +227,7 @@  static bool ctx_was_changed(struct xdp_page_head *head)
 
 static void reset_ctx(struct xdp_page_head *head)
 {
-	if (likely(!ctx_was_changed(head)))
+	if (likely(!frame_was_changed(head) && !ctx_was_changed(head)))
 		return;
 
 	head->ctx.data = head->orig_ctx.data;