* Re: Patch "bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback" has been added to the 6.1-stable tree [not found] <20250522231656.3254864-1-sashal@kernel.org> @ 2025-05-22 23:25 ` Jason Xing 2025-05-23 23:17 ` Martin KaFai Lau 0 siblings, 1 reply; 3+ messages in thread From: Jason Xing @ 2025-05-22 23:25 UTC (permalink / raw) To: stable Cc: stable-commits, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, David Ahern On Fri, May 23, 2025 at 7:17 AM Sasha Levin <sashal@kernel.org> wrote: > > This is a note to let you know that I've just added the patch titled > > bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback > > to the 6.1-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > The filename of the patch is: > bpf-prevent-unsafe-access-to-the-sock-fields-in-the-.patch > and it can be found in the queue-6.1 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable@vger.kernel.org> know about it. Hi, I'm notified that this patch has been added into many branches, which is against my expectations. The BPF timestaping feature was implemented in 6.14 and the patch you are handling is just one of them. The function of this patch prevents unexpected bpf programs using this feature from triggering fatal problems. So, IMHO, we don't need this patch in all the older/stable branches:) Thanks, Jason > > > > commit 00b709040e0fdf5949dfbf02f38521e0b10943ac > Author: Jason Xing <kerneljasonxing@gmail.com> > Date: Thu Feb 20 15:29:31 2025 +0800 > > bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback > > [ Upstream commit fd93eaffb3f977b23bc0a48d4c8616e654fcf133 ] > > 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. > > 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. > > Signed-off-by: Jason Xing <kerneljasonxing@gmail.com> > Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> > Link: https://patch.msgid.link/20250220072940.99994-4-kerneljasonxing@gmail.com > Signed-off-by: Sasha Levin <sashal@kernel.org> > > diff --git a/include/linux/filter.h b/include/linux/filter.h > index f3ef1a8965bb2..09cc8fb735f02 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -1319,6 +1319,7 @@ struct bpf_sock_ops_kern { > void *skb_data_end; > u8 op; > u8 is_fullsock; > + u8 is_locked_tcp_sock; > u8 remaining_opt_len; > u64 temp; /* temp and everything after is not > * initialized to 0 before calling > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 83e0362e3b721..63caa3181dfe6 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -2409,6 +2409,7 @@ static inline int tcp_call_bpf(struct sock *sk, int op, u32 nargs, u32 *args) > memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp)); > if (sk_fullsock(sk)) { > sock_ops.is_fullsock = 1; > + sock_ops.is_locked_tcp_sock = 1; > sock_owned_by_me(sk); > } > > diff --git a/net/core/filter.c b/net/core/filter.c > index 497b41ac399da..5c9f3fcb957bb 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -10240,10 +10240,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type, > } \ > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \ > struct bpf_sock_ops_kern, \ > - is_fullsock), \ > + is_locked_tcp_sock), \ > fullsock_reg, si->src_reg, \ > offsetof(struct bpf_sock_ops_kern, \ > - is_fullsock)); \ > + is_locked_tcp_sock)); \ > *insn++ = BPF_JMP_IMM(BPF_JEQ, fullsock_reg, 0, jmp); \ > if (si->dst_reg == si->src_reg) \ > *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \ > @@ -10328,10 +10328,10 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type, > temp)); \ > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \ > struct bpf_sock_ops_kern, \ > - is_fullsock), \ > + is_locked_tcp_sock), \ > reg, si->dst_reg, \ > offsetof(struct bpf_sock_ops_kern, \ > - is_fullsock)); \ > + is_locked_tcp_sock)); \ > *insn++ = BPF_JMP_IMM(BPF_JEQ, reg, 0, 2); \ > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \ > struct bpf_sock_ops_kern, sk),\ > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index db1a99df29d55..16f4a41a068e4 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -168,6 +168,7 @@ static void bpf_skops_parse_hdr(struct sock *sk, struct sk_buff *skb) > memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp)); > sock_ops.op = BPF_SOCK_OPS_PARSE_HDR_OPT_CB; > sock_ops.is_fullsock = 1; > + sock_ops.is_locked_tcp_sock = 1; > sock_ops.sk = sk; > bpf_skops_init_skb(&sock_ops, skb, tcp_hdrlen(skb)); > > @@ -184,6 +185,7 @@ static void bpf_skops_established(struct sock *sk, int bpf_op, > memset(&sock_ops, 0, offsetof(struct bpf_sock_ops_kern, temp)); > sock_ops.op = bpf_op; > sock_ops.is_fullsock = 1; > + sock_ops.is_locked_tcp_sock = 1; > sock_ops.sk = sk; > /* sk with TCP_REPAIR_ON does not have skb in tcp_finish_connect */ > if (skb) > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 40568365cdb3b..2f109f1968253 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -509,6 +509,7 @@ static void bpf_skops_hdr_opt_len(struct sock *sk, struct sk_buff *skb, > sock_owned_by_me(sk); > > sock_ops.is_fullsock = 1; > + sock_ops.is_locked_tcp_sock = 1; > sock_ops.sk = sk; > } > > @@ -554,6 +555,7 @@ static void bpf_skops_write_hdr_opt(struct sock *sk, struct sk_buff *skb, > sock_owned_by_me(sk); > > sock_ops.is_fullsock = 1; > + sock_ops.is_locked_tcp_sock = 1; > sock_ops.sk = sk; > } > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Patch "bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback" has been added to the 6.1-stable tree 2025-05-22 23:25 ` Patch "bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback" has been added to the 6.1-stable tree Jason Xing @ 2025-05-23 23:17 ` Martin KaFai Lau 2025-05-27 14:53 ` Greg KH 0 siblings, 1 reply; 3+ messages in thread From: Martin KaFai Lau @ 2025-05-23 23:17 UTC (permalink / raw) To: Jason Xing, stable Cc: stable-commits, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, David Ahern On 5/22/25 4:25 PM, Jason Xing wrote: > On Fri, May 23, 2025 at 7:17 AM Sasha Levin <sashal@kernel.org> wrote: >> >> This is a note to let you know that I've just added the patch titled >> >> bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback >> >> to the 6.1-stable tree which can be found at: >> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary >> >> The filename of the patch is: >> bpf-prevent-unsafe-access-to-the-sock-fields-in-the-.patch >> and it can be found in the queue-6.1 subdirectory. >> >> If you, or anyone else, feels it should not be added to the stable tree, >> please let <stable@vger.kernel.org> know about it. > > Hi, > > I'm notified that this patch has been added into many branches, which > is against my expectations. The BPF timestaping feature was > implemented in 6.14 and the patch you are handling is just one of them. > > The function of this patch prevents unexpected bpf programs using this > feature from triggering > fatal problems. So, IMHO, we don't need this patch in all the > older/stable branches:) > > Thanks, > Jason > > >> >> >> >> commit 00b709040e0fdf5949dfbf02f38521e0b10943ac >> Author: Jason Xing <kerneljasonxing@gmail.com> >> Date: Thu Feb 20 15:29:31 2025 +0800 >> >> bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback >> >> [ Upstream commit fd93eaffb3f977b23bc0a48d4c8616e654fcf133 ] >> >> The subsequent patch will implement BPF TX timestamping. It will Agree. The patch is a preparation work for the new bpf tx timestamping feature. It is not stable material. Thanks, Martin >> 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. >> >> 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. >> >> Signed-off-by: Jason Xing <kerneljasonxing@gmail.com> >> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> >> Link: https://patch.msgid.link/20250220072940.99994-4-kerneljasonxing@gmail.com >> Signed-off-by: Sasha Levin <sashal@kernel.org> ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Patch "bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback" has been added to the 6.1-stable tree 2025-05-23 23:17 ` Martin KaFai Lau @ 2025-05-27 14:53 ` Greg KH 0 siblings, 0 replies; 3+ messages in thread From: Greg KH @ 2025-05-27 14:53 UTC (permalink / raw) To: Martin KaFai Lau Cc: Jason Xing, stable, stable-commits, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Eric Dumazet, Neal Cardwell, Kuniyuki Iwashima, David S. Miller, Jakub Kicinski, Paolo Abeni, Simon Horman, David Ahern On Fri, May 23, 2025 at 04:17:21PM -0700, Martin KaFai Lau wrote: > On 5/22/25 4:25 PM, Jason Xing wrote: > > On Fri, May 23, 2025 at 7:17 AM Sasha Levin <sashal@kernel.org> wrote: > > > > > > This is a note to let you know that I've just added the patch titled > > > > > > bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback > > > > > > to the 6.1-stable tree which can be found at: > > > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > > > > > The filename of the patch is: > > > bpf-prevent-unsafe-access-to-the-sock-fields-in-the-.patch > > > and it can be found in the queue-6.1 subdirectory. > > > > > > If you, or anyone else, feels it should not be added to the stable tree, > > > please let <stable@vger.kernel.org> know about it. > > > > Hi, > > > > I'm notified that this patch has been added into many branches, which > > is against my expectations. The BPF timestaping feature was > > implemented in 6.14 and the patch you are handling is just one of them. > > > > The function of this patch prevents unexpected bpf programs using this > > feature from triggering > > fatal problems. So, IMHO, we don't need this patch in all the > > older/stable branches:) > > > > Thanks, > > Jason > > > > > > > > > > > > > > > > commit 00b709040e0fdf5949dfbf02f38521e0b10943ac > > > Author: Jason Xing <kerneljasonxing@gmail.com> > > > Date: Thu Feb 20 15:29:31 2025 +0800 > > > > > > bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback > > > > > > [ Upstream commit fd93eaffb3f977b23bc0a48d4c8616e654fcf133 ] > > > > > > The subsequent patch will implement BPF TX timestamping. It will > > Agree. The patch is a preparation work for the new bpf tx timestamping > feature. It is not stable material. Dropped from everywhere, thanks. greg k-h ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-27 14:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250522231656.3254864-1-sashal@kernel.org>
2025-05-22 23:25 ` Patch "bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback" has been added to the 6.1-stable tree Jason Xing
2025-05-23 23:17 ` Martin KaFai Lau
2025-05-27 14:53 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.