netdev - Re: [RFC PATCH v9 01/11] bpf: Support getting referenced kptr from struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMB2axNDVCdH7stBj8-duOcV1P=qjyjUAR+YXywVMx8HgRPokg@mail.gmail.com>
Date: Wed, 24 Jul 2024 10:00:30 -0700
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, yangpeihao@...u.edu.cn, 
	daniel@...earbox.net, andrii@...nel.org, alexei.starovoitov@...il.com, 
	martin.lau@...nel.org, sinquersw@...il.com, toke@...hat.com, jhs@...atatu.com, 
	jiri@...nulli.us, sdf@...gle.com, xiyou.wangcong@...il.com, 
	yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v9 01/11] bpf: Support getting referenced kptr from
 struct_ops argument

On Tue, Jul 23, 2024 at 5:32 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 7/14/24 10:51 AM, Amery Hung wrote:
> > @@ -21004,6 +21025,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
> >               mark_reg_known_zero(env, regs, BPF_REG_1);
> >       }
> >
> > +     if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +             ctx_arg_info = (struct bpf_ctx_arg_aux *)env->prog->aux->ctx_arg_info;
> > +             for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
> > +                     if (ctx_arg_info[i].refcounted)
> > +                             ctx_arg_info[i].ref_obj_id = acquire_reference_state(env, 0);
> > +     }
> > +
>
> I think this will miss a case when passing the struct_ops prog ctx (i.e. "__u64
> *ctx") to a global subprog. Something like this:
>
> __noinline int subprog_release(__u64 *ctx __arg_ctx)
> {
>         struct task_struct *task = (struct task_struct *)ctx[1];
>         int dummy = (int)ctx[0];
>
>         bpf_task_release(task);
>
>         return dummy + 1;
> }
>
> SEC("struct_ops/subprog_ref")
> __failure
> int test_subprog_ref(__u64 *ctx)
> {
>         struct task_struct *task = (struct task_struct *)ctx[1];
>
>         bpf_task_release(task);
>
>         return subprog_release(ctx);;
> }
>
> SEC(".struct_ops.link")
> struct bpf_testmod_ops subprog_ref = {
>         .test_refcounted = (void *)test_subprog_ref,
> };
>

Thanks for pointing this out. The test did failed.

> A quick thought is, I think tracking the ctx's ref id in the env->cur_state may
> not be the correct place.

I think it is a bit tricky because subprogs are checked independently
and their state is folded (i.e., there can be multiple edges from the
main program to a subprog).

Maybe the verifier can rewrite the program: set the refcounted ctx to
NULL when releasing reference. Then, in do_check_common(), if it is a
global subprog, we mark refcounted ctx as PTR_MAYBE_NULL to force a
runtime check. How does it sound?

>
> [ Just want to bring up what I have noticed so far. I will stop at here for
> today and will continue. ]