Commit d3bd7413 authored by Daniel Borkmann's avatar Daniel Borkmann Committed by Alexei Starovoitov

bpf: fix sanitation of alu op with pointer / scalar type from different paths

While 979d63d5 ("bpf: prevent out of bounds speculation on pointer
arithmetic") took care of rejecting alu op on pointer when e.g. pointer
came from two different map values with different map properties such as
value size, Jann reported that a case was not covered yet when a given
alu op is used in both "ptr_reg += reg" and "numeric_reg += reg" from
different branches where we would incorrectly try to sanitize based
on the pointer's limit. Catch this corner case and reject the program
instead.

Fixes: 979d63d5 ("bpf: prevent out of bounds speculation on pointer arithmetic")
Reported-by: default avatarJann Horn <jannh@google.com>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarAlexei Starovoitov <ast@kernel.org>
Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parent 466f89e9
...@@ -172,6 +172,7 @@ struct bpf_verifier_state_list { ...@@ -172,6 +172,7 @@ struct bpf_verifier_state_list {
#define BPF_ALU_SANITIZE_SRC 1U #define BPF_ALU_SANITIZE_SRC 1U
#define BPF_ALU_SANITIZE_DST 2U #define BPF_ALU_SANITIZE_DST 2U
#define BPF_ALU_NEG_VALUE (1U << 2) #define BPF_ALU_NEG_VALUE (1U << 2)
#define BPF_ALU_NON_POINTER (1U << 3)
#define BPF_ALU_SANITIZE (BPF_ALU_SANITIZE_SRC | \ #define BPF_ALU_SANITIZE (BPF_ALU_SANITIZE_SRC | \
BPF_ALU_SANITIZE_DST) BPF_ALU_SANITIZE_DST)
......
...@@ -3103,6 +3103,40 @@ static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg, ...@@ -3103,6 +3103,40 @@ static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg,
} }
} }
static bool can_skip_alu_sanitation(const struct bpf_verifier_env *env,
const struct bpf_insn *insn)
{
return env->allow_ptr_leaks || BPF_SRC(insn->code) == BPF_K;
}
static int update_alu_sanitation_state(struct bpf_insn_aux_data *aux,
u32 alu_state, u32 alu_limit)
{
/* If we arrived here from different branches with different
* state or limits to sanitize, then this won't work.
*/
if (aux->alu_state &&
(aux->alu_state != alu_state ||
aux->alu_limit != alu_limit))
return -EACCES;
/* Corresponding fixup done in fixup_bpf_calls(). */
aux->alu_state = alu_state;
aux->alu_limit = alu_limit;
return 0;
}
static int sanitize_val_alu(struct bpf_verifier_env *env,
struct bpf_insn *insn)
{
struct bpf_insn_aux_data *aux = cur_aux(env);
if (can_skip_alu_sanitation(env, insn))
return 0;
return update_alu_sanitation_state(aux, BPF_ALU_NON_POINTER, 0);
}
static int sanitize_ptr_alu(struct bpf_verifier_env *env, static int sanitize_ptr_alu(struct bpf_verifier_env *env,
struct bpf_insn *insn, struct bpf_insn *insn,
const struct bpf_reg_state *ptr_reg, const struct bpf_reg_state *ptr_reg,
...@@ -3117,7 +3151,7 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env, ...@@ -3117,7 +3151,7 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
struct bpf_reg_state tmp; struct bpf_reg_state tmp;
bool ret; bool ret;
if (env->allow_ptr_leaks || BPF_SRC(insn->code) == BPF_K) if (can_skip_alu_sanitation(env, insn))
return 0; return 0;
/* We already marked aux for masking from non-speculative /* We already marked aux for masking from non-speculative
...@@ -3133,19 +3167,8 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env, ...@@ -3133,19 +3167,8 @@ static int sanitize_ptr_alu(struct bpf_verifier_env *env,
if (retrieve_ptr_limit(ptr_reg, &alu_limit, opcode, off_is_neg)) if (retrieve_ptr_limit(ptr_reg, &alu_limit, opcode, off_is_neg))
return 0; return 0;
if (update_alu_sanitation_state(aux, alu_state, alu_limit))
/* If we arrived here from different branches with different
* limits to sanitize, then this won't work.
*/
if (aux->alu_state &&
(aux->alu_state != alu_state ||
aux->alu_limit != alu_limit))
return -EACCES; return -EACCES;
/* Corresponding fixup done in fixup_bpf_calls(). */
aux->alu_state = alu_state;
aux->alu_limit = alu_limit;
do_sim: do_sim:
/* Simulate and find potential out-of-bounds access under /* Simulate and find potential out-of-bounds access under
* speculative execution from truncation as a result of * speculative execution from truncation as a result of
...@@ -3418,6 +3441,8 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, ...@@ -3418,6 +3441,8 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
s64 smin_val, smax_val; s64 smin_val, smax_val;
u64 umin_val, umax_val; u64 umin_val, umax_val;
u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32; u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32;
u32 dst = insn->dst_reg;
int ret;
if (insn_bitness == 32) { if (insn_bitness == 32) {
/* Relevant for 32-bit RSH: Information can propagate towards /* Relevant for 32-bit RSH: Information can propagate towards
...@@ -3452,6 +3477,11 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, ...@@ -3452,6 +3477,11 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
switch (opcode) { switch (opcode) {
case BPF_ADD: case BPF_ADD:
ret = sanitize_val_alu(env, insn);
if (ret < 0) {
verbose(env, "R%d tried to add from different pointers or scalars\n", dst);
return ret;
}
if (signed_add_overflows(dst_reg->smin_value, smin_val) || if (signed_add_overflows(dst_reg->smin_value, smin_val) ||
signed_add_overflows(dst_reg->smax_value, smax_val)) { signed_add_overflows(dst_reg->smax_value, smax_val)) {
dst_reg->smin_value = S64_MIN; dst_reg->smin_value = S64_MIN;
...@@ -3471,6 +3501,11 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env, ...@@ -3471,6 +3501,11 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
dst_reg->var_off = tnum_add(dst_reg->var_off, src_reg.var_off); dst_reg->var_off = tnum_add(dst_reg->var_off, src_reg.var_off);
break; break;
case BPF_SUB: case BPF_SUB:
ret = sanitize_val_alu(env, insn);
if (ret < 0) {
verbose(env, "R%d tried to sub from different pointers or scalars\n", dst);
return ret;
}
if (signed_sub_overflows(dst_reg->smin_value, smax_val) || if (signed_sub_overflows(dst_reg->smin_value, smax_val) ||
signed_sub_overflows(dst_reg->smax_value, smin_val)) { signed_sub_overflows(dst_reg->smax_value, smin_val)) {
/* Overflow possible, we know nothing */ /* Overflow possible, we know nothing */
......
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