2025/5/30 02:16, "Cong Wang" <xiyou.wangc...@gmail.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.wangc...@gmail.com>
> >
> > Signed-off-by: Jiayuan Chen <jiayuan.c...@linux.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
>
