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

Message-ID: <gkx7axo3mau4jb7ojsdl4lwrtkuxsbnozplupscl3vvl3zfqg5@qnc5qfwzcwlj>
Date: Fri, 24 Jan 2025 21:24:48 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Jiayuan Chen <mrpre@....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 2025-01-24 01:15:51, Jiayuan Chen wrote:
> [ 2172.936997] ------------[ cut here ]------------
> [ 2172.936999] kernel BUG at lib/iov_iter.c:629!
> ......
> [ 2172.944996] PKRU: 55555554
> [ 2172.945155] Call Trace:
> [ 2172.945299]  <TASK>
> [ 2172.945428]  ? die+0x36/0x90
> [ 2172.945601]  ? do_trap+0xdd/0x100
> [ 2172.945795]  ? iov_iter_revert+0x178/0x180
> [ 2172.946031]  ? iov_iter_revert+0x178/0x180
> [ 2172.946267]  ? do_error_trap+0x7d/0x110
> [ 2172.946499]  ? iov_iter_revert+0x178/0x180
> [ 2172.946736]  ? exc_invalid_op+0x50/0x70
> [ 2172.946961]  ? iov_iter_revert+0x178/0x180
> [ 2172.947197]  ? asm_exc_invalid_op+0x1a/0x20
> [ 2172.947446]  ? iov_iter_revert+0x178/0x180
> [ 2172.947683]  ? iov_iter_revert+0x5c/0x180
> [ 2172.947913]  tls_sw_sendmsg_locked.isra.0+0x794/0x840
> [ 2172.948206]  tls_sw_sendmsg+0x52/0x80
> [ 2172.948420]  ? inet_sendmsg+0x1f/0x70
> [ 2172.948634]  __sys_sendto+0x1cd/0x200
> [ 2172.948848]  ? find_held_lock+0x2b/0x80
> [ 2172.949072]  ? syscall_trace_enter+0x140/0x270
> [ 2172.949330]  ? __lock_release.isra.0+0x5e/0x170
> [ 2172.949595]  ? find_held_lock+0x2b/0x80
> [ 2172.949817]  ? syscall_trace_enter+0x140/0x270
> [ 2172.950211]  ? lockdep_hardirqs_on_prepare+0xda/0x190
> [ 2172.950632]  ? ktime_get_coarse_real_ts64+0xc2/0xd0
> [ 2172.951036]  __x64_sys_sendto+0x24/0x30
> [ 2172.951382]  do_syscall_64+0x90/0x170
> ......
> 
> After calling bpf_exec_tx_verdict(), the size of msg_pl->sg may increase,
> e.g., when the BPF program executes bpf_msg_push_data().
> 
> If the BPF program sets cork_bytes and sg.size is smaller than cork_bytes,
> it will return -ENOSPC and attempt to roll back to the non-zero copy
> logic. However, during rollback, msg->msg_iter is reset, but since
> msg_pl->sg.size has been increased, subsequent executions will exceed the
> actual size of msg_iter.
> '''
> iov_iter_revert(&msg->msg_iter, msg_pl->sg.size - orig_size);
> '''
> 
> The changes in this commit are based on the following considerations:
> 
> 1. When cork_bytes is set, rolling back to non-zero copy logic is
> 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;
        }

> 
> This current change is based on the principle of minimal modification,
> which won't make things worse. If we still encounter similar panic
> further, we can consider a more comprehensive solution.
> 
> Fixes: fcb14cb1bdac ("new iov_iter flavour - ITER_UBUF")
> Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling")
> Signed-off-by: Jiayuan Chen <mrpre@....com>
> ---
>  net/tls/tls_sw.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> 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?

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.