| Message ID | 20220706174857.3799351-1-sdf@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: check attach_func_proto return type more carefully | expand |
On Wed, Jul 06, 2022 at 10:48:57AM -0700, Stanislav Fomichev wrote: > Syzkaller reports the following crash: > RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline] > RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline] > RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610 > > With the following reproducer: > bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80) > > Because we don't enforce expected_attach_type for XDP programs, > we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP' > part in check_return_code and follow up with testing > `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto` > is NULL. > > Let's add a new btf_func_returns_void() wrapper which is more defensive > and use it in the places where we currently do '!->type' check. > > Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor") > Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > include/linux/btf.h | 5 +++++ > kernel/bpf/trampoline.c | 2 +- > kernel/bpf/verifier.c | 8 ++++---- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 1bfed7fa0428..17ba7d27a8ad 100644 > --- a/include/linux/btf.h > +++ b/include/linux/btf.h > @@ -302,6 +302,11 @@ static inline u16 btf_func_linkage(const struct btf_type *t) > return BTF_INFO_VLEN(t->info); > } > > +static inline bool btf_func_returns_void(const struct btf_type *t) > +{ > + return t && !t->type; > +} > + > static inline bool btf_type_kflag(const struct btf_type *t) > { > return BTF_INFO_KFLAG(t->info); > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > index 6cd226584c33..9c4cb4c8a5fa 100644 > --- a/kernel/bpf/trampoline.c > +++ b/kernel/bpf/trampoline.c > @@ -400,7 +400,7 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) > case BPF_TRACE_FEXIT: > return BPF_TRAMP_FEXIT; > case BPF_LSM_MAC: > - if (!prog->aux->attach_func_proto->type) > + if (btf_func_returns_void(prog->aux->attach_func_proto)) > /* The function returns void, we cannot modify its > * return value. > */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index df3ec6b05f05..e3ee6f70939b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7325,7 +7325,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > break; > case BPF_FUNC_set_retval: > if (env->prog->expected_attach_type == BPF_LSM_CGROUP) { > - if (!env->prog->aux->attach_func_proto->type) { > + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { > /* Make sure programs that attach to void > * hooks don't try to modify return value. > */ > @@ -10447,7 +10447,7 @@ static int check_return_code(struct bpf_verifier_env *env) > if (!is_subprog && > (prog_type == BPF_PROG_TYPE_STRUCT_OPS || > prog_type == BPF_PROG_TYPE_LSM) && > - !prog->aux->attach_func_proto->type) > + btf_func_returns_void(prog->aux->attach_func_proto)) > return 0; It seems there is another problem here. It returns early here for prog_type == BPF_PROG_TYPE_LSM. It should only do that for expected_attach_type != BPF_LSM_CGROUP. Otherwise, the later verbose(env, "Note, BPF_LSM_CGROUP...") will not get a chance. > > /* eBPF calling convention is such that R0 is used > @@ -10547,7 +10547,7 @@ static int check_return_code(struct bpf_verifier_env *env) > */ > return 0; > } > - if (!env->prog->aux->attach_func_proto->type) { > + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { > /* Make sure programs that attach to void > * hooks don't try to modify return value. > */ > @@ -10572,7 +10572,7 @@ static int check_return_code(struct bpf_verifier_env *env) > if (!tnum_in(range, reg->var_off)) { > verbose_invalid_scalar(env, reg, &range, "program exit", "R0"); > if (prog->expected_attach_type == BPF_LSM_CGROUP && I think the problem is more like missing prog_type == BPF_PROG_TYPE_LSM [&& expected_attach_type == BPF_LSM_CGROUP] here instead of testing !attach_func_proto in all places. > - !prog->aux->attach_func_proto->type) > + btf_func_returns_void(prog->aux->attach_func_proto)) > verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n"); > return -EINVAL; > } > -- > 2.37.0.rc0.161.g10f37bed90-goog >
On Wed, Jul 6, 2022 at 12:11 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Wed, Jul 06, 2022 at 10:48:57AM -0700, Stanislav Fomichev wrote: > > Syzkaller reports the following crash: > > RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline] > > RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline] > > RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610 > > > > With the following reproducer: > > bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80) > > > > Because we don't enforce expected_attach_type for XDP programs, > > we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP' > > part in check_return_code and follow up with testing > > `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto` > > is NULL. > > > > Let's add a new btf_func_returns_void() wrapper which is more defensive > > and use it in the places where we currently do '!->type' check. > > > > Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor") > > Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > include/linux/btf.h | 5 +++++ > > kernel/bpf/trampoline.c | 2 +- > > kernel/bpf/verifier.c | 8 ++++---- > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/btf.h b/include/linux/btf.h > > index 1bfed7fa0428..17ba7d27a8ad 100644 > > --- a/include/linux/btf.h > > +++ b/include/linux/btf.h > > @@ -302,6 +302,11 @@ static inline u16 btf_func_linkage(const struct btf_type *t) > > return BTF_INFO_VLEN(t->info); > > } > > > > +static inline bool btf_func_returns_void(const struct btf_type *t) > > +{ > > + return t && !t->type; > > +} > > + > > static inline bool btf_type_kflag(const struct btf_type *t) > > { > > return BTF_INFO_KFLAG(t->info); > > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c > > index 6cd226584c33..9c4cb4c8a5fa 100644 > > --- a/kernel/bpf/trampoline.c > > +++ b/kernel/bpf/trampoline.c > > @@ -400,7 +400,7 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) > > case BPF_TRACE_FEXIT: > > return BPF_TRAMP_FEXIT; > > case BPF_LSM_MAC: > > - if (!prog->aux->attach_func_proto->type) > > + if (btf_func_returns_void(prog->aux->attach_func_proto)) > > /* The function returns void, we cannot modify its > > * return value. > > */ > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index df3ec6b05f05..e3ee6f70939b 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7325,7 +7325,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > break; > > case BPF_FUNC_set_retval: > > if (env->prog->expected_attach_type == BPF_LSM_CGROUP) { > > - if (!env->prog->aux->attach_func_proto->type) { > > + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { > > /* Make sure programs that attach to void > > * hooks don't try to modify return value. > > */ > > @@ -10447,7 +10447,7 @@ static int check_return_code(struct bpf_verifier_env *env) > > if (!is_subprog && > > (prog_type == BPF_PROG_TYPE_STRUCT_OPS || > > prog_type == BPF_PROG_TYPE_LSM) && > > - !prog->aux->attach_func_proto->type) > > + btf_func_returns_void(prog->aux->attach_func_proto)) > > return 0; > It seems there is another problem here. > It returns early here for prog_type == BPF_PROG_TYPE_LSM. > It should only do that for expected_attach_type != BPF_LSM_CGROUP. > > Otherwise, the later verbose(env, "Note, BPF_LSM_CGROUP...") will > not get a chance. Ah, true, will add expected_attach_type check here as well, thanks! > > > > /* eBPF calling convention is such that R0 is used > > @@ -10547,7 +10547,7 @@ static int check_return_code(struct bpf_verifier_env *env) > > */ > > return 0; > > } > > - if (!env->prog->aux->attach_func_proto->type) { > > + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { > > /* Make sure programs that attach to void > > * hooks don't try to modify return value. > > */ > > @@ -10572,7 +10572,7 @@ static int check_return_code(struct bpf_verifier_env *env) > > if (!tnum_in(range, reg->var_off)) { > > verbose_invalid_scalar(env, reg, &range, "program exit", "R0"); > > if (prog->expected_attach_type == BPF_LSM_CGROUP && > I think the problem is more like missing > prog_type == BPF_PROG_TYPE_LSM [&& expected_attach_type == BPF_LSM_CGROUP] here > instead of testing !attach_func_proto in all places. SG! > > - !prog->aux->attach_func_proto->type) > > + btf_func_returns_void(prog->aux->attach_func_proto)) > > verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n"); > > return -EINVAL; > > } > > -- > > 2.37.0.rc0.161.g10f37bed90-goog > >
diff --git a/include/linux/btf.h b/include/linux/btf.h index 1bfed7fa0428..17ba7d27a8ad 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -302,6 +302,11 @@ static inline u16 btf_func_linkage(const struct btf_type *t) return BTF_INFO_VLEN(t->info); } +static inline bool btf_func_returns_void(const struct btf_type *t) +{ + return t && !t->type; +} + static inline bool btf_type_kflag(const struct btf_type *t) { return BTF_INFO_KFLAG(t->info); diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index 6cd226584c33..9c4cb4c8a5fa 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -400,7 +400,7 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) case BPF_TRACE_FEXIT: return BPF_TRAMP_FEXIT; case BPF_LSM_MAC: - if (!prog->aux->attach_func_proto->type) + if (btf_func_returns_void(prog->aux->attach_func_proto)) /* The function returns void, we cannot modify its * return value. */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index df3ec6b05f05..e3ee6f70939b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7325,7 +7325,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn break; case BPF_FUNC_set_retval: if (env->prog->expected_attach_type == BPF_LSM_CGROUP) { - if (!env->prog->aux->attach_func_proto->type) { + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { /* Make sure programs that attach to void * hooks don't try to modify return value. */ @@ -10447,7 +10447,7 @@ static int check_return_code(struct bpf_verifier_env *env) if (!is_subprog && (prog_type == BPF_PROG_TYPE_STRUCT_OPS || prog_type == BPF_PROG_TYPE_LSM) && - !prog->aux->attach_func_proto->type) + btf_func_returns_void(prog->aux->attach_func_proto)) return 0; /* eBPF calling convention is such that R0 is used @@ -10547,7 +10547,7 @@ static int check_return_code(struct bpf_verifier_env *env) */ return 0; } - if (!env->prog->aux->attach_func_proto->type) { + if (btf_func_returns_void(env->prog->aux->attach_func_proto)) { /* Make sure programs that attach to void * hooks don't try to modify return value. */ @@ -10572,7 +10572,7 @@ static int check_return_code(struct bpf_verifier_env *env) if (!tnum_in(range, reg->var_off)) { verbose_invalid_scalar(env, reg, &range, "program exit", "R0"); if (prog->expected_attach_type == BPF_LSM_CGROUP && - !prog->aux->attach_func_proto->type) + btf_func_returns_void(prog->aux->attach_func_proto)) verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n"); return -EINVAL; }
Syzkaller reports the following crash: RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline] RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline] RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610 With the following reproducer: bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80) Because we don't enforce expected_attach_type for XDP programs, we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP' part in check_return_code and follow up with testing `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto` is NULL. Let's add a new btf_func_returns_void() wrapper which is more defensive and use it in the places where we currently do '!->type' check. Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor") Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com Signed-off-by: Stanislav Fomichev <sdf@google.com> --- include/linux/btf.h | 5 +++++ kernel/bpf/trampoline.c | 2 +- kernel/bpf/verifier.c | 8 ++++---- 3 files changed, 10 insertions(+), 5 deletions(-)