Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode
@ 2019-11-04 23:36 Jakub Kicinski
  2019-11-05 14:48 ` John Fastabend
  2019-11-06  2:08 ` David Miller
  0 siblings, 2 replies; 3+ messages in thread
From: Jakub Kicinski @ 2019-11-04 23:36 UTC (permalink / raw)
  To: davem
  Cc: netdev, oss-drivers, borisp, aviadye, john.fastabend, daniel,
	Jakub Kicinski, syzbot+f8495bff23a879a6d0bd,
	syzbot+6f50c99e8f6194bf363f, Eric Biggers, herbert, glider,
	linux-crypto

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 nor
(as John points out) the fact that msg->sg is a ring buffer.

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 and rework
the comparison between curr and end to take into account wrapping.

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.

v2:
 - take into account that msg->sg is a ring buffer (John).

Link: https://lore.kernel.org/netdev/20191030160542.30295-1-jakub.kicinski@netronome.com/ (v1)

Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
Reported-by: syzbot+f8495bff23a879a6d0bd@syzkaller.appspotmail.com
Reported-by: syzbot+6f50c99e8f6194bf363f@syzkaller.appspotmail.com
Co-developed-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
CC: Eric Biggers <ebiggers@kernel.org>
CC: herbert@gondor.apana.org.au
CC: glider@google.com
CC: linux-crypto@vger.kernel.org
---
 include/linux/skmsg.h |  9 ++++++---
 net/core/skmsg.c      | 20 +++++++++++++++-----
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index e4b3fb4bb77c..ce7055259877 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -139,6 +139,11 @@ static inline void sk_msg_apply_bytes(struct sk_psock *psock, u32 bytes)
 	}
 }
 
+static inline u32 sk_msg_iter_dist(u32 start, u32 end)
+{
+	return end >= start ? end - start : end + (MAX_MSG_FRAGS - start);
+}
+
 #define sk_msg_iter_var_prev(var)			\
 	do {						\
 		if (var == 0)				\
@@ -198,9 +203,7 @@ static inline u32 sk_msg_elem_used(const struct sk_msg *msg)
 	if (sk_msg_full(msg))
 		return MAX_MSG_FRAGS;
 
-	return msg->sg.end >= msg->sg.start ?
-		msg->sg.end - msg->sg.start :
-		msg->sg.end + (MAX_MSG_FRAGS - msg->sg.start);
+	return sk_msg_iter_dist(msg->sg.start, msg->sg.end);
 }
 
 static inline struct scatterlist *sk_msg_elem(struct sk_msg *msg, int which)
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index cf390e0aa73d..ad31e4e53d0a 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -270,18 +270,28 @@ void sk_msg_trim(struct sock *sk, struct sk_msg *msg, int len)
 
 	msg->sg.data[i].length -= trim;
 	sk_mem_uncharge(sk, trim);
+	/* Adjust copybreak if it falls into the trimmed part of last buf */
+	if (msg->sg.curr == i && msg->sg.copybreak > msg->sg.data[i].length)
+		msg->sg.copybreak = msg->sg.data[i].length;
 out:
-	/* If we trim data before curr pointer update copybreak and current
-	 * so that any future copy operations start at new copy location.
+	sk_msg_iter_var_next(i);
+	msg->sg.end = i;
+
+	/* If we trim data a full sg elem 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) {
+	if (!msg->sg.size) {
+		msg->sg.curr = msg->sg.start;
+		msg->sg.copybreak = 0;
+	} else if (sk_msg_iter_dist(msg->sg.start, msg->sg.curr) >=
+		   sk_msg_iter_dist(msg->sg.start, msg->sg.end)) {
+		sk_msg_iter_var_prev(i);
 		msg->sg.curr = i;
 		msg->sg.copybreak = msg->sg.data[i].length;
 	}
-	sk_msg_iter_var_next(i);
-	msg->sg.end = i;
 }
 EXPORT_SYMBOL_GPL(sk_msg_trim);
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode
  2019-11-04 23:36 [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode Jakub Kicinski
@ 2019-11-05 14:48 ` John Fastabend
  2019-11-06  2:08 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: John Fastabend @ 2019-11-05 14:48 UTC (permalink / raw)
  To: Jakub Kicinski, davem
  Cc: netdev, oss-drivers, borisp, aviadye, john.fastabend, daniel,
	Jakub Kicinski, syzbot+f8495bff23a879a6d0bd,
	syzbot+6f50c99e8f6194bf363f, Eric Biggers, herbert, glider,
	linux-crypto

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 nor
> (as John points out) the fact that msg->sg is a ring buffer.
> 
> 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 and rework
> the comparison between curr and end to take into account wrapping.
> 
> 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.
> 
> v2:
>  - take into account that msg->sg is a ring buffer (John).
> 
> Link: https://lore.kernel.org/netdev/20191030160542.30295-1-jakub.kicinski@netronome.com/ (v1)
> 
> Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
> Reported-by: syzbot+f8495bff23a879a6d0bd@syzkaller.appspotmail.com
> Reported-by: syzbot+6f50c99e8f6194bf363f@syzkaller.appspotmail.com
> Co-developed-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> CC: Eric Biggers <ebiggers@kernel.org>
> CC: herbert@gondor.apana.org.au
> CC: glider@google.com
> CC: linux-crypto@vger.kernel.org
> ---

I'll run it through our CI here when I get a chance but LGTM
thanks for sending this and tracking it down. Per process.rst
I guess we add Signed-off-by lines instead of acks from
co-developers. News to me.

Signed-off-by: John Fastabend <john.fastabend@gmail.com>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode
  2019-11-04 23:36 [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode Jakub Kicinski
  2019-11-05 14:48 ` John Fastabend
@ 2019-11-06  2:08 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2019-11-06  2:08 UTC (permalink / raw)
  To: jakub.kicinski
  Cc: netdev, oss-drivers, borisp, aviadye, john.fastabend, daniel,
	syzbot+f8495bff23a879a6d0bd, syzbot+6f50c99e8f6194bf363f,
	ebiggers, herbert, glider, linux-crypto

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon,  4 Nov 2019 15:36:57 -0800

> 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 nor
> (as John points out) the fact that msg->sg is a ring buffer.
> 
> 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 and rework
> the comparison between curr and end to take into account wrapping.
> 
> 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.
> 
> v2:
>  - take into account that msg->sg is a ring buffer (John).
> 
> Link: https://lore.kernel.org/netdev/20191030160542.30295-1-jakub.kicinski@netronome.com/ (v1)
> 
> Fixes: d829e9c4112b ("tls: convert to generic sk_msg interface")
> Reported-by: syzbot+f8495bff23a879a6d0bd@syzkaller.appspotmail.com
> Reported-by: syzbot+6f50c99e8f6194bf363f@syzkaller.appspotmail.com
> Co-developed-by: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Applied and queued up for -stable, thanks Jakub.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 23:36 [PATCH net v2] net/tls: fix sk_msg trim on fallback to copy mode Jakub Kicinski
2019-11-05 14:48 ` John Fastabend
2019-11-06  2:08 ` David Miller

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git