[V2,bpf,2/2] bpf: Protect against int overflow for stack access size

Commit Message

Andrei Matei March 27, 2024, 2:42 a.m. UTC

This patch re-introduces protection against the size of access to stack
memory being negative; the access size can appear negative as a result
of overflowing its signed int representation. This should not actually
happen, as there are other protections along the way, but we should
protect against it anyway. One code path was missing such protections
(fixed in the previous patch in the series), causing out-of-bounds array
accesses in check_stack_range_initialized(). This patch causes the
verification of a program with such a non-sensical access size to fail.

This check used to exist in a more indirect way, but was inadvertendly
removed in a833a17aeac7.

Fixes: a833a17aeac7 ("bpf: Fix verification of indirect var-off stack access")
Reported-by: [e-mail]
Reported-by: [e-mail]
Closes: https://lore.kernel.org/bpf/[e-mail]/
Signed-off-by: Andrei Matei <[e-mail]>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Andrii Nakryiko March 27, 2024, 4:46 p.m. UTC | #1

On Tue, Mar 26, 2024 at 7:43 PM Andrei Matei <[e-mail]> wrote:
>
> This patch re-introduces protection against the size of access to stack
> memory being negative; the access size can appear negative as a result
> of overflowing its signed int representation. This should not actually
> happen, as there are other protections along the way, but we should
> protect against it anyway. One code path was missing such protections
> (fixed in the previous patch in the series), causing out-of-bounds array
> accesses in check_stack_range_initialized(). This patch causes the
> verification of a program with such a non-sensical access size to fail.
>
> This check used to exist in a more indirect way, but was inadvertendly
> removed in a833a17aeac7.
>
> Fixes: a833a17aeac7 ("bpf: Fix verification of indirect var-off stack access")
> Reported-by: [e-mail]
> Reported-by: [e-mail]
> Closes: https://lore.kernel.org/bpf/[e-mail]/
> Signed-off-by: Andrei Matei <[e-mail]>
> ---
>  kernel/bpf/verifier.c | 5 +++++
>  1 file changed, 5 insertions(+)
>

LGTM

Acked-by: Andrii Nakryiko <[e-mail]>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0bfc0050db28..353985b2b6a2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6701,6 +6701,11 @@ static int check_stack_access_within_bounds(
>         err = check_stack_slot_within_bounds(env, min_off, state, type);
>         if (!err && max_off > 0)
>                 err = -EINVAL; /* out of stack access into non-negative offsets */
> +       if (!err && access_size < 0)
> +               /* access_size should not be negative (or overflow an int); others checks
> +                * along the way should have prevented such an access.
> +                */
> +               err = -EFAULT; /* invalid negative access size; integer overflow? */
>
>         if (err) {
>                 if (tnum_is_const(reg->var_off)) {
> --
> 2.40.1
>
>

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 0bfc0050db28..353985b2b6a2 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6701,6 +6701,11 @@  static int check_stack_access_within_bounds(
 	err = check_stack_slot_within_bounds(env, min_off, state, type);
 	if (!err && max_off > 0)
 		err = -EINVAL; /* out of stack access into non-negative offsets */
+	if (!err && access_size < 0)
+		/* access_size should not be negative (or overflow an int); others checks
+		 * along the way should have prevented such an access.
+		 */
+		err = -EFAULT; /* invalid negative access size; integer overflow? */
 
 	if (err) {
 		if (tnum_is_const(reg->var_off)) {