On 10/3/22 2:03 PM, Kumar Kartikeya Dwivedi wrote:
On Mon, 3 Oct 2022 at 21:53, Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxxxxxxx
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,
Ah, it seems you have some of the work already. Do you have it in somewhere
like github? Is the 'trusting ctx loads' piece small enough that can be landed
together with this set first?
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.
