Re: [RFC PATCH bpf-next RESEND 03/16] bpf: Improve bpf kfuncs pointer arguments chain of trust

[Juntong Deng]

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.