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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Nov 2019 19:25:13 -0800 From: John Fastabend <john.fastabend@...il.com> To: Jakub Kicinski <jakub.kicinski@...ronome.com>, borisp@...lanox.com, aviadye@...lanox.com, john.fastabend@...il.com, daniel@...earbox.net Cc: netdev@...r.kernel.org, Jakub Kicinski <jakub.kicinski@...ronome.com>, syzbot+df0d4ec12332661dd1f9@...kaller.appspotmail.com Subject: RE: [RFC net] net/tls: clear SG markings on encryption error Jakub Kicinski wrote: > When tls_do_encryption() fails the SG lists are left with the > SG_END and SG_CHAIN marks in place. One could hope that once > encryption fails we will never see the record again, but that > is in fact not true. Commit d3b18ad31f93 ("tls: add bpf support > to sk_msg handling") added special handling to ENOMEM and ENOSPC > errors which mean we may see the same record re-submitted. > > In all honesty I don't understand why we need the ENOMEM handling. > Waiting for socket memory without setting SOCK_NOSPACE on any > random memory allocation failure seems slightly ill advised. > > Having said that, undoing the SG markings seems wise regardless. > > Reported-by: syzbot+df0d4ec12332661dd1f9@...kaller.appspotmail.com > Fixes: 130b392c6cd6 ("net: tls: Add tls 1.3 support") > Fixes: d3b18ad31f93 ("tls: add bpf support to sk_msg handling") > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com> > --- > John, I'm sending this mostly to ask if we can safely remove > the ENOMEM handling? :) > What ENOMEM are you asking about here? The return code handling from bpf_exec_tx_verdict? ret = bpf_exec_tx_verdict(msg_pl, sk, full_record, record_type, &copied, msg->msg_flags); if (ret) { if (ret == -EINPROGRESS) num_async++; else if (ret == -ENOMEM) goto wait_for_memory; else if (ret == -ENOSPC) goto rollback_iter; else if (ret != -EAGAIN) goto send_end; } I would want to run it through some of our tests but I don't think there is anything specific about BPF that needs it to be handled. I was just trying to handle the error case gracefully and wait_for_memory seems like the right behavior to me. What is ill advised here? > I was going to try the sockmap tests myself, but looks like the current > LLVM 10 build I get from their debs just segfaults when trying to build > selftest :/ > > Also there's at least one more bug in this piece of code, TLS 1.3 > can't assume there's at least one free SG entry. There should always be one free SG entry at the end of the ring that is used for chaining. >From sk_msg_sg{} /* The extra element is used for chaining the front and sections when * the list becomes partitioned (e.g. end < start). The crypto APIs * require the chaining. */ struct scatterlist data[MAX_MSG_FRAGS + 1]; Can we use that element in that case? Otherwise probably can add an extra element there if needed, data[MAX_MSG_FRAGS + 2]. > > net/tls/tls_sw.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index 24161750a737..4a0ea87b20cf 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -737,6 +737,19 @@ static int tls_push_record(struct sock *sk, int flags, > if (rc < 0) { > if (rc != -EINPROGRESS) { > tls_err_abort(sk, EBADMSG); > + > + i = msg_pl->sg.end; > + if (prot->version == TLS_1_3_VERSION) { > + sg_mark_end(sk_msg_elem(msg_pl, i)); > + sg_unmark_end(sk_msg_elem(msg_pl, i)); > + } > + sk_msg_iter_var_prev(i); > + sg_unmark_end(sk_msg_elem(msg_pl, i)); > + > + i = msg_en->sg.end; > + sk_msg_iter_var_prev(i); > + sg_unmark_end(sk_msg_elem(msg_en, i)); > + > if (split) { > tls_ctx->pending_open_record_frags = true; > tls_merge_open_record(sk, rec, tmp, orig_end); > -- > 2.23.0 > Can you copy the tls_push_record() error handling from BPF side instead of embedding more into tls_push_record itself? err = tls_push_record(sk, flags, record_type); if (err < 0) { *copied -= sk_msg_free(sk, msg); tls_free_open_rec(sk); goto out_err; } If the BPF program is not installed I guess you can skip the copied part because you wont have the 'more_data' case. So something like (untested/compiled/etc) diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 141da093ff04..0469eb73bc88 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -771,8 +771,14 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk, policy = !(flags & MSG_SENDPAGE_NOPOLICY); psock = sk_psock_get(sk); - if (!psock || !policy) - return tls_push_record(sk, flags, record_type); + if (!psock || !policy) { + err = tls_push_record(sk, flags, record_type); + if (err) { + sk_msg_free(sk, msg); + tls_free_open_rec(sk); // might not be needed in noBPF + } + return err; + } more_data: enospc = sk_msg_full(msg); if (psock->eval == __SK_NONE) {
Powered by blists - more mailing lists