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 17:54:41 -0700
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Kui-Feng Lee <sinquersw@...il.com>, 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

On Thu, May 16, 2024 at 4:45 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 5/16/24 4:14 PM, Amery Hung wrote:
> > 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.
>
> The part that no skb acquire kfunc should be available to the qdisc struct_ops
> prog is understood. I think it just needs to clarify the commit message and
> remove the "It must be released and cannot be acquired more than once" part.
>

Got it. I will improve the clarity of the commit message.

In addition, I will also remove "struct_ops_ref_acquire_dup_ref.c" as
whether duplicate references can be acquired through kfunc is out of
scope (should be taken care of by struct_ops implementer). Actually,
this testcase should load the and it does load...

As for the name, do you have any thoughts?

Thanks,
Amery

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