[PATCH stable 4.19,4.20] bpf: fix sanitation rewrite in case of non-pointers

[PATCH stable 4.19,4.20] bpf: fix sanitation rewrite in case of non-pointers

[Daniel Borkmann]

commit 3612af783cf52c74a031a2f11b82247b2599d3cd upstream.

Marek reported that he saw an issue with the below snippet in that
timing measurements where off when loaded as unpriv while results
were reasonable when loaded as privileged:

    [...]
    uint64_t a = bpf_ktime_get_ns();
    uint64_t b = bpf_ktime_get_ns();
    uint64_t delta = b - a;
    if ((int64_t)delta > 0) {
    [...]

Turns out there is a bug where a corner case is missing in the fix
d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
type from different paths"), namely fixup_bpf_calls() only checks
whether aux has a non-zero alu_state, but it also needs to test for
the case of BPF_ALU_NON_POINTER since in both occasions we need to
skip the masking rewrite (as there is nothing to mask).

Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
Reported-by: Marek Majkowski <marek@cloudflare.com>
Reported-by: Arthur Fabre <afabre@cloudflare.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4d81be2..bcb42aa 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6035,7 +6035,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
 			u32 off_reg;
 
 			aux = &env->insn_aux_data[i + delta];
-			if (!aux->alu_state)
+			if (!aux->alu_state ||
+			    aux->alu_state == BPF_ALU_NON_POINTER)
 				continue;
 
 			isneg = aux->alu_state & BPF_ALU_NEG_VALUE;
-- 
2.9.5

Re: [PATCH stable 4.19,4.20] bpf: fix sanitation rewrite in case of non-pointers

[Greg KH]

On Mon, Mar 04, 2019 at 01:38:56PM +0100, Daniel Borkmann wrote:
> commit 3612af783cf52c74a031a2f11b82247b2599d3cd upstream.
> 
> Marek reported that he saw an issue with the below snippet in that
> timing measurements where off when loaded as unpriv while results
> were reasonable when loaded as privileged:
> 
>     [...]
>     uint64_t a = bpf_ktime_get_ns();
>     uint64_t b = bpf_ktime_get_ns();
>     uint64_t delta = b - a;
>     if ((int64_t)delta > 0) {
>     [...]
> 
> Turns out there is a bug where a corner case is missing in the fix
> d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar
> type from different paths"), namely fixup_bpf_calls() only checks
> whether aux has a non-zero alu_state, but it also needs to test for
> the case of BPF_ALU_NON_POINTER since in both occasions we need to
> skip the masking rewrite (as there is nothing to mask).
> 
> Fixes: d3bd7413e0ca ("bpf: fix sanitation of alu op with pointer / scalar type from different paths")
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Reported-by: Arthur Fabre <afabre@cloudflare.com>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Link: https://lore.kernel.org/netdev/CAJPywTJqP34cK20iLM5YmUMz9KXQOdu1-+BZrGMAGgLuBWz7fg@mail.gmail.com/T/
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  kernel/bpf/verifier.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4d81be2..bcb42aa 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6035,7 +6035,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>  			u32 off_reg;
>  
>  			aux = &env->insn_aux_data[i + delta];
> -			if (!aux->alu_state)
> +			if (!aux->alu_state ||
> +			    aux->alu_state == BPF_ALU_NON_POINTER)
>  				continue;
>  
>  			isneg = aux->alu_state & BPF_ALU_NEG_VALUE;
> -- 
> 2.9.5
> 

Thanks, now queued up.

greg k-h