Commit 4374f256 authored by Edward Cree's avatar Edward Cree Committed by Daniel Borkmann

bpf/verifier: fix bounds calculation on BPF_RSH

Incorrect signed bounds were being computed.
If the old upper signed bound was positive and the old lower signed bound was
negative, this could cause the new upper signed bound to be too low,
leading to security issues.

Fixes: b03c9f9f ("bpf/verifier: track signed and unsigned min/max values")
Reported-by: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarEdward Cree <ecree@solarflare.com>
Acked-by: default avatarAlexei Starovoitov <ast@kernel.org>
[jannh@google.com: changed description to reflect bug impact]
Signed-off-by: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
parent 19c832ed
...@@ -2190,20 +2190,22 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, ...@@ -2190,20 +2190,22 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
mark_reg_unknown(env, regs, insn->dst_reg); mark_reg_unknown(env, regs, insn->dst_reg);
break; break;
} }
/* BPF_RSH is an unsigned shift, so make the appropriate casts */ /* BPF_RSH is an unsigned shift. If the value in dst_reg might
if (dst_reg->smin_value < 0) { * be negative, then either:
if (umin_val) { * 1) src_reg might be zero, so the sign bit of the result is
/* Sign bit will be cleared */ * unknown, so we lose our signed bounds
dst_reg->smin_value = 0; * 2) it's known negative, thus the unsigned bounds capture the
} else { * signed bounds
/* Lost sign bit information */ * 3) the signed bounds cross zero, so they tell us nothing
dst_reg->smin_value = S64_MIN; * about the result
dst_reg->smax_value = S64_MAX; * If the value in dst_reg is known nonnegative, then again the
} * unsigned bounts capture the signed bounds.
} else { * Thus, in all cases it suffices to blow away our signed bounds
dst_reg->smin_value = * and rely on inferring new ones from the unsigned bounds and
(u64)(dst_reg->smin_value) >> umax_val; * var_off of the result.
} */
dst_reg->smin_value = S64_MIN;
dst_reg->smax_value = S64_MAX;
if (src_known) if (src_known)
dst_reg->var_off = tnum_rshift(dst_reg->var_off, dst_reg->var_off = tnum_rshift(dst_reg->var_off,
umin_val); umin_val);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment