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@gmail.com>
Signed-off-by: Jiayuan Chen <jiayuan.chen@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)) {
+ struct sk_msg *enc = &ctx->open_rec->msg_encrypted;
+
+ sk_msg_trim(sk, enc, enc->sg.size - delta);
+ }
+ }
}
if (msg->cork_bytes && msg->cork_bytes > msg->sg.size &&
!enospc && !full_record) {
--
2.47.1On 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@gmail.com>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@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:
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
2025/5/30 02:16, "Cong Wang" <xiyou.wangcong@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.wangcong@gmail.com>
> >
> > Signed-off-by: Jiayuan Chen <jiayuan.chen@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
>
On 2025-06-02 11:04:50, Jiayuan Chen wrote: > 2025/5/30 02:16, "Cong Wang" <xiyou.wangcong@gmail.com> 写到: > > > > > On Fri, May 23, 2025 at 09:18:58PM +0800, Jiayuan Chen wrote: > > [...] > > > > 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. If we never see the NULL lets just drop the WARN. In general we prefer not to scatter warns everywhere. Can you resubmit without it? Thanks! John
On 2025-05-23 21:18:58, 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@gmail.com>
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> ---
LGTM thanks.
Reviewed-by: John Fastabend <john.fastabend@gmail.com>
© 2016 - 2025 Red Hat, Inc.