Skip to content

Commit 3df126f

Browse files
Jakub Kicinskidavem330
Jakub Kicinski
authored and
davem330
committed
bpf: don't (ab)use instructions to store state
Storing state in reserved fields of instructions makes it impossible to run verifier on programs already marked as read-only. Allocate and use an array of per-instruction state instead. While touching the error path rename and move existing jump target. Suggested-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Acked-by: Alexei Starovoitov <ast@kernel.org> Acked-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent eadb414 commit 3df126f

File tree

1 file changed

+40
-30
lines changed

1 file changed

+40
-30
lines changed

‎kernel/bpf/verifier.c

Lines changed: 40 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,10 @@ struct verifier_stack_elem {
181181
struct verifier_stack_elem *next;
182182
};
183183

184+
struct bpf_insn_aux_data {
185+
enum bpf_reg_type ptr_type; /* pointer type for load/store insns */
186+
};
187+
184188
#define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */
185189

186190
/* single container for all structs
@@ -197,6 +201,7 @@ struct verifier_env {
197201
u32 id_gen; /* used to generate unique reg IDs */
198202
bool allow_ptr_leaks;
199203
bool seen_direct_write;
204+
struct bpf_insn_aux_data *insn_aux_data; /* array of per-insn state */
200205
};
201206

202207
#define BPF_COMPLEXITY_LIMIT_INSNS 65536
@@ -2344,7 +2349,7 @@ static int do_check(struct verifier_env *env)
23442349
return err;
23452350

23462351
} else if (class == BPF_LDX) {
2347-
enum bpf_reg_type src_reg_type;
2352+
enum bpf_reg_type *prev_src_type, src_reg_type;
23482353

23492354
/* check for reserved fields is already done */
23502355

@@ -2374,16 +2379,18 @@ static int do_check(struct verifier_env *env)
23742379
continue;
23752380
}
23762381

2377-
if (insn->imm == 0) {
2382+
prev_src_type = &env->insn_aux_data[insn_idx].ptr_type;
2383+
2384+
if (*prev_src_type == NOT_INIT) {
23782385
/* saw a valid insn
23792386
* dst_reg = *(u32 *)(src_reg + off)
2380-
* use reserved 'imm' field to mark this insn
2387+
* save type to validate intersecting paths
23812388
*/
2382-
insn->imm = src_reg_type;
2389+
*prev_src_type = src_reg_type;
23832390

2384-
} else if (src_reg_type != insn->imm &&
2391+
} else if (src_reg_type != *prev_src_type &&
23852392
(src_reg_type == PTR_TO_CTX ||
2386-
insn->imm == PTR_TO_CTX)) {
2393+
*prev_src_type == PTR_TO_CTX)) {
23872394
/* ABuser program is trying to use the same insn
23882395
* dst_reg = *(u32*) (src_reg + off)
23892396
* with different pointer types:
@@ -2396,7 +2403,7 @@ static int do_check(struct verifier_env *env)
23962403
}
23972404

23982405
} else if (class == BPF_STX) {
2399-
enum bpf_reg_type dst_reg_type;
2406+
enum bpf_reg_type *prev_dst_type, dst_reg_type;
24002407

24012408
if (BPF_MODE(insn->code) == BPF_XADD) {
24022409
err = check_xadd(env, insn);
@@ -2424,11 +2431,13 @@ static int do_check(struct verifier_env *env)
24242431
if (err)
24252432
return err;
24262433

2427-
if (insn->imm == 0) {
2428-
insn->imm = dst_reg_type;
2429-
} else if (dst_reg_type != insn->imm &&
2434+
prev_dst_type = &env->insn_aux_data[insn_idx].ptr_type;
2435+
2436+
if (*prev_dst_type == NOT_INIT) {
2437+
*prev_dst_type = dst_reg_type;
2438+
} else if (dst_reg_type != *prev_dst_type &&
24302439
(dst_reg_type == PTR_TO_CTX ||
2431-
insn->imm == PTR_TO_CTX)) {
2440+
*prev_dst_type == PTR_TO_CTX)) {
24322441
verbose("same insn cannot be used with different pointers\n");
24332442
return -EINVAL;
24342443
}
@@ -2686,10 +2695,11 @@ static void convert_pseudo_ld_imm64(struct verifier_env *env)
26862695
static int convert_ctx_accesses(struct verifier_env *env)
26872696
{
26882697
const struct bpf_verifier_ops *ops = env->prog->aux->ops;
2698+
const int insn_cnt = env->prog->len;
26892699
struct bpf_insn insn_buf[16], *insn;
26902700
struct bpf_prog *new_prog;
26912701
enum bpf_access_type type;
2692-
int i, insn_cnt, cnt;
2702+
int i, cnt, delta = 0;
26932703

26942704
if (ops->gen_prologue) {
26952705
cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
@@ -2703,18 +2713,16 @@ static int convert_ctx_accesses(struct verifier_env *env)
27032713
if (!new_prog)
27042714
return -ENOMEM;
27052715
env->prog = new_prog;
2716+
delta += cnt - 1;
27062717
}
27072718
}
27082719

27092720
if (!ops->convert_ctx_access)
27102721
return 0;
27112722

2712-
insn_cnt = env->prog->len;
2713-
insn = env->prog->insnsi;
2723+
insn = env->prog->insnsi + delta;
27142724

27152725
for (i = 0; i < insn_cnt; i++, insn++) {
2716-
u32 insn_delta;
2717-
27182726
if (insn->code == (BPF_LDX | BPF_MEM | BPF_W) ||
27192727
insn->code == (BPF_LDX | BPF_MEM | BPF_DW))
27202728
type = BPF_READ;
@@ -2724,11 +2732,8 @@ static int convert_ctx_accesses(struct verifier_env *env)
27242732
else
27252733
continue;
27262734

2727-
if (insn->imm != PTR_TO_CTX) {
2728-
/* clear internal mark */
2729-
insn->imm = 0;
2735+
if (env->insn_aux_data[i].ptr_type != PTR_TO_CTX)
27302736
continue;
2731-
}
27322737

