On Tue, Mar 26, 2024 at 7:43 PM Andrei Matei <andreimatei1@xxxxxxxxx> 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: syzbot+33f4297b5f927648741a@xxxxxxxxxxxxxxxxxxxxxxxxx
> Reported-by: syzbot+aafd0513053a1cbf52ef@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/bpf/CAADnVQLORV5PT0iTAhRER+iLBTkByCYNBYyvBSgjN1T31K+gOw@xxxxxxxxxxxxxx/
> Signed-off-by: Andrei Matei <andreimatei1@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
LGTM
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> 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
>
>
