| 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
| ||
|
Message-ID: <CAP01T74mQPMktHJiPoZ7z-UfFCRoxOexpBe_X2v3rLpE5A+WEA@mail.gmail.com>
Date: Fri, 17 May 2024 02:23:37 +0200
From: Kumar Kartikeya Dwivedi <memxor@...il.com>
To: Amery Hung <ameryhung@...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,
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 v8 01/20] bpf: Support passing referenced kptr to
struct_ops programs
On Fri, 17 May 2024 at 02:17, Amery Hung <ameryhung@...il.com> wrote:
>
> On Thu, May 16, 2024 at 4:59 PM Kumar Kartikeya Dwivedi
> <memxor@...il.com> wrote:
> >
> > On Fri, 10 May 2024 at 21:24, Amery Hung <ameryhung@...il.com> wrote:
> > >
> > > This patch supports struct_ops programs that acqurie referenced kptrs
> > > throguh arguments. In Qdisc_ops, an skb is passed to ".enqueue" in the
> > > first argument. The qdisc becomes the sole owner of the skb and must
> > > enqueue or drop the skb. This matches the referenced kptr semantic
> > > in bpf. However, the existing practice of acquiring a referenced kptr via
> > > a kfunc with KF_ACQUIRE does not play well in this case. Calling kfuncs
> > > repeatedly allows the user to acquire multiple references, while there
> > > should be only one reference to a unique skb in a qdisc.
> > >
> > > The solutioin is to make a struct_ops program automatically acquire a
> > > referenced kptr through a tagged argument in the stub function. When
> > > tagged with "__ref_acquired" (suggestion for a better name?), an
> > > reference kptr (ref_obj_id > 0) will be acquired automatically when
> > > entering the program. In addition, only the first read to the arguement
> > > is allowed and it will yeild a referenced kptr.
> > >
> > > Signed-off-by: Amery Hung <amery.hung@...edance.com>
> > > ---
> > > include/linux/bpf.h | 3 +++
> > > kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++----
> > > kernel/bpf/btf.c | 10 +++++++++-
> > > kernel/bpf/verifier.c | 16 +++++++++++++---
> > > 4 files changed, 38 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 9c6a7b8ff963..6aabca1581fe 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -914,6 +914,7 @@ struct bpf_insn_access_aux {
> > > struct {
> > > struct btf *btf;
> > > u32 btf_id;
> > > + u32 ref_obj_id;
> > > };
> > > };
> > > struct bpf_verifier_log *log; /* for verbose logs */
> > > @@ -1416,6 +1417,8 @@ struct bpf_ctx_arg_aux {
> > > enum bpf_reg_type reg_type;
> > > struct btf *btf;
> > > u32 btf_id;
> > > + u32 ref_obj_id;
> > > + bool ref_acquired;
> > > };
> > >
> > > struct btf_mod_pair {
> > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> > > index 86c7884abaf8..bca8e5936846 100644
> > > --- a/kernel/bpf/bpf_struct_ops.c
> > > +++ b/kernel/bpf/bpf_struct_ops.c
> > > @@ -143,6 +143,7 @@ void bpf_struct_ops_image_free(void *image)
> > > }
> > >
> > > #define MAYBE_NULL_SUFFIX "__nullable"
> > > +#define REF_ACQUIRED_SUFFIX "__ref_acquired"
> > > #define MAX_STUB_NAME 128
> > >
> > > /* Return the type info of a stub function, if it exists.
> > > @@ -204,6 +205,7 @@ static int prepare_arg_info(struct btf *btf,
> > > struct bpf_struct_ops_arg_info *arg_info)
> > > {
> > > const struct btf_type *stub_func_proto, *pointed_type;
> > > + bool is_nullable = false, is_ref_acquired = false;
> > > const struct btf_param *stub_args, *args;
> > > struct bpf_ctx_arg_aux *info, *info_buf;
> > > u32 nargs, arg_no, info_cnt = 0;
> > > @@ -240,8 +242,11 @@ static int prepare_arg_info(struct btf *btf,
> > > /* Skip arguments that is not suffixed with
> > > * "__nullable".
> > > */
> > > - if (!btf_param_match_suffix(btf, &stub_args[arg_no],
> > > - MAYBE_NULL_SUFFIX))
> > > + is_nullable = btf_param_match_suffix(btf, &stub_args[arg_no],
> > > + MAYBE_NULL_SUFFIX);
> > > + is_ref_acquired = btf_param_match_suffix(btf, &stub_args[arg_no],
> > > + REF_ACQUIRED_SUFFIX);
> > > + if (!(is_nullable || is_ref_acquired))
> > > continue;
> > >
> > > /* Should be a pointer to struct */
> > > @@ -269,11 +274,15 @@ static int prepare_arg_info(struct btf *btf,
> > > }
> > >
> > > /* Fill the information of the new argument */
> > > - info->reg_type =
> > > - PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > > info->btf_id = arg_btf_id;
> > > info->btf = btf;
> > > info->offset = offset;
> > > + if (is_nullable) {
> > > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID | PTR_MAYBE_NULL;
> > > + } else if (is_ref_acquired) {
> > > + info->reg_type = PTR_TRUSTED | PTR_TO_BTF_ID;
> > > + info->ref_acquired = true;
> > > + }
> > >
> > > info++;
> > > info_cnt++;
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 8c95392214ed..e462fb4a4598 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6316,7 +6316,8 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > >
> > > /* this is a pointer to another type */
> > > for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
> > > - const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
> > > + struct bpf_ctx_arg_aux *ctx_arg_info =
> > > + (struct bpf_ctx_arg_aux *)&prog->aux->ctx_arg_info[i];
> > >
> > > if (ctx_arg_info->offset == off) {
> > > if (!ctx_arg_info->btf_id) {
> > > @@ -6324,9 +6325,16 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > return false;
> > > }
> > >
> > > + if (ctx_arg_info->ref_acquired && !ctx_arg_info->ref_obj_id) {
> > > + bpf_log(log, "cannot acquire a reference to context argument offset %u\n", off);
> > > + return false;
> > > + }
> > > +
> > > info->reg_type = ctx_arg_info->reg_type;
> > > info->btf = ctx_arg_info->btf ? : btf_vmlinux;
> > > info->btf_id = ctx_arg_info->btf_id;
> > > + info->ref_obj_id = ctx_arg_info->ref_obj_id;
> > > + ctx_arg_info->ref_obj_id = 0;
> > > return true;
> >
> > I think this is fragile. What if the compiler produces two independent
> > paths in the program which read the skb pointer once?
> > Technically, the program is still reading the skb pointer once at runtime.
> > Then you will reset ref_obj_id to 0 when exploring one, and assign as
> > 0 in the other one, causing errors.
> > ctx_arg_info appears to be global for the program.
> >
> > I think the better way would be to check if ref_obj_id is still part
> > of the reference state.
> > If the ref_obj_id has already been dropped from reference_state, then
> > any loads should get ref_obj_id = 0.
> > That would happen when dropping or enqueueing the skb into qdisc,
> > which would (I presume) do release_reference_state(ref_obj_id).
> > Otherwise, all of them can share the same ref_obj_id. You won't have
> > to implement "can only read once" logic,
> > and when you enqueue stuff in the qdisc, all identical copies produced
> > from different load instructions will be invalidated.
> > Same ref_obj_id == unique ownership of the same object.
> > You can already have multiple copies through rX = rY, multiple ctx
> > loads of skb will produce a similar verifier state.
> >
> > So, on entry, assign ctx_arg_info->ref_obj_id uniquely, then on each load:
> > if reference_state.find(ctx_arg_info->ref_obj_id) == true; then
> > info->ref_obj_id = ctx_arg_info->ref_obj_id; else info->ref_obj_id =
> > 0;
> >
> > Let me know if I missed something.
>
> You are right. The current approach will falsely reject valid programs,
> and your suggestion makes sense.
Also, I wonder whether when ref_obj_id has been released, we should
mark the loaded register as unknown scalar, vs skb with ref_obj_id =
0?
Otherwise right now it will take PTR_TO_BTF_ID | PTR_TRUSTED as
reg_type, and I think verifier will permit reads even if ref_obj_id =
0.
This will surely be bad once skb is dropped/enqueued, since the
program should no longer be able to read such memory.
>
> Thanks,
> Amery
Powered by blists - more mailing lists