On Wed, Dec 6, 2023 at 8:58 AM Andrei Matei <andreimatei1@xxxxxxxxx> 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@xxxxxxxxx>
> Fixes: 01f810ace9ed3 ("bpf: Allow variable-offset stack access")
> Closes: https://lore.kernel.org/bpf/CACkBjsZGEUaRCHsmaX=h-efVogsRfK1FPxmkgb0Os_frnHiNdw@xxxxxxxxxxxxxx/
> Signed-off-by: Andrei Matei <andreimatei1@xxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> 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
>
