On Mon, Dec 4, 2023 at 2:02 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 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 | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index af2819d5c8ee..a428735d232e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6816,10 +6816,7 @@ 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 - 1;
> }
>
> err = check_stack_slot_within_bounds(min_off, state, type);
> —
(Resend, forgot cc list)
Andrei, thanks for the quick fix! But with this fix, I suspect the
max_off would be incorrect when access_size is zero. We probably
should do something like this:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2a9d521b64f4..70d5201f7d08 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6556,10 +6556,9 @@ static int check_stack_access_within_bounds(
return -EACCES;
}
min_off = reg->smin_value + off;
+ max_off = reg->smax_value + off;
if (access_size > 0)
- max_off = reg->smax_value + off + access_size - 1;
- else
- max_off = min_off;
+ max_off += access_size - 1;
}
err = check_stack_slot_within_bounds(env, min_off, state, type);
