netdev - Re: [PATCH bpf-next v9 03/12] bpf: stop unsafely accessing TCP fields in bpf callbacks

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]

Message-ID: <CAL+tcoDVGLnoWSSg7TGjnQDvq+etUy6m=XBUBDDmcQZp3GQ1bA@mail.gmail.com>
Date: Tue, 11 Feb 2025 16:08:02 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemdebruijn.kernel@...il.com, 
	willemb@...gle.com, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, 
	eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, horms@...nel.org, bpf@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v9 03/12] bpf: stop unsafely accessing TCP fields
 in bpf callbacks

On Tue, Feb 11, 2025 at 2:34 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 2/8/25 2:32 AM, Jason Xing wrote:
> > The "is_locked_tcp_sock" flag is added to indicate that the callback
> > site has a tcp_sock locked.
>
> It should mention that the later TX timestamping callbacks will not own the
> lock. This is what this patch is primarily for. We know the background, but
> future code readers may not. We will eventually become the readers of this patch
> in a few years' time.
>
> >
> > Apply the new member is_locked_tcp_sock in the existing callbacks
>
> It is hard to read "Apply the new member....". "Apply" could mean a few things.
> "Set to 1" is clearer.
>
>
> > where is_fullsock is set to 1 can stop UDP socket accessing struct
>
> The UDP part is future proof. This set does not support UDP which has to be
> clear in the commit message. This has been brought up before also.
>
> > tcp_sock and stop TCP socket without sk lock protecting does the
> > similar thing, or else it could be catastrophe leading to panic.
> >
> > To keep it simple, instead of distinguishing between read and write
> > access, users aren't allowed all read/write access to the tcp_sock
> > through the older bpf_sock_ops ctx. The new timestamping callbacks
> > can use newer helpers to read everything from a sk (e.g. bpf_core_cast),
> > so nothing is lost.
>
> (Subject):
> bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback
>
> (Why):
> The subsequent patch will implement BPF TX timestamping. It will call the
> sockops BPF program without holding the sock lock.
>
> This breaks the current assumption that all sock ops programs will hold the sock
> lock. The sock's fields of the uapi's bpf_sock_ops requires this assumption.
>
> (What and How):
> To address this,
> a new "u8 is_locked_tcp_sock;" field is added. This patch sets it in the current
> sock_ops callbacks. The "is_fullsock" test is then replaced by the
> "is_locked_tcp_sock" test during sock_ops_convert_ctx_access().
>
> The new TX timestamping callbacks added in the subsequent patch will not have
> this set. This will prevent unsafe access from the new timestamping callbacks.
>
> Potentially, we could allow read-only access. However, this would require
> identifying which callback is read-safe-only and also requires additional BPF
> instruction rewrites in the covert_ctx. Since the BPF program can always read
> everything from a socket (e.g., by using bpf_core_cast), this patch keeps it
> simple and disables all read and write access to any socket fields through the
> bpf_sock_ops UAPI from the new TX timestamping callback.
>
> Moreover, note that some of the fields in bpf_sock_ops are specific to tcp_sock,
> and sock_ops currently only supports tcp_sock. In the future, UDP timestamping
> will be added, which will also break this assumption. The same idea used in this
> patch will be reused. Considering that the current sock_ops only supports
> tcp_sock, the variable is named is_locked_"tcp"_sock.

Thanks so much for polishing the commit message. I appreciate it!

Will adjust it.

Thanks,
Jason