netdev - Re: [RFC PATCH v8 02/20] selftests/bpf: Test referenced kptr arguments of struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 16 May 2024 16:14:55 -0700
From: Amery Hung <ameryhung@...il.com>
To: Kui-Feng Lee <sinquersw@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, yangpeihao@...u.edu.cn, 
	daniel@...earbox.net, andrii@...nel.org, martin.lau@...nel.org, 
	toke@...hat.com, jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com, 
	xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v8 02/20] selftests/bpf: Test referenced kptr
 arguments of struct_ops programs

I thought about patch 1-4 a bit more after the discussion in LSFMMBPF and
I think we should keep what "ref_acquried" does, but maybe rename it to
"ref_moved".

We discussed the lifecycle of skb in qdisc and changes to struct_ops and
bpf semantics. In short, At the beginning of .enqueue, the kernel passes
the ownership of an skb to a qdisc. We do not increase the reference count
of skb since this is an ownership transfer, not kernel and qdisc both
holding references to the skb. (The counterexample can be found in RFC v7.
See how weird skb release kfuncs look[0]). The skb should be either
enqueued or dropped. Then, in .dequeue, an skb will be removed from the
queue and the ownership will be returned to the kernel.

Referenced kptr in bpf already carries the semantic of ownership. Thus,
what we need here is to enable struct_ops programs to get a referenced
kptr from the argument and returning referenced kptr (achieved via patch
1-4).

Proper handling of referenced objects is important for safety reasons.
In the case of bpf qdisc, there are three problematic situations as listed
below, and referenced kptr has taken care of (1) and (2).

(1) .enqueue not enqueueing nor dropping the skb, causing reference leak

(2) .dequeue making up an invalid skb ptr and returning to kernel

(3) If bpf qdisc operators can duplicate skb references, multiple
    references to the same skb can be present. If we enqueue these
    references to a collection and dequeue one, since skb->dev will be
    restored after the skb is removed from the collection, other skb in
    the collection will then have invalid skb->rbnode as "dev" and "rbnode"
    share the same memory.

A discussion point was about introducing and enforcing a unique reference
semantic (PTR_UNIQUE) to mitigate (3). After giving it more thoughts, I
think we should keep "ref_acquired", and be careful about kernel-side
implementation that could return referenced kptr. Taking a step back, (3)
is only problematic because I made an assumption that the kfunc only
increases the reference count of skb (i.e., skb_get()). It could have been
done safely using skb_copy() or maybe pskb_copy(). In other words, it is a
kernel implementation issue, and not a verifier issue. Besides, the
verifier has no knowledge about what a kfunc with KF_ACQUIRE does
internally whatsoever.

In v8, we try to do this safely by only allowing reading "ref_acquired"-
annotated argument once. Since the argument passed to struct_ops never
changes when during a single invocation, it will always be referencing the
same kernel object. Therefore, reading more than once and returning
mulitple references shouldn't be allowed. Maybe "ref_moved" is a more
precise annotation label, hinting that the ownership is transferred.

[0] https://lore.kernel.org/netdev/2d31261b245828d09d2f80e0953e911a9c38573a.1705432850.git.amery.hung@bytedance.com/