netdev - Re: [PATCH bpf v1 1/2] bpf: fix ktls panic

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] [day] [month] [year] [list]

Message-ID: <y4ubu3oa3het5ofmyki52hhxy4uf6abasgfzjmyr4hawfvotjo@mbcb77maqppm>
Date: Fri, 31 Jan 2025 14:20:09 +0800
From: Jiayuan Chen <mrpre@....com>
To: John Fastabend <john.fastabend@...il.com>
Cc: bpf@...r.kernel.org, borisp@...dia.com, kuba@...nel.org, 
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, horms@...nel.org, 
	andrii@...nel.org, eddyz87@...il.com, mykolal@...com, ast@...nel.org, 
	daniel@...earbox.net, martin.lau@...ux.dev, song@...nel.org, yonghong.song@...ux.dev, 
	kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, 
	shuah@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf v1 1/2] bpf: fix ktls panic

On Sat, Jan 25, 2025 at 02:51:49PM +0800, Jiayuan Chen wrote:
> On Fri, Jan 24, 2025 at 09:24:48PM -0800, John Fastabend wrote:
> > On 2025-01-24 01:15:51, Jiayuan Chen wrote:
> > > [ 2172.936997] ------------[ cut here ]------------
> > > [ 2172.936999] kernel BUG at lib/iov_iter.c:629!
> > > ......
> > > pointless and can directly go to zero-copy logic.
> > > 
> > > 2. Suppose sg.size is initially 5, and we push it to 100, setting
> > > apply_bytes to 7. Then, 98 bytes of data are sent out, leaving 2 bytes to
> > > be processed. The rollback logic cannot determine which data has been
> > > processed and which hasn't.
> > 
> > This is the error path we are talking about correct?
> > 
> >         if (msg->cork_bytes && msg->cork_bytes > msg->sg.size &&
> >             !enospc && !full_record) {
> >                 err = -ENOSPC;
> >                 goto out_err;
> >         }
> > 
> yes, it its.
> > > 
> > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> > > index 7bcc9b4408a2..b3cae4dd4f49 100644
> > > --- a/net/tls/tls_sw.c
> > > +++ b/net/tls/tls_sw.c
> > > @@ -1120,9 +1120,13 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
> > >  					num_async++;
> > >  				else if (ret == -ENOMEM)
> > >  					goto wait_for_memory;
> > > -				else if (ctx->open_rec && ret == -ENOSPC)
> > > +				else if (ctx->open_rec && ret == -ENOSPC) {
> > > +					if (msg_pl->cork_bytes) {
> > > +						ret = 0;
> > > +						goto send_end;
> > > +					}
> > 
> > The app will lose bytes here I suspect if we return copied == try_to_copy then
> > no error makes it to the user?
> 
> I looked into the corking logic for non-TLS sockets in tcp_bpf_sendmsg,
> and I found that when a "cork" situation occurs, the user-space send
> doesn't return an error, and the returned length is the same as the input
> length parameter, even if some data is cached. I think TLS should also
> behave similarly.
> 
> Additionally, I saw that the current non-zero-copy logic for handling
> corking is written as:
> '''
> line 1177
> 
> else if (ret != -EAGAIN) {
> 	if (ret == -ENOSPC)
> 		ret = 0;
> 	goto send_end;
> }
> '''
> 
> Meanwhile, I set cork_bytes to 1 and tested the following behavior logic:
> '''
> send(msg, 1);
> send(msg+1, 1);
> send(msg+2, remain_length);
> '''
> Both the sender and receiver seem to be working normally both for TLS and
> non-TLS sockets.
>  
> > Could we return delta from bpf_exec_tx_verdict and then we can calculate
> > the correct number of bytes to revert? I'll need to check but its not
> > clear to me if BPF program pushes data that the right thing is done with
> > delta there now.
> > 
> > Thanks for looking into this.
> 
> Let's assume the original data is "abcdefgh" (8 bytes), and after 3 pushes
> by the BPF program, it becomes 11-byte data: "abc?de?fgh?".
> 
> Then, we set cork_bytes to 6, which means the first 6 bytes have been
> processed, and the remaining 5 bytes "?fgh?" will be cached until the
> length meets the cork_bytes requirement.
> 
> However, some data in "?fgh?" is not within 'sg->msg_iter'
> (but in msg_pl instead), especially the data "?" we pushed.
> 
> So it doesn't seem as simple as just reverting through an offset of msg_iter.
> It appears that 'msg_iter' and 'msg_pl' are two separate objects,
> and the BPF program modifies the scatterlist within the msg_pl.
> 
> --
> Thanks.
> 

Hi John

Just checking in on the status of my patch. If there's any new feedback,
I'm happy to move forward with the next steps.

Regards.