2018-05-16 17:50:31

by Matt Mullins

Subject: [PATCH net] 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(-)

diff --git a/include/net/tls.h b/include/net/tls.h
index b400d0bb7448..f5fb16da3860 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -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];

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 71e79597f940..e1c93ce74e0f 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -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,
--
2.14.1

2018-05-17 18:51:39

by David Miller

Subject: Re: [PATCH net] tls: don't use stack memory in a scatterlist

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

> 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?

2018-05-17 20:29:57

by Matt Mullins

Subject: Re: [PATCH net] tls: don't use stack memory in a scatterlist

On Thu, 2018-05-17 at 14:50 -0400, David Miller wrote:
> 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.

2018-05-17 20:43:24

by David Miller

Subject: Re: [PATCH net] tls: don't use stack memory in a scatterlist

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

> On Thu, 2018-05-17 at 14:50 -0400, David Miller wrote:
>> 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.