netdev - Re: [PATCH net 1/2] bpf/verifier: fix bounds calculation on BPF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez15EV+p+XVpNA=M4fE7b1Y=v5mO-4y=Z3GJydQSaU+0Og@mail.gmail.com>
Date:   Tue, 5 Dec 2017 20:35:24 +0100
From:   Jann Horn <jannh@...gle.com>
To:     Edward Cree <ecree@...arflare.com>
Cc:     davem <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH net 1/2] bpf/verifier: fix bounds calculation on BPF_RSH

On Tue, Dec 5, 2017 at 8:15 PM, Edward Cree <ecree@...arflare.com> wrote:
> Incorrect signed bounds were being computed, although this had no effect
>  since the propagation in __reg_deduce_bounds() happened to overwrite them.
>
> Fixes: b03c9f9fdc37 ("bpf/verifier: track signed and unsigned min/max values")
> Reported-by: Jann Horn <jannh@...gle.com>
> Signed-off-by: Edward Cree <ecree@...arflare.com>
> ---
>  kernel/bpf/verifier.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d4593571c404..5bed7f773c87 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2184,20 +2184,22 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
>                         mark_reg_unknown(env, regs, insn->dst_reg);
>                         break;
>                 }
> -               /* BPF_RSH is an unsigned shift, so make the appropriate casts */
> -               if (dst_reg->smin_value < 0) {
> -                       if (umin_val) {
> -                               /* Sign bit will be cleared */
> -                               dst_reg->smin_value = 0;
> -                       } else {
> -                               /* Lost sign bit information */
> -                               dst_reg->smin_value = S64_MIN;
> -                               dst_reg->smax_value = S64_MAX;
> -                       }
> -               } else {
> -                       dst_reg->smin_value =
> -                               (u64)(dst_reg->smin_value) >> umax_val;
> -               }
> +               /* BPF_RSH is an unsigned shift.  If the value in dst_reg might
> +                * be negative, then either:
> +                * 1) src_reg might be zero, so the sign bit of the result is
> +                *    unknown, so we lose our signed bounds
> +                * 2) it's known negative, thus the unsigned bounds capture the
> +                *    signed bounds
> +                * 3) the signed bounds cross zero, so they tell us nothing
> +                *    about the result
> +                * If the value in dst_reg is known nonnegative, then again the
> +                * unsigned bounts capture the signed bounds.
> +                * Thus, in all cases it suffices to blow away our signed bounds
> +                * and rely on inferring new ones from the unsigned bounds and
> +                * var_off of the result.
> +                */
> +               dst_reg->smin_value = S64_MIN;
> +               dst_reg->smax_value = S64_MAX;
>                 if (src_known)
>                         dst_reg->var_off = tnum_rshift(dst_reg->var_off,
>                                                        umin_val);
>

Reviewed-by: Jann Horn <jannh@...gle.com>