Re: [PATCH bpf-next v1 1/2] bpf,ktls: Fix data corruption when using bpf_msg_pop_data() in ktls

[Jiayuan Chen]

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



> 
> 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@xxxxxxxxx>
> > 
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> > 
> >  ---
> > 
> >  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
>