On 7/23/24 01:20, Kumar Kartikeya Dwivedi wrote:
On Thu, 11 Jul 2024 at 13:26, Juntong Deng <juntong.deng@xxxxxxxxxxx> wrote:
Currently we have only three ways to get valid pointers:
1. Pointers which are passed as tracepoint or struct_ops
callback arguments.
2. Pointers which were returned from a KF_ACQUIRE kfunc.
3. Guaranteed valid nested pointers (e.g. using the
BTF_TYPE_SAFE_TRUSTED macro)
But this does not cover all cases and we cannot get valid
pointers to some objects, causing the chain of trust to be
broken (we cannot get a valid object pointer from another
valid object pointer).
The following are some examples of cases that are not covered:
1. struct socket
There is no reference counting in a struct socket, the reference
counting is actually in the struct file, so it does not make sense
to use a combination of KF_ACQUIRE and KF_RELEASE to trick the
verifier to make the pointer to struct socket valid.
Yes, but the KF_OBTAIN like flag also needs to ensure that lifetime
relationships are reflected in the verifier state.
If we return a trusted pointer A using bpf_sock_from_file, but
argument B it takes is later released, the verifier needs to ensure
that the pointer A whose lifetime belongs to that pointer B also gets
scrubbed.
Thanks for your review!
You are right, I will fix this problem in the next version of the patch.
2. sk_write_queue in struct sock
sk_write_queue is a struct member in struct sock, not a pointer
member, so we cannot use the guaranteed valid nested pointer method
to get a valid pointer to sk_write_queue.
I think Matt recently had a patch addressing this issue:
https://lore.kernel.org/bpf/20240709210939.1544011-1-mattbobrowski@xxxxxxxxxx/
I believe that should resolve this one (as far as passing them into
KF_TRUSTED_ARGS kfuncs is concerned atleast).
Thanks for letting me know.
I tested it and it works well in most cases, but there are a few cases
that are not fully resolved.
Yes, the verifier has relaxed the constraint on non-zero offset
pointers, but the type of the pointer does not change.
This means that passing non-zero offset pointers as arguments to kfuncs
with KF_ACQUIRE will be rejected by the verifier because KF_ACQUIRE
requires strict type match and casting cannot be used.
An example, bpf_skb_peek_tail:
# ; struct sk_buff *skb = bpf_skb_peek_tail(head);
@ test_restore_udp_socket.bpf.c:209
# 75: (bf) r1 = r2 ;
frame2: R1_w=ptr_sock(ref_obj_id=6,off=168)
R2=ptr_sock(ref_obj_id=6,off=168) refs=4,6
# 76: (85) call bpf_skb_peek_tail#101113
# kernel function bpf_skb_peek_tail args#0 expected pointer to
STRUCT sk_buff_head but R1 has a pointer to STRUCT sock
Should we relax the strict type-matching constraint on non-zero offset
pointers when used as arguments to kfuncs with KF_ACQUIRE?
In addition, this method is not portable (such as &task->cpus_mask),
and the offset of the member will change once a new structure member
is added, or an old structure member is removed.
Now that we have relaxed the constraints on non-zero offset pointers,
this method will probably be used a lot, maybe we could add a
BPF_CORE_POINTER macro to get a pointer to a struct member?
(Different from BPF_CORE_READ, which is reading member contents)
For example, BPF_CORE_POINTER(task, cpus_mask), which is a simple
user-friendly wrapper for __builtin_preserve_access_index.
3. The pointer returned by iterator next method
Currently we cannot pass the pointer returned by the iterator next
method as argument to the KF_TRUSTED_ARGS kfuncs, because the pointer
returned by the iterator next method is not "valid".
This does sound ok though.
This patch adds the KF_OBTAIN flag to solve examples 1 and 2, for cases
where a valid pointer can be obtained without manipulating the reference
count. For KF_OBTAIN kfuncs, the arguments must be valid pointers.
KF_OBTAIN kfuncs guarantees that if the passed pointer argument is valid,
then the pointer returned by KF_OBTAIN kfuncs is also valid.
For example, bpf_socket_from_file() is KF_OBTAIN, and if the struct file
pointer passed in is valid (KF_ACQUIRE), then the struct socket pointer
returned is also valid. Another example, bpf_receive_queue_from_sock() is
KF_OBTAIN, and if the struct sock pointer passed in is valid, then the
sk_receive_queue pointer returned is also valid.
In addition, this patch sets the pointer returned by the iterator next
method to be valid. This is based on the fact that if the iterator is
implemented correctly, then the pointer returned from the iterator next
method should be valid. This does not make the NULL pointer valid.
If the iterator next method has the KF_RET_NULL flag, then the verifier
will ask the ebpf program to check the NULL pointer.
Signed-off-by: Juntong Deng <juntong.deng@xxxxxxxxxxx>
---
I think you should look at bpf_tcp_sock helper (and others), which
converts struct bpf_sock to bpf_tcp_sock. It also transfers the
ref_obj_id into the return value to ensure ownership is reflected
correctly regardless of the type. That pattern has a specific name
(is_ptr_cast_function), but idk what to call this.
Thanks for mentioning this part of the code!
I tried it, it was very helpful.
After setting ref_obj_id, once pointer A (passed into KF_OBTAIN kfuncs)
is released, then pointer B (returned from KF_OBTAIN kfuncs)
becomes invalid.
I will include this fix in the next version of the patch.
