| 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
| ||
|
Message-ID: <20191031152444.773c183b@cakuba.netronome.com>
Date: Thu, 31 Oct 2019 15:24:44 -0700
From: Jakub Kicinski <jakub.kicinski@...ronome.com>
To: John Fastabend <john.fastabend@...il.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
oss-drivers@...ronome.com, borisp@...lanox.com,
aviadye@...lanox.com, daniel@...earbox.net,
syzbot+f8495bff23a879a6d0bd@...kaller.appspotmail.com,
syzbot+6f50c99e8f6194bf363f@...kaller.appspotmail.com,
Eric Biggers <ebiggers@...nel.org>,
herbert@...dor.apana.org.au, glider@...gle.com,
linux-crypto@...r.kernel.org
Subject: Re: [PATCH net] net/tls: fix sk_msg trim on fallback to copy mode
On Thu, 31 Oct 2019 15:05:53 -0700, John Fastabend wrote:
> Jakub Kicinski wrote:
> > sk_msg_trim() tries to only update curr pointer if it falls into
> > the trimmed region. The logic, however, does not take into the
> > account pointer wrapping that sk_msg_iter_var_prev() does.
> > This means that when the message was trimmed completely, the new
> > curr pointer would have the value of MAX_MSG_FRAGS - 1, which is
> > neither smaller than any other value, nor would it actually be
> > correct.
> >
> > Special case the trimming to 0 length a little bit.
> >
> > This bug caused the TLS code to not copy all of the message, if
> > zero copy filled in fewer sg entries than memcopy would need.
> >
> > Big thanks to Alexander Potapenko for the non-KMSAN reproducer.
> >
> > Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
> > Reported-by: syzbot+f8495bff23a879a6d0bd@...kaller.appspotmail.com
> > Reported-by: syzbot+6f50c99e8f6194bf363f@...kaller.appspotmail.com
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > ---
> > Daniel, John, does this look okay?
>
> Thanks for the second ping!
No problem, I was worried the patch got categorized as TLS and therefore
lower prio ;)
> > CC: Eric Biggers <ebiggers@...nel.org>
> > CC: herbert@...dor.apana.org.au
> > CC: glider@...gle.com
> > CC: linux-crypto@...r.kernel.org
> >
> > net/core/skmsg.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> > index cf390e0aa73d..c42c145216b1 100644
> > --- a/net/core/skmsg.c
> > +++ b/net/core/skmsg.c
> > @@ -276,7 +276,10 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
> > * However trimed data that has not yet been used in a copy op
> > * does not require an update.
> > */
> > - if (msg->sg.curr >= i) {
> > + if (!msg->sg.size) {
> > + msg->sg.curr = 0;
> > + msg->sg.copybreak = 0;
> > + } else if (msg->sg.curr >= i) {
> > msg->sg.curr = i;
> > msg->sg.copybreak = msg->sg.data[i].length;
> > }
> > --
>
>
> Its actually not sufficient. We can't directly do comparisons against curr
> like this. msg->sg is a ring buffer so we have to be careful for these
> types of comparisons.
>
> Examples hopefully help explian. Consider the case with a ring layout on
> entering sk_msg_trim,
>
> 0 1 2 N = MAX_MSG_FRAGS
> |_|_|_|...|_|_|_|...|_|_|_|_|....|_|_|
> ^ ^ ^
> curr end start
>
> Start trimming from end
>
> 0 1 2 N = MAX_MSG_FRAGS
> |X|X|X|...|X|X|_|...|_|_|i|X|....|X|X|
> ^ ^ ^
> curr end start
>
> We trim backwards through ring with sk_msg_iter_var_prev(). And its
> possible to end with the result of above where 'i' is greater than curr
> and greater than start leaving scatterlist elements so size != 0.
>
> i > curr && i > start && sg.size != 0
>
> but we wont catch it with this condition
>
> if (msg->sg.curr >= i)
>
> So we won't reset curr and copybreak so we have a potential issue now
> where curr is pointing at data that has been trimmed.
I see, that makes sense and explains some of the complexity!
Perhaps the simplest way to go would be to adjust the curr as we go
then? The comparison logic could get a little hairy. So like this:
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index cf390e0aa73d..c2b0f9cb589c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -261,25 +261,29 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
msg->sg.size = len;
while (msg->sg.data[i].length &&
trim >= msg->sg.data[i].length) {
+ bool move_curr = msg->sg.curr == i;
+
trim -= msg->sg.data[i].length;
sk_msg_free_elem(sk, msg, i, true);
sk_msg_iter_var_prev(i);
+ if (move_curr) {
+ msg->sg.curr = i;
+ msg->sg.copybreak = msg->sg.data[i].length;
+ }
if (!trim)
goto out;
}
msg->sg.data[i].length -= trim;
sk_mem_uncharge(sk, trim);
-out:
/* If we trim data before curr pointer update copybreak and current
* so that any future copy operations start at new copy location.
* However trimed data that has not yet been used in a copy op
* does not require an update.
*/
- if (msg->sg.curr >= i) {
- msg->sg.curr = i;
+ if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
msg->sg.copybreak = msg->sg.data[i].length;
- }
+out:
sk_msg_iter_var_next(i);
msg->sg.end = i;
}
> I'll put together a fix but the correct thing to do here is a proper
> ring greater than op which is not what we have there. Although, your patch
> is also really a good one to have because reseting curr = 0 and
> copybreak = 0 when possible keeps the ring from being fragmented which
> avoids chaining when we push scatterlists down to crypto layer. So for
> your patch,
>
> Acked-By: John Fastabend <john.fastabend@...il.com>
>
> If it should go to net or net-next I think is probably up for debate
>
> Nice catch!!! Can you send me the reproducer?
I was using the repro from the syzbot report:
https://syzkaller.appspot.com/bug?extid=6f50c99e8f6194bf363f
plus this hack from Alexander to avoid the need for KMSAN:
https://lkml.kernel.org/linux-crypto/CAG_fn=UGCoDk04tL2vB981JmXgo6+-RUPmrTa3dSsK5UbZaTjA@mail.gmail.com/
Powered by blists - more mailing lists