netdev - Re: [RFC PATCH v8 02/20] selftests/bpf: Test referenced kptr arguments of 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: <CAMB2axN3XwSmvk2eC9OnaUk5QvXS6sLVv148NrepkbtjCixVwg@mail.gmail.com>
Date: Fri, 10 May 2024 15:16:32 -0700
From: Amery Hung <ameryhung@...il.com>
To: Kui-Feng Lee <sinquersw@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, yangpeihao@...u.edu.cn, 
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org, 
	toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com, 
	xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v8 02/20] selftests/bpf: Test referenced kptr
 arguments of struct_ops programs

On Fri, May 10, 2024 at 2:33 PM Kui-Feng Lee <sinquersw@...il.com> wrote:
>
>
>
> On 5/10/24 12:23, Amery Hung wrote:
> > A reference is automatically acquired for a referenced kptr argument
> > annotated via the stub function with "__ref_acquired" in a struct_ops
> > program. It must be released and cannot be acquired more than once.
> >
> > The test first checks whether a reference to the correct type is acquired
> > in "ref_acquire". Then, we check if the verifier correctly rejects the
> > program that fails to release the reference (i.e., reference leak) in
> > "ref_acquire_ref_leak". Finally, we check if the reference can be only
> > acquired once through the argument in "ref_acquire_dup_ref".
> >
> > Signed-off-by: Amery Hung <amery.hung@...edance.com>
> > ---
> >   .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  7 +++
> >   .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  2 +
> >   .../prog_tests/test_struct_ops_ref_acquire.c  | 58 +++++++++++++++++++
> >   .../bpf/progs/struct_ops_ref_acquire.c        | 27 +++++++++
> >   .../progs/struct_ops_ref_acquire_dup_ref.c    | 24 ++++++++
> >   .../progs/struct_ops_ref_acquire_ref_leak.c   | 19 ++++++
> >   6 files changed, 137 insertions(+)
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_ref_acquire.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_ref_acquire.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_ref_acquire_dup_ref.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_ref_acquire_ref_leak.c
> >
> >
>   ... skipped ...
> > +
> > diff --git a/tools/testing/selftests/bpf/progs/struct_ops_ref_acquire.c b/tools/testing/selftests/bpf/progs/struct_ops_ref_acquire.c
> > new file mode 100644
> > index 000000000000..bae342db0fdb
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/struct_ops_ref_acquire.c
> > @@ -0,0 +1,27 @@
> > +#include <vmlinux.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include "../bpf_testmod/bpf_testmod.h"
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +void bpf_task_release(struct task_struct *p) __ksym;
> > +
> > +/* This is a test BPF program that uses struct_ops to access a referenced
> > + * kptr argument. This is a test for the verifier to ensure that it recongnizes
> > + * the task as a referenced object (i.e., ref_obj_id > 0).
> > + */
> > +SEC("struct_ops/test_ref_acquire")
> > +int BPF_PROG(test_ref_acquire, int dummy,
> > +          struct task_struct *task)
> > +{
> > +     bpf_task_release(task);
>
> This looks weird for me.
>
> According to what you mentioned in the patch 1, the purpose is to
> prevent acquiring multiple references from happening. So, is it possible
> to return NULL from the acquire function if having returned a reference
> before?

The purpose of req_acquired is to allow acquiring a referenced kptr in
struct_ops argument just once. Whether multiple references can be
acquired/duplicated later I think could be orthogonal.

In bpf qdisc, we ensure unique reference of skb through ref_acquired and
the fact that there is no bpf_ref_count in sk_buff (so that users cannot
use bpf_ref_acquire()).

In this case, it is true that programs like below will be able to get
multiple references to task (Is this the scenario you have in mind?).
Thus, if the users want to enforce the unique reference semantic, they
need to make bpf_task_acquire() unavailable as well.

SEC("struct_ops/test_ref_acquire")
int BPF_PROG(test_ref_acquire, int dummy,
             struct task_struct *task)
{
        struct task_struct task2;
        task2 = bpf_task_acquire(task);
        bpf_task_release(task);
        if (task2)
            bpf_task_release(task2);
        return 0;
}


>
>
> > +
> > +     return 0;
> > +}
> > +
> > +
> ... skipped ...