Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221001144716.3403120-1-void@manifault.com> <20221001144716.3403120-3-void@manifault.com>
 <CAP01T74TtMARkfYWsYY0+cnsx2w4axB1LtvF-RFMAihW7v=LUw@mail.gmail.com>
 <YzsBSoGnPEIJADSH@maniforge.dhcp.thefacebook.com> <CAP01T76OR3J_P8YMq4ZgKHBpuZyA0zgsPy+tq9htbX=j6AVyOg@mail.gmail.com>
 <fb3e81b7-8360-5132-59ac-0e74483eb25f@linux.dev>
In-Reply-To: <fb3e81b7-8360-5132-59ac-0e74483eb25f@linux.dev>
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date:   Mon, 3 Oct 2022 23:03:01 +0200
Message-ID: <CAP01T77tCdKTJo=sByg5GsW1OrQmNXV4fmBDKUVtbnwEaJBpVA@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs
To:     Martin KaFai Lau <martin.lau@linux.dev>
Cc:     David Vernet <void@manifault.com>, ast@kernel.org,
        daniel@iogearbox.net, andrii@kernel.org, kernel-team@fb.com,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org, yhs@fb.com,
        song@kernel.org, john.fastabend@gmail.com, kpsingh@kernel.org,
        sdf@google.com, haoluo@google.com, jolsa@kernel.org, tj@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 3 Oct 2022 at 21:53, Martin KaFai Lau <martin.lau@linux.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.