Skip to contentSkip to conversation

tls: don't use stack memory in a scatterlist

PATCH In review Matt Mullins 2 participants 4 messages May 16, 2018 – May 17, 2018

Subsystems (3)
Networking Network drivers Networking TLS
lore
Share & tools
lore

Matt Mullins

author
#0 lore raw

PATCH 1/1 tls: don't use stack memory in a scatterlist

scatterlist code expects virt_to_page() to work, which fails with CONFIG_VMAP_STACK=y.

Fixes c46234ebb4d1e ("tls: RX path for ktls")
Signed-off-by Matt Mullins <[email protected]>
---
 include/net/tls.h | 3 +++
 net/tls/tls_sw.c  | 9 ++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)
include/net/tls.h +3 -0
index b400d0bb7448..f5fb16da3860 100644
@@ -97,6 +97,9 @@ struct tls_sw_context {
 	u8 control;
 	bool decrypted;
 
+	char rx_aad_ciphertext[TLS_AAD_SPACE_SIZE];
+	char rx_aad_plaintext[TLS_AAD_SPACE_SIZE];
+
 	/* Sending context */
 	char aad_space[TLS_AAD_SPACE_SIZE];
net/tls/tls_sw.c +4 -5
index 71e79597f940..e1c93ce74e0f 100644
@@ -680,7 +680,6 @@ static int decrypt_skb(struct sock *sk, struct sk_buff *skb,
 	struct scatterlist *sgin = &sgin_arr[0];
 	struct strp_msg *rxm = strp_msg(skb);
 	int ret, nsg = ARRAY_SIZE(sgin_arr);
-	char aad_recv[TLS_AAD_SPACE_SIZE];
 	struct sk_buff *unused;
 
 	ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE,

@@ -698,13 +697,13 @@ static int decrypt_skb(struct sock *sk, struct sk_buff *skb,
 	}
 
 	sg_init_table(sgin, nsg);
-	sg_set_buf(&sgin[0], aad_recv, sizeof(aad_recv));
+	sg_set_buf(&sgin[0], ctx->rx_aad_ciphertext, TLS_AAD_SPACE_SIZE);
 
 	nsg = skb_to_sgvec(skb, &sgin[1],
 			   rxm->offset + tls_ctx->rx.prepend_size,
 			   rxm->full_len - tls_ctx->rx.prepend_size);
 
-	tls_make_aad(aad_recv,
+	tls_make_aad(ctx->rx_aad_ciphertext,
 		     rxm->full_len - tls_ctx->rx.overhead_size,
 		     tls_ctx->rx.rec_seq,
 		     tls_ctx->rx.rec_seq_size,

@@ -803,12 +802,12 @@ int tls_sw_recvmsg(struct sock *sk,
 			if (to_copy <= len && page_count < MAX_SKB_FRAGS &&
 			    likely(!(flags & MSG_PEEK)))  {
 				struct scatterlist sgin[MAX_SKB_FRAGS + 1];
-				char unused[21];
 				int pages = 0;
 
 				zc = true;
 				sg_init_table(sgin, MAX_SKB_FRAGS + 1);
-				sg_set_buf(&sgin[0], unused, 13);
+				sg_set_buf(&sgin[0], ctx->rx_aad_plaintext,
+					   TLS_AAD_SPACE_SIZE);
 
 				err = zerocopy_from_iter(sk, &msg->msg_iter,
 							 to_copy, &pages,
Signature · 1 line
2.14.1
maintainer
#1 lore raw

From: Matt Mullins <[email protected]>
Date: Wed, 16 May 2018 10:48:40 -0700

Quoted 5 lines

scatterlist code expects virt_to_page() to work, which fails with CONFIG_VMAP_STACK=y.

Fixes c46234ebb4d1e ("tls: RX path for ktls")
Signed-off-by Matt Mullins <[email protected]>

Applied and queued up for -stable, thanks.

I'm surprised this problem wasn't discovered sooner. How exactly did you discover it? Did you actually see it trigger or is this purely from code inspection?

author
#2 lore raw
Quoted 3 lines · David Miller

I'm surprised this problem wasn't discovered sooner. How exactly did you discover it? Did you actually see it trigger or is this purely from code inspection?

Honestly, I'm not sure how it got uncovered, but it was observed at runtime. Doron Roberts-Kedes hit a null pointer dereference so we turned on CONFIG_DEBUG_SG -- then it became a proper BUG_ON(!virt_addr_valid(buf)); in sg_set_buf.

maintainer
#3 lore raw

From: Matt Mullins <[email protected]>
Date: Thu, 17 May 2018 20:28:46 +0000

Quoted 9 lines · David Miller
Quoted 3 lines

I'm surprised this problem wasn't discovered sooner. How exactly did you discover it? Did you actually see it trigger or is this purely from code inspection?

Honestly, I'm not sure how it got uncovered, but it was observed at runtime. Doron Roberts-Kedes hit a null pointer dereference so we turned on CONFIG_DEBUG_SG -- then it became a proper BUG_ON(!virt_addr_valid(buf)); in sg_set_buf.

Fair enough, thanks for the info.