27332738
cnt = ops->convert_ctx_access(type, insn->dst_reg, insn->src_reg,
27342739
insn->off, insn_buf, env->prog);
@@ -2737,18 +2742,16 @@ static int convert_ctx_accesses(struct verifier_env *env)
27372742
return -EINVAL;
27382743
}
27392744

2740-
new_prog = bpf_patch_insn_single(env->prog, i, insn_buf, cnt);
2745+
new_prog = bpf_patch_insn_single(env->prog, i + delta, insn_buf,
2746+
cnt);
27412747
if (!new_prog)
27422748
return -ENOMEM;
27432749

2744-
insn_delta = cnt - 1;
2750+
delta += cnt - 1;
27452751

27462752
/* keep walking new program and skip insns we just inserted */
27472753
env->prog = new_prog;
2748-
insn = new_prog->insnsi + i + insn_delta;
2749-
2750-
insn_cnt += insn_delta;
2751-
i += insn_delta;
2754+
insn = new_prog->insnsi + i + delta;
27522755
}
27532756

27542757
return 0;
@@ -2792,6 +2795,11 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
27922795
if (!env)
27932796
return -ENOMEM;
27942797

2798+
env->insn_aux_data = vzalloc(sizeof(struct bpf_insn_aux_data) *
2799+
(*prog)->len);
2800+
ret = -ENOMEM;
2801+
if (!env->insn_aux_data)
2802+
goto err_free_env;
27952803
env->prog = *prog;
27962804

27972805
/* grab the mutex to protect few globals used by verifier */
@@ -2810,12 +2818,12 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
28102818
/* log_* values have to be sane */
28112819
if (log_size < 128 || log_size > UINT_MAX >> 8 ||
28122820
log_level == 0 || log_ubuf == NULL)
2813-
goto free_env;
2821+
goto err_unlock;
28142822

28152823
ret = -ENOMEM;
28162824
log_buf = vmalloc(log_size);
28172825
if (!log_buf)
2818-
goto free_env;
2826+
goto err_unlock;
28192827
} else {
28202828
log_level = 0;
28212829
}
@@ -2884,14 +2892,16 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
28842892
free_log_buf:
28852893
if (log_level)
28862894
vfree(log_buf);
2887-
free_env:
28882895
if (!env->prog->aux->used_maps)
28892896
/* if we didn't copy map pointers into bpf_prog_info, release
28902897
* them now. Otherwise free_bpf_prog_info() will release them.
28912898
*/
28922899
release_maps(env);
28932900
*prog = env->prog;
2894-
kfree(env);
2901+
err_unlock:
28952902
mutex_unlock(&bpf_verifier_lock);
2903+
vfree(env->insn_aux_data);
2904+
err_free_env:
2905+
kfree(env);
28962906
return ret;
28972907
}

0 commit comments

Comments
 (0)