On Wed, Jul 6, 2022 at 12:11 PM Martin KaFai Lau <kafai@xxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > ---
> > 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
> >
