BPF List
 help / color / mirror / Atom feed
* [PATCH bpf-next v4 0/2] bpf: fix verification of indirect var-off stack access
@ 2023-12-06 16:58 Andrei Matei
  2023-12-06 16:58 ` [PATCH bpf-next v4 1/2] " Andrei Matei
  2023-12-06 16:58 ` [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow Andrei Matei
  0 siblings, 2 replies; 7+ messages in thread
From: Andrei Matei @ 2023-12-06 16:58 UTC (permalink / raw)
  To: bpf, andrii.nakryiko, sunhao.th, eddyz87; +Cc: Andrei Matei

V3 to V4:
  - include a test per Eduard's request
  - target bpf-next per Alexei's request (patches didn't change)

V2 to V3:
  - simplify checks for max_off (don't call
    check_stack_slot_within_bounds for it)
  - append a commit to protect against overflow in the addition of the
    register and the offset

V1 to V2:
  - fix max_off calculation for access size = 0

Andrei Matei (2):
  bpf: fix verification of indirect var-off stack access
  bpf: guard stack limits against 32bit overflow

 kernel/bpf/verifier.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v4 1/2] bpf: fix verification of indirect var-off stack access
  2023-12-06 16:58 [PATCH bpf-next v4 0/2] bpf: fix verification of indirect var-off stack access Andrei Matei
