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: <b2486867-0fee-4972-ad71-7b54e8a5d2b6@gmail.com>
Date: Fri, 10 May 2024 14:33:31 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Amery Hung <ameryhung@...il.com>, netdev@...r.kernel.org
Cc: 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 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?


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