[PATCH bpf] bpf: fix off-by-one error in adjust_subprog_starts

[PATCH bpf] bpf: fix off-by-one error in adjust_subprog_starts

[Edward Cree]

When patching in a new sequence for the first insn of a subprog, the start
 of that subprog does not change (it's the first insn of the sequence), so
 adjust_subprog_starts should check start <= off (rather than < off).
Also added a test to test_verifier.c (it's essentially the syz reproducer).

Fixes: cc8b0b92a169 ("bpf: introduce function calls (function boundaries)")
Reported-by: syzbot+4fc427c7af994b0948be@syzkaller.appspotmail.com
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
I'm assuming I don't need to get a Signed-off-by from syzkaller to use its
 reproducer like this; I'm not an expert on the copyright niceties of works
 written by bots.

 kernel/bpf/verifier.c                       |  2 +-
 tools/testing/selftests/bpf/test_verifier.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1971ca325fb4..6dd419550aba 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5650,7 +5650,7 @@ static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len
 		return;
 	/* NOTE: fake 'exit' subprog should be updated as well. */
 	for (i = 0; i <= env->subprog_cnt; i++) {
-		if (env->subprog_info[i].start < off)
+		if (env->subprog_info[i].start <= off)
 			continue;
 		env->subprog_info[i].start += len - 1;
 	}
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 6f61df62f690..550b7e46bf4a 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -13896,6 +13896,25 @@ static struct bpf_test tests[] = {
 		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 		.result = ACCEPT,
 	},
+	{
+		"calls: ctx read at start of subprog",
+		.insns = {
+			BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 5),
+			BPF_JMP_REG(BPF_JSGT, BPF_REG_0, BPF_REG_0, 0),
+			BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+			BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
+			BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+			BPF_EXIT_INSN(),
+			BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_1, 0),
+			BPF_MOV64_IMM(BPF_REG_0, 0),
+			BPF_EXIT_INSN(),
+		},
+		.prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
+		.errstr_unpriv = "function calls to other bpf functions are allowed for root only",
+		.result_unpriv = REJECT,
+		.result = ACCEPT,
+	},
 };
 
 static int probe_filter_length(const struct bpf_insn *fp)

Re: [PATCH bpf] bpf: fix off-by-one error in adjust_subprog_starts

[Dmitry Vyukov]

On Fri, Nov 16, 2018 at 4:00 AM, Edward Cree <ecree@solarflare.com> wrote:
> When patching in a new sequence for the first insn of a subprog, the start
>  of that subprog does not change (it's the first insn of the sequence), so
>  adjust_subprog_starts should check start <= off (rather than < off).
> Also added a test to test_verifier.c (it's essentially the syz reproducer).
>
> Fixes: cc8b0b92a169 ("bpf: introduce function calls (function boundaries)")
> Reported-by: syzbot+4fc427c7af994b0948be@syzkaller.appspotmail.com
> Signed-off-by: Edward Cree <ecree@solarflare.com>
>
> ---
> I'm assuming I don't need to get a Signed-off-by from syzkaller to use its
>  reproducer like this; I'm not an expert on the copyright niceties of works
>  written by bots.

My understanding is that copyright applies only if you directly reuse
the code (e.g. copy-paste and change). Copyright does not cover ideas
nor algorithms.
The test does not look like syzkaller reproducer. If you wrote the
test yourself, it should not have any issues with copyright. But I am
not a layer too.

The intention is that you can reuse it. I don't know if/what we need
to do to make it "official".

Thanks for the quick fix.


>  kernel/bpf/verifier.c                       |  2 +-
>  tools/testing/selftests/bpf/test_verifier.c | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1971ca325fb4..6dd419550aba 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5650,7 +5650,7 @@ static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len
>                 return;
>         /* NOTE: fake 'exit' subprog should be updated as well. */
>         for (i = 0; i <= env->subprog_cnt; i++) {
> -               if (env->subprog_info[i].start < off)
> +               if (env->subprog_info[i].start <= off)
>                         continue;
>                 env->subprog_info[i].start += len - 1;
>         }
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 6f61df62f690..550b7e46bf4a 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -13896,6 +13896,25 @@ static struct bpf_test tests[] = {
>                 .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>                 .result = ACCEPT,
>         },
> +       {
> +               "calls: ctx read at start of subprog",
> +               .insns = {
> +                       BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
> +                       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 5),
> +                       BPF_JMP_REG(BPF_JSGT, BPF_REG_0, BPF_REG_0, 0),
> +                       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +                       BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 1, 0, 2),
> +                       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> +                       BPF_EXIT_INSN(),
> +                       BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_1, 0),
> +                       BPF_MOV64_IMM(BPF_REG_0, 0),
> +                       BPF_EXIT_INSN(),
> +               },
> +               .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
> +               .errstr_unpriv = "function calls to other bpf functions are allowed for root only",
> +               .result_unpriv = REJECT,
> +               .result = ACCEPT,
> +       },
>  };
>
>  static int probe_filter_length(const struct bpf_insn *fp)
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/bce0322a-6392-3fd4-a6fb-562160c26198%40solarflare.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [PATCH bpf] bpf: fix off-by-one error in adjust_subprog_starts

[Y Song]

On Fri, Nov 16, 2018 at 12:00 PM Edward Cree <ecree@solarflare.com> wrote:
>
> When patching in a new sequence for the first insn of a subprog, the start
>  of that subprog does not change (it's the first insn of the sequence), so
>  adjust_subprog_starts should check start <= off (rather than < off).
> Also added a test to test_verifier.c (it's essentially the syz reproducer).
>
> Fixes: cc8b0b92a169 ("bpf: introduce function calls (function boundaries)")
> Reported-by: syzbot+4fc427c7af994b0948be@syzkaller.appspotmail.com
> Signed-off-by: Edward Cree <ecree@solarflare.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf] bpf: fix off-by-one error in adjust_subprog_starts

[Alexei Starovoitov]

On Fri, Nov 16, 2018 at 12:00:07PM +0000, Edward Cree wrote:
> When patching in a new sequence for the first insn of a subprog, the start
>  of that subprog does not change (it's the first insn of the sequence), so
>  adjust_subprog_starts should check start <= off (rather than < off).
> Also added a test to test_verifier.c (it's essentially the syz reproducer).
> 
> Fixes: cc8b0b92a169 ("bpf: introduce function calls (function boundaries)")
> Reported-by: syzbot+4fc427c7af994b0948be@syzkaller.appspotmail.com
> Signed-off-by: Edward Cree <ecree@solarflare.com>

thanks for quick analysis and fix.
Applied, Thanks