linux-kernel - Re: [PATCH bpf-next v1 1/2] bpf,ktls: Fix data corruption when using bpf_msg_pop

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: <d99805aaeadd9cd041c9048801084648832a6da1@linux.dev>
Date: Mon, 02 Jun 2025 11:04:50 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "Cong Wang" <xiyou.wangcong@...il.com>
Cc: bpf@...r.kernel.org, "Boris Pismenny" <borisp@...dia.com>, "John
 Fastabend" <john.fastabend@...il.com>, "Jakub Kicinski"
 <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, "Paolo Abeni" <pabeni@...hat.com>, "Simon
 Horman" <horms@...nel.org>, "Andrii Nakryiko" <andrii@...nel.org>,
 "Eduard Zingerman" <eddyz87@...il.com>, "Mykola Lysenko"
 <mykolal@...com>, "Alexei Starovoitov" <ast@...nel.org>, "Daniel
 Borkmann" <daniel@...earbox.net>, "Martin KaFai Lau"
 <martin.lau@...ux.dev>, "Song Liu" <song@...nel.org>, "Yonghong Song"
 <yonghong.song@...ux.dev>, "KP Singh" <kpsingh@...nel.org>, "Stanislav
 Fomichev" <sdf@...ichev.me>, "Hao Luo" <haoluo@...gle.com>, "Jiri Olsa"
 <jolsa@...nel.org>, "Shuah Khan" <shuah@...nel.org>, "Ihor Solodrai"
 <isolodrai@...a.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 1/2] bpf,ktls: Fix data corruption when using
 bpf_msg_pop_data() in ktls

2025/5/30 02:16, "Cong Wang" <xiyou.wangcong@...il.com> 写到:

> 
> On Fri, May 23, 2025 at 09:18:58PM +0800, Jiayuan Chen wrote:
> 
> > 
> > When sending plaintext data, we initially calculated the corresponding
> > 
> >  ciphertext length. However, if we later reduced the plaintext data length
> > 
> >  via socket policy, we failed to recalculate the ciphertext length.
> > 
> >  
> > 
> >  This results in transmitting buffers containing uninitialized data during
> > 
> >  ciphertext transmission.
> > 
> >  
> > 
> >  This causes uninitialized bytes to be appended after a complete
> > 
> >  "Application Data" packet, leading to errors on the receiving end when
> > 
> >  parsing TLS record.
> > 
> >  
> > 
> >  Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> > 
> >  Reported-by: Cong Wang <xiyou.wangcong@...il.com>
> > 
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@...ux.dev>
> > 
> >  ---
> > 
> >  net/tls/tls_sw.c | 15 +++++++++++++++
> > 
> >  1 file changed, 15 insertions(+)
> > 
> >  
> > 
> >  diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > 
> >  index fc88e34b7f33..b23a4655be6a 100644
> > 
> >  --- a/net/tls/tls_sw.c
> > 
> >  +++ b/net/tls/tls_sw.c
> > 
> >  @@ -872,6 +872,21 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
> > 
> >  delta = msg->sg.size;
> > 
> >  psock->eval = sk_psock_msg_verdict(sk, psock, msg);
> > 
> >  delta -= msg->sg.size;
> > 
> >  +
> > 
> >  + if ((s32)delta > 0) {
> > 
> >  + /* It indicates that we executed bpf_msg_pop_data(),
> > 
> >  + * causing the plaintext data size to decrease.
> > 
> >  + * Therefore the encrypted data size also needs to
> > 
> >  + * correspondingly decrease. We only need to subtract
> > 
> >  + * delta to calculate the new ciphertext length since
> > 
> >  + * ktls does not support block encryption.
> > 
> >  + */
> > 
> >  + if (!WARN_ON_ONCE(!ctx->open_rec)) {
> > 
> 
> I am wondering if we need to WARN here? Because the code below this
> 
> handles it gracefully:
> 

Hi Cong

The ctx->open_rec is freed after a TLS record is processed (regardless
of whether the redirect check passes or triggers a redirect).
The 'if (rec)' check in the subsequent code you print is indeed designed
to handle the expected lifecycle state of open_rec.

But the code path I modified should never see a NULL open_rec under normal
operation As this is a bug fix, I need to ensure the fix itself doesn't
create new issues. 

Thanks.

>  931 bool reset_eval = !ctx->open_rec;
> 
>  932 
> 
>  933 rec = ctx->open_rec;
> 
>  934 if (rec) {
> 
>  935 msg = &rec->msg_plaintext;
> 
>  936 if (!msg->apply_bytes)
> 
>  937 reset_eval = true;
> 
>  938 }
> 
>  939 if (reset_eval) {
> 
>  940 psock->eval = __SK_NONE;
> 
>  941 if (psock->sk_redir) {
> 
>  942 sock_put(psock->sk_redir);
> 
>  943 psock->sk_redir = NULL;
> 
>  944 }
> 
>  945 }
> 
> Thanks for fixing it!
> 
> Cong
>