All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls
  2020-12-11  3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song
@ 2020-12-11  3:41 ` Yonghong Song
  2020-12-14 21:26   ` Daniel Borkmann
  2020-12-11  3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song
  1 sibling, 1 reply; 5+ messages in thread
From: Yonghong Song @ 2020-12-11  3:41 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu

Currently, when checking stack memory accessed by helper calls,
for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are
allowed.

Song discovered an issue where the below bpf program
  int dump_task(struct bpf_iter__task *ctx)
  {
    struct seq_file *seq = ctx->meta->seq;
    static char[] info = "abc";
    BPF_SEQ_PRINTF(seq, "%s\n", info);
    return 0;
  }
may cause a verifier failure.

The verifier output looks like:
  ; struct seq_file *seq = ctx->meta->seq;
  1: (79) r1 = *(u64 *)(r1 +0)
  ; BPF_SEQ_PRINTF(seq, "%s\n", info);
  2: (18) r2 = 0xffff9054400f6000
  4: (7b) *(u64 *)(r10 -8) = r2
  5: (bf) r4 = r10
  ;
  6: (07) r4 += -8
  ; BPF_SEQ_PRINTF(seq, "%s\n", info);
  7: (18) r2 = 0xffff9054400fe000
  9: (b4) w3 = 4
  10: (b4) w5 = 8
  11: (85) call bpf_seq_printf#126
   R1_w=ptr_seq_file(id=0,off=0,imm=0) R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0)
  R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value
  last_idx 11 first_idx 0
  regs=8 stack=0 before 10: (b4) w5 = 8
  regs=8 stack=0 before 9: (b4) w3 = 4
  invalid indirect read from stack off -8+0 size 8

