On Wed, 12 Sep 2018 at 15:50, Alexei Starovoitov
<alexei.starovoi...@gmail.com> wrote:
>
> On Tue, Sep 11, 2018 at 05:36:33PM -0700, Joe Stringer wrote:
> > ...
> > +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type
> > prev)
> > +{
> > + return src != prev && (!reg_type_mismatch_ok(src) ||
> > + !reg_type_mismatch_ok(prev));
> > +}
> > +
> > static int do_check(struct bpf_verifier_env *env)
> > {
> > struct bpf_verifier_state *state;
> > @@ -4778,9 +4862,7 @@ static int do_check(struct bpf_verifier_env *env)
> > */
> > *prev_src_type = src_reg_type;
> >
> > - } else if (src_reg_type != *prev_src_type &&
> > - (src_reg_type == PTR_TO_CTX ||
> > - *prev_src_type == PTR_TO_CTX)) {
> > + } else if (reg_type_mismatch(src_reg_type,
> > *prev_src_type)) {
> > /* ABuser program is trying to use the same
> > insn
> > * dst_reg = *(u32*) (src_reg + off)
> > * with different pointer types:
> > @@ -4826,8 +4908,8 @@ static int do_check(struct bpf_verifier_env *env)
> > if (*prev_dst_type == NOT_INIT) {
> > *prev_dst_type = dst_reg_type;
> > } else if (dst_reg_type != *prev_dst_type &&
> > - (dst_reg_type == PTR_TO_CTX ||
> > - *prev_dst_type == PTR_TO_CTX)) {
> > + (!reg_type_mismatch_ok(dst_reg_type) ||
> > + !reg_type_mismatch_ok(*prev_dst_type))) {
>
> reg_type_mismatch() could have been used here as well ?
Missed that before, will fix.
> > verbose(env, "same insn cannot be used with
> > different pointers\n");
> > return -EINVAL;
> > }
> > @@ -5244,10 +5326,14 @@ static void sanitize_dead_code(struct
> > bpf_verifier_env *env)
> > }
> > }
> >
> > -/* convert load instructions that access fields of 'struct __sk_buff'
> > - * into sequence of instructions that access fields of 'struct sk_buff'
> > +/* convert load instructions that access fields of a context type into a
> > + * sequence of instructions that access fields of the underlying structure:
> > + * struct __sk_buff -> struct sk_buff
> > + * struct bpf_sock_ops -> struct sock
> > */
> > -static int convert_ctx_accesses(struct bpf_verifier_env *env)
> > +static int convert_ctx_accesses(struct bpf_verifier_env *env,
> > + bpf_convert_ctx_access_t convert_ctx_access,
> > + enum bpf_reg_type ctx_type)
> > {
> > const struct bpf_verifier_ops *ops = env->ops;
> > int i, cnt, size, ctx_field_size, delta = 0;
> > @@ -5274,12 +5360,14 @@ static int convert_ctx_accesses(struct
> > bpf_verifier_env *env)
> > }
> > }
> >
> > - if (!ops->convert_ctx_access || bpf_prog_is_dev_bound(env->prog->aux))
> > + if (!convert_ctx_access || bpf_prog_is_dev_bound(env->prog->aux))
> > return 0;
> >
> > insn = env->prog->insnsi + delta;
> >
> > for (i = 0; i < insn_cnt; i++, insn++) {
> > + enum bpf_reg_type ptr_type;
> > +
> > if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
> > insn->code == (BPF_LDX | BPF_MEM | BPF_H) ||
> > insn->code == (BPF_LDX | BPF_MEM | BPF_W) ||
> > @@ -5321,7 +5409,8 @@ static int convert_ctx_accesses(struct
> > bpf_verifier_env *env)
> > continue;
> > }
> >
> > - if (env->insn_aux_data[i + delta].ptr_type != PTR_TO_CTX)
> > + ptr_type = env->insn_aux_data[i + delta].ptr_type;
> > + if (ptr_type != ctx_type)
> > continue;
> >
> > ctx_field_size = env->insn_aux_data[i + delta].ctx_field_size;
> > @@ -5354,8 +5443,8 @@ static int convert_ctx_accesses(struct
> > bpf_verifier_env *env)
> > }
> >
> > target_size = 0;
> > - cnt = ops->convert_ctx_access(type, insn, insn_buf, env->prog,
> > - &target_size);
> > + cnt = convert_ctx_access(type, insn, insn_buf, env->prog,
> > + &target_size);
> > if (cnt == 0 || cnt >= ARRAY_SIZE(insn_buf) ||
> > (ctx_field_size && !target_size)) {
> > verbose(env, "bpf verifier is misconfigured\n");
> > @@ -5899,7 +5988,13 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr
> > *attr)
> >
> > if (ret == 0)
> > /* program is valid, convert *(u32*)(ctx + off) accesses */
> > - ret = convert_ctx_accesses(env);
> > + ret = convert_ctx_accesses(env, env->ops->convert_ctx_access,
> > + PTR_TO_CTX);
> > +
> > + if (ret == 0)
> > + /* Convert *(u32*)(sock_ops + off) accesses */
> > + ret = convert_ctx_accesses(env, bpf_sock_convert_ctx_access,
> > + PTR_TO_SOCKET);
>
> can it be done in single pass instead?
> env->insn_aux_data tells us what kind of conversion needs to happen.
> while progs are small, I guess, it's ok, but would like to see a follow up
> patch to this.
Yeah, good point - upon review it seems like this would be a simple
change (incremental):
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index db05f78bfc6e..7fa1b695a2a1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5329,9 +5329,7 @@ static void sanitize_dead_code(struct
bpf_verifier_env *env)
* struct __sk_buff -> struct sk_buff
* struct bpf_sock_ops -> struct sock
*/
-static int convert_ctx_accesses(struct bpf_verifier_env *env,
- bpf_convert_ctx_access_t convert_ctx_access,
- enum bpf_reg_type ctx_type)
+static int convert_ctx_accesses(struct bpf_verifier_env *env)
{
const struct bpf_verifier_ops *ops = env->ops;
int i, cnt, size, ctx_field_size, delta = 0;
@@ -5358,13 +5356,13 @@ static int convert_ctx_accesses(struct
bpf_verifier_env *env,
}
}
- if (!convert_ctx_access || bpf_prog_is_dev_bound(env->prog->aux))
+ if (bpf_prog_is_dev_bound(env->prog->aux))
return 0;
insn = env->prog->insnsi + delta;
for (i = 0; i < insn_cnt; i++, insn++) {
- enum bpf_reg_type ptr_type;
+ bpf_convert_ctx_access_t convert_ctx_access;
if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
insn->code == (BPF_LDX | BPF_MEM | BPF_H) ||
@@ -5407,9 +5405,18 @@ static int convert_ctx_accesses(struct
bpf_verifier_env *env,
continue;
}
- ptr_type = env->insn_aux_data[i + delta].ptr_type;
- if (ptr_type != ctx_type)
+ switch (env->insn_aux_data[i + delta].ptr_type) {
+ case PTR_TO_CTX:
+ if (!ops->convert_ctx_access)
+ continue;
+ convert_ctx_access = ops->convert_ctx_access;
+ break;
+ case PTR_TO_SOCKET:
+ convert_ctx_access = bpf_sock_convert_ctx_access;
+ break;
+ default:
continue;
+ }
ctx_field_size = env->insn_aux_data[i + delta].ctx_field_size;
size = BPF_LDST_BYTES(insn);
@@ -5986,13 +5993,7 @@ int bpf_check(struct bpf_prog **prog, union
bpf_attr *attr)
if (ret == 0)
/* program is valid, convert *(u32*)(ctx + off) accesses */
- ret = convert_ctx_accesses(env, env->ops->convert_ctx_access,
- PTR_TO_CTX);
-
- if (ret == 0)
- /* Convert *(u32*)(sock_ops + off) accesses */
- ret = convert_ctx_accesses(env, bpf_sock_convert_ctx_access,
- PTR_TO_SOCKET);
+ ret = convert_ctx_accesses(env);
if (ret == 0)
ret = fixup_bpf_calls(env);
