Message-ID: <YztbOo7TgOoN1bVB@maniforge.dhcp.thefacebook.com>
Date:   Mon, 3 Oct 2022 16:59:22 -0500
From:   David Vernet <void@...ifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     Martin KaFai Lau <martin.lau@...ux.dev>, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, kernel-team@...com,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org, yhs@...com,
        song@...nel.org, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, tj@...nel.org
Subject: Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs

On Mon, Oct 03, 2022 at 11:03:01PM +0200, Kumar Kartikeya Dwivedi wrote:
> On Mon, 3 Oct 2022 at 21:53, Martin KaFai Lau <martin.lau@...ux.dev> wrote:
> >
> > On 10/3/22 8:56 AM, Kumar Kartikeya Dwivedi wrote:
> > >>> Also, could you include a test to make sure sleepable programs cannot
> > >>> call bpf_task_acquire? It seems to assume RCU read lock is held while
> > >>> that may not be true. If already not possible, maybe a WARN_ON_ONCE
> > >>> inside the helper to ensure future cases don't creep in.
> > >>
> > >> I don't _think_ it's unsafe for a sleepable program to call
> > >> bpf_task_acquire(). My understanding is that the struct task_struct *
> > >> parameter to bpf_task_acquire() is not PTR_UNTRUSTED, so it's safe to
> > >> dereference directly in the kfunc. The implicit assumption here is that
> > >> the task was either passed to the BPF program (which is calling
> > >> bpf_task_acquire()) from the main kernel in something like a trace or
> > >> struct_ops callback, or it was a referenced kptr that was removed from a
> > >> map with bpf_kptr_xchg(), and is now owned by the BPF program. Given
> > >> that the ptr type is not PTR_UNTRUSTED, it seemed correct to assume that
> > >> the task was valid in bpf_task_acquire() regardless of whether we were
> > >> in an RCU read region or not, but please let me know if I'm wrong about
> > >
> > > I don't think it's correct. You can just walk arbitrary structures and
> > > obtain a normal PTR_TO_BTF_ID that looks seemingly ok to the verifier
> > > but has no lifetime guarantees. It's a separate pre-existing problem
> > > unrelated to your series [0]. PTR_UNTRUSTED is not set for those cases
> > > currently.
> > >
> > > So the argument to your bpf_task_acquire may already be freed by then.
> > > This issue would be exacerbated in sleepable BPF programs, where RCU
> > > read lock is not held, so some cases of pointer walking where it may
> > > be safe no longer holds.
> > >
> > > I am planning to clean this up, but I'd still prefer if we don't allow
> > > this helper in sleepable programs, yet. kptr_get is ok to allow.
> > > Once you have explicit BPF RCU read sections, sleepable programs can
> > > take it, do loads, and operate on the RCU pointer safely until they
> > > invalidate it with the outermost bpf_rcu_read_unlock. It's needed for
> > > local kptrs as well, not just this. I plan to post this very soon, so
> > > we should be able to fix it up in the current cycle after landing your
> > > series.
> > >
> > > __rcu tags in the kernel will automatically be understood by the
> > > verifier and for the majority of the correctly annotated cases this
> > > will work fine and not break tracing programs.
> > >
> > > [0]: https://lore.kernel.org/bpf/CAADnVQJxe1QT5bvcsrZQCLeZ6kei6WEESP5bDXf_5qcB2Bb6_Q@mail.gmail.com
> > >
> > >> that.  Other kfuncs I saw such as bpf_xdp_ct_lookup() assumed that the
> > >> parameter passed by the BPF program (which itself was passing on a
> > >> pointer given to it by the main kernel) is valid as well.
> > >
> > > Yeah, but the CT API doesn't assume validity of random PTR_TO_BTF_ID,
> > > it requires KF_TRUSTED_ARGS which forces them to have ref_obj_id != 0.
> >
> > Other than ref_obj_id != 0, could the PTR_TO_BTF_ID obtained through
> > btf_ctx_access be marked as trusted (e.g. the ctx[0] in the selftest here)
> > and bpf_task_acquire() will require KF_TRUSTED_ARGS? would it be safe in general?
> >
> 
> Recently eed807f62610 ("bpf: Tweak definition of KF_TRUSTED_ARGS")
> relaxed things a bit, now that constraint is only for PTR_TO_BTF_ID
> and it allows other types without ref_obj_id > 0.
> 
> But you're right, ctx loads should usually be trusted, this is the
> current plan (also elaborated a bit in that link [0]), usually that is
> true, an exception is free path as you noted in your reply for patch 1
> (and especially fexit path where object may not even be alive
> anymore). There are some details to work out, but eventually I'd want
> to force KF_TRUSTED_ARGS by default for all kfuncs, and we only make
> exceptions in some special cases by having some KF_UNSAFE flag or
> __unsafe tag for arguments. It's becoming harder to think about all
> permutations if we're too permissive by default in terms of acceptable
> arguments.

Thanks for providing additional context, Kumar. So what do we want to do
for this patch set? IMO it doesn't seem useful to restrict
bpf_kfunc_acquire() to only be callable by non-sleepable programs if our
goal is to avoid crashes for nested task structs. We could easily
accidentally crash if e.g. those pointers are NULL, or someone is doing
something weird like stashing some extra flag bits in unused portions of
the pointer which are masked out when it's actually dereferenced
regardless of whether we're in RCU.  Trusting ctx loads sounds like the
right approach, barring some of the challenges you pointed out such as
dealing with fexit paths after free where the object may not be valid
anymore.

In general, it seems like we should maybe decide on what our policy
should be for kfuncs until we can wire up whatever we need to properly
trust ctx.

Powered by blists - more mailing lists