@ 2023-12-06 16:58 ` Andrei Matei
  2023-12-06 17:12   ` Eduard Zingerman
  2023-12-06 18:56   ` Andrii Nakryiko
  2023-12-06 16:58 ` [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow Andrei Matei
  1 sibling, 2 replies; 7+ messages in thread
From: Andrei Matei @ 2023-12-06 16:58 UTC (permalink / raw)
  To: bpf, andrii.nakryiko, sunhao.th, eddyz87; +Cc: Andrei Matei

This patch fixes a bug around the verification of possibly-zero-sized
stack accesses. When the access was done through a var-offset stack
pointer, check_stack_access_within_bounds was incorrectly computing the
maximum-offset of a zero-sized read to be the same as the register's min
offset. Instead, we have to take in account the register's maximum
possible value. The patch also simplifies how the max offset is checked;
the check is now simpler than for min offset.

The bug was allowing accesses to erroneously pass the
check_stack_access_within_bounds() checks, only to later crash in
check_stack_range_initialized() when all the possibly-affected stack
slots are iterated (this time with a correct max offset).
check_stack_range_initialized() is relying on
check_stack_access_within_bounds() for its accesses to the
stack-tracking vector to be within bounds; in the case of zero-sized
accesses, we were essentially only verifying that the lowest possible
slot was within bounds. We would crash when the max-offset of the stack
pointer was >= 0 (which shouldn't pass verification, and hopefully is
not something anyone's code attempts to do in practice).

Thanks Hao for reporting!

Reported-by: Hao Sun <sunhao.th@gmail.com>
Fixes: 01f810ace9ed3 ("bpf: Allow variable-offset stack access")
Closes: https://lore.kernel.org/bpf/CACkBjsZGEUaRCHsmaX=h-efVogsRfK1FPxmkgb0Os_frnHiNdw@mail.gmail.com/
Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
---
 kernel/bpf/verifier.c                         | 14 +++------
 .../selftests/bpf/progs/verifier_var_off.c    | 29 +++++++++++++++++++
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e5ce530641ba..137240681fa9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6620,10 +6620,7 @@ static int check_stack_access_within_bounds(
 
 	if (tnum_is_const(reg->var_off)) {
 		min_off = reg->var_off.value + off;
-		if (access_size > 0)
-			max_off = min_off + access_size - 1;
-		else
-			max_off = min_off;
+		max_off = min_off + access_size;
 	} else {
 		if (reg->smax_value >= BPF_MAX_VAR_OFF ||
 		    reg->smin_value <= -BPF_MAX_VAR_OFF) {
@@ -6632,15 +6629,12 @@ static int check_stack_access_within_bounds(
 			return -EACCES;
 		}
 		min_off = reg->smin_value + off;
-		if (access_size > 0)
-			max_off = reg->smax_value + off + access_size - 1;
-		else
-			max_off = min_off;
+		max_off = reg->smax_value + off + access_size;
 	}
 
 	err = check_stack_slot_within_bounds(min_off, state, type);
-	if (!err)
-		err = check_stack_slot_within_bounds(max_off, state, type);
+	if (!err && max_off > 0)
+		err = -EINVAL; /* out of stack access into non-negative offsets */
 
 	if (err) {
 		if (tnum_is_const(reg->var_off)) {
diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
index 83a90afba785..9fb32b292017 100644
--- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
+++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
@@ -224,6 +224,35 @@ __naked void access_max_out_of_bound(void)
 	: __clobber_all);
 }
 
+/* Similar to the test above, but this time check the special case of a
+ * zero-sized stack access. We used to have a bug causing crashes for zero-sized
+ * out-of-bounds accesses.
+ */
+SEC("socket")
+__description("indirect variable-offset stack access, zero-sized, max out of bound")
+__failure __msg("invalid variable-offset indirect access to stack R1")
+__naked void zero_sized_access_max_out_of_bound(void)
+{
+	asm volatile ("                     \
+	r0 = 0;                             \
+	/* Fill some stack */               \
+	*(u64*)(r10 - 16) = r0;             \
+	*(u64*)(r10 - 8) = r0;              \
+	/* Get an unknown value */          \
+	r1 = *(u32*)(r1 + 0);               \
+	r1 &= 64;                           \
+	r1 += -16;                          \
+	/* r1 is now anywhere in [-16,48)*/ \
+	r1 += r10;                          \
+	r2 = 0;                             \
+	r3 = 0;                             \
+	call %[bpf_probe_read_kernel];      \
+	exit;                               \
+"	:
+	: __imm(bpf_probe_read_kernel)
+	: __clobber_all);
+}
+
 SEC("lwt_in")
 __description("indirect variable-offset stack access, min out of bound")
 __failure __msg("invalid variable-offset indirect access to stack R2")
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow
  2023-12-06 16:58 [PATCH bpf-next v4 0/2] bpf: fix verification of indirect var-off stack access Andrei Matei
  2023-12-06 16:58 ` [PATCH bpf-next v4 1/2] " Andrei Matei
@ 2023-12-06 16:58 ` Andrei Matei
  2023-12-06 19:04   ` Andrii Nakryiko
  1 sibling, 1 reply; 7+ messages in thread
From: Andrei Matei @ 2023-12-06 16:58 UTC (permalink / raw)
  To: bpf, andrii.nakryiko, sunhao.th, eddyz87; +Cc: Andrei Matei

This patch promotes the arithmetic around checking stack bounds to be
done in the 64-bit domain, instead of the current 32bit. The arithmetic
implies adding together a 64-bit register with a int offset. The
register was checked to be below 1<<29 when it was variable, but not
when it was fixed. The offset either comes from an instruction (in which
case it is 16 bit), from another register (in which case the caller
checked it to be below 1<<29 [1]), or from the size of an argument to a
kfunc (in which case it can be a u32 [2]). Between the register being
inconsistently checked to be below 1<<29, and the offset being up to an
u32, it appears that we were open to overflowing the `int`s which were
currently used for arithmetic.

[1] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L7494-L7498
[2] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L11904

Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
---
 kernel/bpf/verifier.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 137240681fa9..6832ed743765 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6577,7 +6577,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
  * The minimum valid offset is -MAX_BPF_STACK for writes, and
  * -state->allocated_stack for reads.
  */
-static int check_stack_slot_within_bounds(int off,
+static int check_stack_slot_within_bounds(s64 off,
 					  struct bpf_func_state *state,
 					  enum bpf_access_type t)
 {
@@ -6606,7 +6606,7 @@ static int check_stack_access_within_bounds(
 	struct bpf_reg_state *regs = cur_regs(env);
 	struct bpf_reg_state *reg = regs + regno;
 	struct bpf_func_state *state = func(env, reg);
-	int min_off, max_off;
+	s64 min_off, max_off;
 	int err;
 	char *err_extra;
 
@@ -6619,7 +6619,7 @@ static int check_stack_access_within_bounds(
 		err_extra = " write to";
 
 	if (tnum_is_const(reg->var_off)) {
-		min_off = reg->var_off.value + off;
+		min_off = (s64)reg->var_off.value + off;
 		max_off = min_off + access_size;
 	} else {
 		if (reg->smax_value >= BPF_MAX_VAR_OFF ||
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next v4 1/2] bpf: fix verification of indirect var-off stack access
  2023-12-06 16:58 ` [PATCH bpf-next v4 1/2] " Andrei Matei
@ 2023-12-06 17:12   ` Eduard Zingerman
  2023-12-06 18:56   ` Andrii Nakryiko
  1 sibling, 0 replies; 7+ messages in thread
From: Eduard Zingerman @ 2023-12-06 17:12 UTC (permalink / raw)
  To: Andrei Matei, bpf, andrii.nakryiko, sunhao.th

On Wed, 2023-12-06 at 11:58 -0500, Andrei Matei wrote:
[...]
> diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c

You would probably be asked to split this patch in two.
Usually selftests are submitted as separate patches with
'selftests/bpf:' tag. Tests are updated in 'bpf:' patches only if
changes to verifier make some tests invalid (so that it is possible
to do bisects over commit ranges).

Otherwise, lgtm, thank you for adding the test and please add my ack
for the test if v5 would be submitted.

> index 83a90afba785..9fb32b292017 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> @@ -224,6 +224,35 @@ __naked void access_max_out_of_bound(void)
>  	: __clobber_all);
>  }
>  
> +/* Similar to the test above, but this time check the special case of a
> + * zero-sized stack access. We used to have a bug causing crashes for zero-sized
> + * out-of-bounds accesses.
> + */
> +SEC("socket")
> +__description("indirect variable-offset stack access, zero-sized, max out of bound")
> +__failure __msg("invalid variable-offset indirect access to stack R1")
> +__naked void zero_sized_access_max_out_of_bound(void)
> +{
> +	asm volatile ("                     \
> +	r0 = 0;                             \
> +	/* Fill some stack */               \
> +	*(u64*)(r10 - 16) = r0;             \
> +	*(u64*)(r10 - 8) = r0;              \
> +	/* Get an unknown value */          \
> +	r1 = *(u32*)(r1 + 0);               \
> +	r1 &= 64;                           \
> +	r1 += -16;                          \
> +	/* r1 is now anywhere in [-16,48)*/ \
> +	r1 += r10;                          \
> +	r2 = 0;                             \
> +	r3 = 0;                             \
> +	call %[bpf_probe_read_kernel];      \
> +	exit;                               \
> +"	:
> +	: __imm(bpf_probe_read_kernel)
> +	: __clobber_all);
> +}
> +
>  SEC("lwt_in")
>  __description("indirect variable-offset stack access, min out of bound")
>  __failure __msg("invalid variable-offset indirect access to stack R2")



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next v4 1/2] bpf: fix verification of indirect var-off stack access
  2023-12-06 16:58 ` [PATCH bpf-next v4 1/2] " Andrei Matei
  2023-12-06 17:12   ` Eduard Zingerman
@ 2023-12-06 18:56   ` Andrii Nakryiko
  2023-12-07  3:30     ` Andrei Matei
  1 sibling, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 18:56 UTC (permalink / raw)
  To: Andrei Matei; +Cc: bpf, sunhao.th, eddyz87

On Wed, Dec 6, 2023 at 8:58 AM Andrei Matei <andreimatei1@gmail.com> wrote:
>
> This patch fixes a bug around the verification of possibly-zero-sized
> stack accesses. When the access was done through a var-offset stack
> pointer, check_stack_access_within_bounds was incorrectly computing the
> maximum-offset of a zero-sized read to be the same as the register's min
> offset. Instead, we have to take in account the register's maximum
> possible value. The patch also simplifies how the max offset is checked;
> the check is now simpler than for min offset.
>
> The bug was allowing accesses to erroneously pass the
> check_stack_access_within_bounds() checks, only to later crash in
> check_stack_range_initialized() when all the possibly-affected stack
> slots are iterated (this time with a correct max offset).
> check_stack_range_initialized() is relying on
> check_stack_access_within_bounds() for its accesses to the
> stack-tracking vector to be within bounds; in the case of zero-sized
> accesses, we were essentially only verifying that the lowest possible
> slot was within bounds. We would crash when the max-offset of the stack
> pointer was >= 0 (which shouldn't pass verification, and hopefully is
> not something anyone's code attempts to do in practice).
>
> Thanks Hao for reporting!
>
> Reported-by: Hao Sun <sunhao.th@gmail.com>
> Fixes: 01f810ace9ed3 ("bpf: Allow variable-offset stack access")
> Closes: https://lore.kernel.org/bpf/CACkBjsZGEUaRCHsmaX=h-efVogsRfK1FPxmkgb0Os_frnHiNdw@mail.gmail.com/
> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> ---
>  kernel/bpf/verifier.c                         | 14 +++------
>  .../selftests/bpf/progs/verifier_var_off.c    | 29 +++++++++++++++++++
>  2 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e5ce530641ba..137240681fa9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6620,10 +6620,7 @@ static int check_stack_access_within_bounds(
>
>         if (tnum_is_const(reg->var_off)) {
>                 min_off = reg->var_off.value + off;
> -               if (access_size > 0)
> -                       max_off = min_off + access_size - 1;
> -               else
> -                       max_off = min_off;
> +               max_off = min_off + access_size;
>         } else {
>                 if (reg->smax_value >= BPF_MAX_VAR_OFF ||
>                     reg->smin_value <= -BPF_MAX_VAR_OFF) {
> @@ -6632,15 +6629,12 @@ static int check_stack_access_within_bounds(
>                         return -EACCES;
>                 }
>                 min_off = reg->smin_value + off;
> -               if (access_size > 0)
> -                       max_off = reg->smax_value + off + access_size - 1;
> -               else
> -                       max_off = min_off;
> +               max_off = reg->smax_value + off + access_size;
>         }
>
>         err = check_stack_slot_within_bounds(min_off, state, type);
> -       if (!err)
> -               err = check_stack_slot_within_bounds(max_off, state, type);
> +       if (!err && max_off > 0)
> +               err = -EINVAL; /* out of stack access into non-negative offsets */
>

this part looks good to me, please add my ack on resubmission

Acked-by: Andrii Nakryiko <andrii@kernel.org>


>         if (err) {
>                 if (tnum_is_const(reg->var_off)) {
> diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> index 83a90afba785..9fb32b292017 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> @@ -224,6 +224,35 @@ __naked void access_max_out_of_bound(void)
>         : __clobber_all);
>  }
>
> +/* Similar to the test above, but this time check the special case of a
> + * zero-sized stack access. We used to have a bug causing crashes for zero-sized
> + * out-of-bounds accesses.
> + */
> +SEC("socket")
> +__description("indirect variable-offset stack access, zero-sized, max out of bound")
> +__failure __msg("invalid variable-offset indirect access to stack R1")
> +__naked void zero_sized_access_max_out_of_bound(void)

as Eduard mentioned, please split off selftests from kernel-side changes

> +{
> +       asm volatile ("                     \
> +       r0 = 0;                             \
> +       /* Fill some stack */               \
> +       *(u64*)(r10 - 16) = r0;             \
> +       *(u64*)(r10 - 8) = r0;              \
> +       /* Get an unknown value */          \
> +       r1 = *(u32*)(r1 + 0);               \
> +       r1 &= 64;                           \

did you mean 63 here? and if yes, why does the test work? :)

> +       r1 += -16;                          \
> +       /* r1 is now anywhere in [-16,48)*/ \

nit: space before */ ?

> +       r1 += r10;                          \
> +       r2 = 0;                             \
> +       r3 = 0;                             \
> +       call %[bpf_probe_read_kernel];      \
> +       exit;                               \
> +"      :
> +       : __imm(bpf_probe_read_kernel)
> +       : __clobber_all);
> +}
> +
>  SEC("lwt_in")
>  __description("indirect variable-offset stack access, min out of bound")
>  __failure __msg("invalid variable-offset indirect access to stack R2")
> --
> 2.39.2
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow
  2023-12-06 16:58 ` [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow Andrei Matei
@ 2023-12-06 19:04   ` Andrii Nakryiko
  0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2023-12-06 19:04 UTC (permalink / raw)
  To: Andrei Matei; +Cc: bpf, sunhao.th, eddyz87

On Wed, Dec 6, 2023 at 8:58 AM Andrei Matei <andreimatei1@gmail.com> wrote:
>
> This patch promotes the arithmetic around checking stack bounds to be
> done in the 64-bit domain, instead of the current 32bit. The arithmetic
> implies adding together a 64-bit register with a int offset. The
> register was checked to be below 1<<29 when it was variable, but not
> when it was fixed. The offset either comes from an instruction (in which
> case it is 16 bit), from another register (in which case the caller
> checked it to be below 1<<29 [1]), or from the size of an argument to a
> kfunc (in which case it can be a u32 [2]). Between the register being
> inconsistently checked to be below 1<<29, and the offset being up to an
> u32, it appears that we were open to overflowing the `int`s which were
> currently used for arithmetic.
>
> [1] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L7494-L7498
> [2] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L11904
>
> Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> ---
>  kernel/bpf/verifier.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>

LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 137240681fa9..6832ed743765 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6577,7 +6577,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
>   * The minimum valid offset is -MAX_BPF_STACK for writes, and
>   * -state->allocated_stack for reads.
>   */
> -static int check_stack_slot_within_bounds(int off,
> +static int check_stack_slot_within_bounds(s64 off,
>                                           struct bpf_func_state *state,
>                                           enum bpf_access_type t)
>  {
> @@ -6606,7 +6606,7 @@ static int check_stack_access_within_bounds(
>         struct bpf_reg_state *regs = cur_regs(env);
>         struct bpf_reg_state *reg = regs + regno;
>         struct bpf_func_state *state = func(env, reg);
> -       int min_off, max_off;
> +       s64 min_off, max_off;
>         int err;
>         char *err_extra;
>
> @@ -6619,7 +6619,7 @@ static int check_stack_access_within_bounds(
>                 err_extra = " write to";
>
>         if (tnum_is_const(reg->var_off)) {
> -               min_off = reg->var_off.value + off;
> +               min_off = (s64)reg->var_off.value + off;
>                 max_off = min_off + access_size;
>         } else {
>                 if (reg->smax_value >= BPF_MAX_VAR_OFF ||
> --
> 2.39.2
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next v4 1/2] bpf: fix verification of indirect var-off stack access
  2023-12-06 18:56   ` Andrii Nakryiko
@ 2023-12-07  3:30     ` Andrei Matei
  0 siblings, 0 replies; 7+ messages in thread
From: Andrei Matei @ 2023-12-07  3:30 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, sunhao.th, eddyz87

On Wed, Dec 6, 2023 at 1:56 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Dec 6, 2023 at 8:58 AM Andrei Matei <andreimatei1@gmail.com> wrote:
> >
> > This patch fixes a bug around the verification of possibly-zero-sized
> > stack accesses. When the access was done through a var-offset stack
> > pointer, check_stack_access_within_bounds was incorrectly computing the
> > maximum-offset of a zero-sized read to be the same as the register's min
> > offset. Instead, we have to take in account the register's maximum
> > possible value. The patch also simplifies how the max offset is checked;
> > the check is now simpler than for min offset.
> >
> > The bug was allowing accesses to erroneously pass the
> > check_stack_access_within_bounds() checks, only to later crash in
> > check_stack_range_initialized() when all the possibly-affected stack
> > slots are iterated (this time with a correct max offset).
> > check_stack_range_initialized() is relying on
> > check_stack_access_within_bounds() for its accesses to the
> > stack-tracking vector to be within bounds; in the case of zero-sized
> > accesses, we were essentially only verifying that the lowest possible
> > slot was within bounds. We would crash when the max-offset of the stack
> > pointer was >= 0 (which shouldn't pass verification, and hopefully is
> > not something anyone's code attempts to do in practice).
> >
> > Thanks Hao for reporting!
> >
> > Reported-by: Hao Sun <sunhao.th@gmail.com>
> > Fixes: 01f810ace9ed3 ("bpf: Allow variable-offset stack access")
> > Closes: https://lore.kernel.org/bpf/CACkBjsZGEUaRCHsmaX=h-efVogsRfK1FPxmkgb0Os_frnHiNdw@mail.gmail.com/
> > Signed-off-by: Andrei Matei <andreimatei1@gmail.com>
> > ---
> >  kernel/bpf/verifier.c                         | 14 +++------
> >  .../selftests/bpf/progs/verifier_var_off.c    | 29 +++++++++++++++++++
> >  2 files changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index e5ce530641ba..137240681fa9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6620,10 +6620,7 @@ static int check_stack_access_within_bounds(
> >
> >         if (tnum_is_const(reg->var_off)) {
> >                 min_off = reg->var_off.value + off;
> > -               if (access_size > 0)
> > -                       max_off = min_off + access_size - 1;
> > -               else
> > -                       max_off = min_off;
> > +               max_off = min_off + access_size;
> >         } else {
> >                 if (reg->smax_value >= BPF_MAX_VAR_OFF ||
> >                     reg->smin_value <= -BPF_MAX_VAR_OFF) {
> > @@ -6632,15 +6629,12 @@ static int check_stack_access_within_bounds(
> >                         return -EACCES;
> >                 }
> >                 min_off = reg->smin_value + off;
> > -               if (access_size > 0)
> > -                       max_off = reg->smax_value + off + access_size - 1;
> > -               else
> > -                       max_off = min_off;
> > +               max_off = reg->smax_value + off + access_size;
> >         }
> >
> >         err = check_stack_slot_within_bounds(min_off, state, type);
> > -       if (!err)
> > -               err = check_stack_slot_within_bounds(max_off, state, type);
> > +       if (!err && max_off > 0)
> > +               err = -EINVAL; /* out of stack access into non-negative offsets */
> >
>
> this part looks good to me, please add my ack on resubmission
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>
> >         if (err) {
> >                 if (tnum_is_const(reg->var_off)) {
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_var_off.c b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> > index 83a90afba785..9fb32b292017 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_var_off.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_var_off.c
> > @@ -224,6 +224,35 @@ __naked void access_max_out_of_bound(void)
> >         : __clobber_all);
> >  }
> >
> > +/* Similar to the test above, but this time check the special case of a
> > + * zero-sized stack access. We used to have a bug causing crashes for zero-sized
> > + * out-of-bounds accesses.
> > + */
> > +SEC("socket")
> > +__description("indirect variable-offset stack access, zero-sized, max out of bound")
> > +__failure __msg("invalid variable-offset indirect access to stack R1")
> > +__naked void zero_sized_access_max_out_of_bound(void)
>
> as Eduard mentioned, please split off selftests from kernel-side changes
>
> > +{
> > +       asm volatile ("                     \
> > +       r0 = 0;                             \
> > +       /* Fill some stack */               \
> > +       *(u64*)(r10 - 16) = r0;             \
> > +       *(u64*)(r10 - 8) = r0;              \
> > +       /* Get an unknown value */          \
> > +       r1 = *(u32*)(r1 + 0);               \
> > +       r1 &= 64;                           \
>
> did you mean 63 here? and if yes, why does the test work? :)

I did mean 63, thanks. But the test worked either way, not much difference.
`r1 &= 64` gives you a positive value that's either 0 or 64 (i.e. all the bits
except one are known), so for the relevant verification code path, it's pretty
equivalent to [0, 64) -- all that matters are the bounds.


>
> > +       r1 += -16;                          \
> > +       /* r1 is now anywhere in [-16,48)*/ \
>
> nit: space before */ ?
>
> > +       r1 += r10;                          \
> > +       r2 = 0;                             \
> > +       r3 = 0;                             \
> > +       call %[bpf_probe_read_kernel];      \
> > +       exit;                               \
> > +"      :
> > +       : __imm(bpf_probe_read_kernel)
> > +       : __clobber_all);
> > +}
> > +
> >  SEC("lwt_in")
> >  __description("indirect variable-offset stack access, min out of bound")
> >  __failure __msg("invalid variable-offset indirect access to stack R2")
> > --
> > 2.39.2
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-12-07  3:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-06 16:58 [PATCH bpf-next v4 0/2] bpf: fix verification of indirect var-off stack access Andrei Matei
2023-12-06 16:58 ` [PATCH bpf-next v4 1/2] " Andrei Matei
2023-12-06 17:12   ` Eduard Zingerman
2023-12-06 18:56   ` Andrii Nakryiko
2023-12-07  3:30     ` Andrei Matei
2023-12-06 16:58 ` [PATCH bpf-next v4 2/2] bpf: guard stack limits against 32bit overflow Andrei Matei
2023-12-06 19:04   ` Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox