| From a6aaece00a57fa6f22575364b3903dfbccf5345d Mon Sep 17 00:00:00 2001 |
| From: Daniel Borkmann <daniel@iogearbox.net> |
| Date: Tue, 23 Mar 2021 09:30:01 +0100 |
| Subject: bpf: Improve verifier error messages for users |
| |
| From: Daniel Borkmann <daniel@iogearbox.net> |
| |
| commit a6aaece00a57fa6f22575364b3903dfbccf5345d upstream. |
| |
| Consolidate all error handling and provide more user-friendly error messages |
| from sanitize_ptr_alu() and sanitize_val_alu(). |
| |
| Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> |
| Reviewed-by: John Fastabend <john.fastabend@gmail.com> |
| Acked-by: Alexei Starovoitov <ast@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/bpf/verifier.c | 86 ++++++++++++++++++++++++++++++++++++-------------- |
| 1 file changed, 63 insertions(+), 23 deletions(-) |
| |
| --- a/kernel/bpf/verifier.c |
| +++ b/kernel/bpf/verifier.c |
| @@ -5328,6 +5328,14 @@ static struct bpf_insn_aux_data *cur_aux |
| return &env->insn_aux_data[env->insn_idx]; |
| } |
| |
| +enum { |
| + REASON_BOUNDS = -1, |
| + REASON_TYPE = -2, |
| + REASON_PATHS = -3, |
| + REASON_LIMIT = -4, |
| + REASON_STACK = -5, |
| +}; |
| + |
| static int retrieve_ptr_limit(const struct bpf_reg_state *ptr_reg, |
| const struct bpf_reg_state *off_reg, |
| u32 *alu_limit, u8 opcode) |
| @@ -5339,7 +5347,7 @@ static int retrieve_ptr_limit(const stru |
| |
| if (!tnum_is_const(off_reg->var_off) && |
| (off_reg->smin_value < 0) != (off_reg->smax_value < 0)) |
| - return -EACCES; |
| + return REASON_BOUNDS; |
| |
| switch (ptr_reg->type) { |
| case PTR_TO_STACK: |
| @@ -5366,11 +5374,11 @@ static int retrieve_ptr_limit(const stru |
| } |
| break; |
| default: |
| - return -EINVAL; |
| + return REASON_TYPE; |
| } |
| |
| if (ptr_limit >= max) |
| - return -ERANGE; |
| + return REASON_LIMIT; |
| *alu_limit = ptr_limit; |
| return 0; |
| } |
| @@ -5390,7 +5398,7 @@ static int update_alu_sanitation_state(s |
| if (aux->alu_state && |
| (aux->alu_state != alu_state || |
| aux->alu_limit != alu_limit)) |
| - return -EACCES; |
| + return REASON_PATHS; |
| |
| /* Corresponding fixup done in fixup_bpf_calls(). */ |
| aux->alu_state = alu_state; |
| @@ -5463,7 +5471,46 @@ do_sim: |
| ret = push_stack(env, env->insn_idx + 1, env->insn_idx, true); |
| if (!ptr_is_dst_reg && ret) |
| *dst_reg = tmp; |
| - return !ret ? -EFAULT : 0; |
| + return !ret ? REASON_STACK : 0; |
| +} |
| + |
| +static int sanitize_err(struct bpf_verifier_env *env, |
| + const struct bpf_insn *insn, int reason, |
| + const struct bpf_reg_state *off_reg, |
| + const struct bpf_reg_state *dst_reg) |
| +{ |
| + static const char *err = "pointer arithmetic with it prohibited for !root"; |
| + const char *op = BPF_OP(insn->code) == BPF_ADD ? "add" : "sub"; |
| + u32 dst = insn->dst_reg, src = insn->src_reg; |
| + |
| + switch (reason) { |
| + case REASON_BOUNDS: |
| + verbose(env, "R%d has unknown scalar with mixed signed bounds, %s\n", |
| + off_reg == dst_reg ? dst : src, err); |
| + break; |
| + case REASON_TYPE: |
| + verbose(env, "R%d has pointer with unsupported alu operation, %s\n", |
| + off_reg == dst_reg ? src : dst, err); |
| + break; |
| + case REASON_PATHS: |
| + verbose(env, "R%d tried to %s from different maps, paths or scalars, %s\n", |
| + dst, op, err); |
| + break; |
| + case REASON_LIMIT: |
| + verbose(env, "R%d tried to %s beyond pointer bounds, %s\n", |
| + dst, op, err); |
| + break; |
| + case REASON_STACK: |
| + verbose(env, "R%d could not be pushed for speculative verification, %s\n", |
| + dst, err); |
| + break; |
| + default: |
| + verbose(env, "verifier internal error: unknown reason (%d)\n", |
| + reason); |
| + break; |
| + } |
| + |
| + return -EACCES; |
| } |
| |
| /* Handles arithmetic on a pointer and a scalar: computes new min/max and var_off. |
| @@ -5553,10 +5600,9 @@ static int adjust_ptr_min_max_vals(struc |
| switch (opcode) { |
| case BPF_ADD: |
| ret = sanitize_ptr_alu(env, insn, ptr_reg, off_reg, dst_reg); |
| - if (ret < 0) { |
| - verbose(env, "R%d tried to add from different maps, paths, or prohibited types\n", dst); |
| - return ret; |
| - } |
| + if (ret < 0) |
| + return sanitize_err(env, insn, ret, off_reg, dst_reg); |
| + |
| /* We can take a fixed offset as long as it doesn't overflow |
| * the s32 'off' field |
| */ |
| @@ -5608,10 +5654,9 @@ static int adjust_ptr_min_max_vals(struc |
| break; |
| case BPF_SUB: |
| ret = sanitize_ptr_alu(env, insn, ptr_reg, off_reg, dst_reg); |
| - if (ret < 0) { |
| - verbose(env, "R%d tried to sub from different maps, paths, or prohibited types\n", dst); |
| - return ret; |
| - } |
| + if (ret < 0) |
| + return sanitize_err(env, insn, ret, off_reg, dst_reg); |
| + |
| if (dst_reg == off_reg) { |
| /* scalar -= pointer. Creates an unknown scalar */ |
| verbose(env, "R%d tried to subtract pointer from scalar\n", |
| @@ -6301,9 +6346,8 @@ static int adjust_scalar_min_max_vals(st |
| s32 s32_min_val, s32_max_val; |
| u32 u32_min_val, u32_max_val; |
| u64 insn_bitness = (BPF_CLASS(insn->code) == BPF_ALU64) ? 64 : 32; |
| - u32 dst = insn->dst_reg; |
| - int ret; |
| bool alu32 = (BPF_CLASS(insn->code) != BPF_ALU64); |
| + int ret; |
| |
| smin_val = src_reg.smin_value; |
| smax_val = src_reg.smax_value; |
| @@ -6362,20 +6406,16 @@ static int adjust_scalar_min_max_vals(st |
| switch (opcode) { |
| 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 (ret < 0) |
| + return sanitize_err(env, insn, ret, NULL, NULL); |
| scalar32_min_max_add(dst_reg, &src_reg); |
| scalar_min_max_add(dst_reg, &src_reg); |
| dst_reg->var_off = tnum_add(dst_reg->var_off, src_reg.var_off); |
| break; |
| 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 (ret < 0) |
| + return sanitize_err(env, insn, ret, NULL, NULL); |
| scalar32_min_max_sub(dst_reg, &src_reg); |
| scalar_min_max_sub(dst_reg, &src_reg); |
| dst_reg->var_off = tnum_sub(dst_reg->var_off, src_reg.var_off); |