Basically, the verifier complains the map_value pointer at "fp-8" location.
To fix the issue, if env->allow_ptr_leaks is true, let us also permit
pointers on the stack to be accessible by the helper.

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Reported-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/verifier.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 93def76cf32b..eebb2d3e16bf 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
 			goto mark;
 
 		if (state->stack[spi].slot_type[0] == STACK_SPILL &&
-		    state->stack[spi].spilled_ptr.type == SCALAR_VALUE) {
+		    (state->stack[spi].spilled_ptr.type == SCALAR_VALUE ||
+		     (state->stack[spi].spilled_ptr.type != NOT_INIT &&
+		      env->allow_ptr_leaks))) {
 			__mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
 			for (j = 0; j < BPF_REG_SIZE; j++)
 				state->stack[spi].slot_type[j] = STACK_MISC;
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls
@ 2020-12-11  3:41 Yonghong Song
  2020-12-11  3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song
  2020-12-11  3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song
  0 siblings, 2 replies; 5+ messages in thread
From: Yonghong Song @ 2020-12-11  3:41 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

This patch permits pointers on stack for helper calls if permission is
granted. Patch #1 described the detailed usecase and Patch #2
added a test.

Changelog:
  v2 -> v3:
    - do not permit spilled reg state NOT_INIT on stack. (Daniel)
  v1 -> v2:
    - fix a verifier test failure due to verifier change.

Yonghong Song (2):
  bpf: permits pointers on stack for helper calls
  selftests/bpf: add a test for ptr_to_map_value on stack for helper
    access

 kernel/bpf/verifier.c                             | 4 +++-
 tools/testing/selftests/bpf/progs/bpf_iter_task.c | 3 ++-
 tools/testing/selftests/bpf/verifier/unpriv.c     | 5 +++--
 3 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access
  2020-12-11  3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song
  2020-12-11  3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song
@ 2020-12-11  3:41 ` Yonghong Song
  1 sibling, 0 replies; 5+ messages in thread
From: Yonghong Song @ 2020-12-11  3:41 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Song Liu

Change bpf_iter_task.c such that pointer to map_value may appear
on the stack for bpf_seq_printf() to access. Without previous
verifier patch, the bpf_iter test will fail.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/progs/bpf_iter_task.c | 3 ++-
 tools/testing/selftests/bpf/verifier/unpriv.c     | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_task.c b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
index 4983087852a0..b7f32c160f4e 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_task.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_task.c
@@ -11,9 +11,10 @@ int dump_task(struct bpf_iter__task *ctx)
 {
 	struct seq_file *seq = ctx->meta->seq;
 	struct task_struct *task = ctx->task;
+	static char info[] = "    === END ===";
 
 	if (task == (void *)0) {
-		BPF_SEQ_PRINTF(seq, "    === END ===\n");
+		BPF_SEQ_PRINTF(seq, "%s\n", info);
 		return 0;
 	}
 
diff --git a/tools/testing/selftests/bpf/verifier/unpriv.c b/tools/testing/selftests/bpf/verifier/unpriv.c
index 91bb77c24a2e..a3fe0fbaed41 100644
--- a/tools/testing/selftests/bpf/verifier/unpriv.c
+++ b/tools/testing/selftests/bpf/verifier/unpriv.c
@@ -108,8 +108,9 @@
 	BPF_EXIT_INSN(),
 	},
 	.fixup_map_hash_8b = { 3 },
-	.errstr = "invalid indirect read from stack off -8+0 size 8",
-	.result = REJECT,
+	.errstr_unpriv = "invalid indirect read from stack off -8+0 size 8",
+	.result_unpriv = REJECT,
+	.result = ACCEPT,
 },
 {
 	"unpriv: mangle pointer on stack 1",
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls
  2020-12-11  3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song
@ 2020-12-14 21:26   ` Daniel Borkmann
  2020-12-15  3:57     ` Yonghong Song
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Borkmann @ 2020-12-14 21:26 UTC (permalink / raw)
  To: Yonghong Song, bpf; +Cc: Alexei Starovoitov, kernel-team, Song Liu

On 12/11/20 4:41 AM, Yonghong Song wrote:
> Currently, when checking stack memory accessed by helper calls,
> for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are
> allowed.
> 
> Song discovered an issue where the below bpf program
>    int dump_task(struct bpf_iter__task *ctx)
>    {
>      struct seq_file *seq = ctx->meta->seq;
>      static char[] info = "abc";
>      BPF_SEQ_PRINTF(seq, "%s\n", info);
>      return 0;
>    }
> may cause a verifier failure.
> 
> The verifier output looks like:
>    ; struct seq_file *seq = ctx->meta->seq;
>    1: (79) r1 = *(u64 *)(r1 +0)
>    ; BPF_SEQ_PRINTF(seq, "%s\n", info);
>    2: (18) r2 = 0xffff9054400f6000
>    4: (7b) *(u64 *)(r10 -8) = r2
>    5: (bf) r4 = r10
>    ;
>    6: (07) r4 += -8
>    ; BPF_SEQ_PRINTF(seq, "%s\n", info);
>    7: (18) r2 = 0xffff9054400fe000
>    9: (b4) w3 = 4
>    10: (b4) w5 = 8
>    11: (85) call bpf_seq_printf#126
>     R1_w=ptr_seq_file(id=0,off=0,imm=0) R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0)
>    R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value
>    last_idx 11 first_idx 0
>    regs=8 stack=0 before 10: (b4) w5 = 8
>    regs=8 stack=0 before 9: (b4) w3 = 4
>    invalid indirect read from stack off -8+0 size 8
> 
> Basically, the verifier complains the map_value pointer at "fp-8" location.
> To fix the issue, if env->allow_ptr_leaks is true, let us also permit
> pointers on the stack to be accessible by the helper.
> 
> Suggested-by: Alexei Starovoitov <ast@kernel.org>
> Reported-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>   kernel/bpf/verifier.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 93def76cf32b..eebb2d3e16bf 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct bpf_verifier_env *env, int regno,
>   			goto mark;
>   
>   		if (state->stack[spi].slot_type[0] == STACK_SPILL &&
> -		    state->stack[spi].spilled_ptr.type == SCALAR_VALUE) {
> +		    (state->stack[spi].spilled_ptr.type == SCALAR_VALUE ||
> +		     (state->stack[spi].spilled_ptr.type != NOT_INIT &&

Thinking more on this, your v2 was actually correct since in such case stype
would have been STACK_MISC or STACK_ZERO and we would have jumped to goto mark
here instead, so the above is not reachable under NOT_INIT. Anyway, I took the
v2 in, thanks!

> +		      env->allow_ptr_leaks))) {
>   			__mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
>   			for (j = 0; j < BPF_REG_SIZE; j++)
>   				state->stack[spi].slot_type[j] = STACK_MISC;
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: permits pointers on stack for helper calls
  2020-12-14 21:26   ` Daniel Borkmann
@ 2020-12-15  3:57     ` Yonghong Song
  0 siblings, 0 replies; 5+ messages in thread
From: Yonghong Song @ 2020-12-15  3:57 UTC (permalink / raw)
  To: Daniel Borkmann, bpf; +Cc: Alexei Starovoitov, kernel-team, Song Liu



On 12/14/20 1:26 PM, Daniel Borkmann wrote:
> On 12/11/20 4:41 AM, Yonghong Song wrote:
>> Currently, when checking stack memory accessed by helper calls,
>> for spills, only PTR_TO_BTF_ID and SCALAR_VALUE are
>> allowed.
>>
>> Song discovered an issue where the below bpf program
>>    int dump_task(struct bpf_iter__task *ctx)
>>    {
>>      struct seq_file *seq = ctx->meta->seq;
>>      static char[] info = "abc";
>>      BPF_SEQ_PRINTF(seq, "%s\n", info);
>>      return 0;
>>    }
>> may cause a verifier failure.
>>
>> The verifier output looks like:
>>    ; struct seq_file *seq = ctx->meta->seq;
>>    1: (79) r1 = *(u64 *)(r1 +0)
>>    ; BPF_SEQ_PRINTF(seq, "%s\n", info);
>>    2: (18) r2 = 0xffff9054400f6000
>>    4: (7b) *(u64 *)(r10 -8) = r2
>>    5: (bf) r4 = r10
>>    ;
>>    6: (07) r4 += -8
>>    ; BPF_SEQ_PRINTF(seq, "%s\n", info);
>>    7: (18) r2 = 0xffff9054400fe000
>>    9: (b4) w3 = 4
>>    10: (b4) w5 = 8
>>    11: (85) call bpf_seq_printf#126
>>     R1_w=ptr_seq_file(id=0,off=0,imm=0) 
>> R2_w=map_value(id=0,off=0,ks=4,vs=4,imm=0)
>>    R3_w=inv4 R4_w=fp-8 R5_w=inv8 R10=fp0 fp-8_w=map_value
>>    last_idx 11 first_idx 0
>>    regs=8 stack=0 before 10: (b4) w5 = 8
>>    regs=8 stack=0 before 9: (b4) w3 = 4
>>    invalid indirect read from stack off -8+0 size 8
>>
>> Basically, the verifier complains the map_value pointer at "fp-8" 
>> location.
>> To fix the issue, if env->allow_ptr_leaks is true, let us also permit
>> pointers on the stack to be accessible by the helper.
>>
>> Suggested-by: Alexei Starovoitov <ast@kernel.org>
>> Reported-by: Song Liu <songliubraving@fb.com>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   kernel/bpf/verifier.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 93def76cf32b..eebb2d3e16bf 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -3769,7 +3769,9 @@ static int check_stack_boundary(struct 
>> bpf_verifier_env *env, int regno,
>>               goto mark;
>>           if (state->stack[spi].slot_type[0] == STACK_SPILL &&
>> -            state->stack[spi].spilled_ptr.type == SCALAR_VALUE) {
>> +            (state->stack[spi].spilled_ptr.type == SCALAR_VALUE ||
>> +             (state->stack[spi].spilled_ptr.type != NOT_INIT &&
> 
> Thinking more on this, your v2 was actually correct since in such case 
> stype
> would have been STACK_MISC or STACK_ZERO and we would have jumped to 
> goto mark
> here instead, so the above is not reachable under NOT_INIT. Anyway, I 
> took the
> v2 in, thanks!

Thanks! I missed this (NOT_INIT is associated with STACK_MISC/STACK_ZERO 
spill type) too.

> 
>> +              env->allow_ptr_leaks))) {
>>               __mark_reg_unknown(env, &state->stack[spi].spilled_ptr);
>>               for (j = 0; j < BPF_REG_SIZE; j++)
>>                   state->stack[spi].slot_type[j] = STACK_MISC;
>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-15  3:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11  3:41 [PATCH bpf-next v3 0/2] bpf: permits pointers on stack for helper calls Yonghong Song
2020-12-11  3:41 ` [PATCH bpf-next v3 1/2] " Yonghong Song
2020-12-14 21:26   ` Daniel Borkmann
2020-12-15  3:57     ` Yonghong Song
2020-12-11  3:41 ` [PATCH bpf-next v3 2/2] selftests/bpf: add a test for ptr_to_map_value on stack for helper access Yonghong Song

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.