RE: [PATCH net-next] tls: Fix socket mem accounting error under async encryption
> -Original Message-
> From: David Miller
> Sent: Wednesday, September 26, 2018 9:10 AM
> To: Vakul Garg
> Cc: netdev@vger.kernel.org; bor...@mellanox.com;
> avia...@mellanox.com; davejwat...@fb.com; doro...@fb.com
> Subject: Re: [PATCH net-next] tls: Fix socket mem accounting error under
> async encryption
>
> From: Vakul Garg
> Date: Wed, 26 Sep 2018 01:54:25 +
>
> > I don't find this patch and one other ("tls: Fixed a memory leak
> > during socket close") in linux-net-next. Could you please kindly
> > check? Regards.
>
> After applying I didn't push out and instead I started a test build, closed my
> laptop, did a lot of other things and just came back to finish the build.
>
> It'll show up momentarily.
>
> Thanks for your patience.
Thanks for explaining the workflow.
BTW, I noticed following build failure.
It gets resolved after reverting d6ab93364734.
CC [M] drivers/net/phy/marvell.o
drivers/net/phy/marvell.c: In function 'm88e1121_config_aneg':
drivers/net/phy/marvell.c:468:25: error: 'autoneg' undeclared (first use in
this function); did you mean 'put_net'?
if (phydev->autoneg != autoneg || changed) {
^~~
put_net
drivers/net/phy/marvell.c:468:25: note: each undeclared identifier is reported
only once for each function it appears in
[PATCH net-next] tls: Fixed a memory leak during socket close
During socket close, if there is a open record with tx context, it needs
to be be freed apart from freeing up plaintext and encrypted scatter
lists. This patch frees up the open record if present in tx context.
Also tls_free_both_sg() has been renamed to tls_free_open_rec() to
indicate that the free record in tx context is being freed inside the
function.
Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg
---
net/tls/tls_sw.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bf03f32aa983..6f2dc1873556 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -310,7 +310,7 @@ static void free_sg(struct sock *sk, struct scatterlist *sg,
*sg_size = 0;
}
-static void tls_free_both_sg(struct sock *sk)
+static void tls_free_open_rec(struct sock *sk)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
@@ -327,6 +327,8 @@ static void tls_free_both_sg(struct sock *sk)
free_sg(sk, rec->sg_plaintext_data,
>sg_plaintext_num_elem,
>sg_plaintext_size);
+
+ kfree(rec);
}
int tls_tx_records(struct sock *sk, int flags)
@@ -1580,7 +1582,7 @@ void tls_sw_free_resources_tx(struct sock *sk)
}
crypto_free_aead(ctx->aead_send);
- tls_free_both_sg(sk);
+ tls_free_open_rec(sk);
kfree(ctx);
}
--
2.13.6
RE: [net-next PATCH] tls: async support causes out-of-bounds access in crypto APIs
> -Original Message-
> From: netdev-ow...@vger.kernel.org On
> Behalf Of John Fastabend
> Sent: Saturday, September 15, 2018 1:32 AM
> To: Vakul Garg ; davejwat...@fb.com
> Cc: doro...@fb.com; netdev@vger.kernel.org;
> alexei.starovoi...@gmail.com; dan...@iogearbox.net;
> da...@davemloft.net
> Subject: [net-next PATCH] tls: async support causes out-of-bounds access in
> crypto APIs
>
> When async support was added it needed to access the sk from the async
> callback to report errors up the stack. The patch tried to use space after the
> aead request struct by directly setting the reqsize field in aead_request.
> This
> is an internal field that should not be used outside the crypto APIs. It is
> used
> by the crypto code to define extra space for private structures used in the
> crypto context. Users of the API then use crypto_aead_reqsize() and add the
> returned amount of bytes to the end of the request memory allocation
> before posting the request to encrypt/decrypt APIs.
>
> So this breaks (with general protection fault and KASAN error, if
> enabled) because the request sent to decrypt is shorter than required causing
> the crypto API out-of-bounds errors. Also it seems unlikely the sk is even
> valid
> by the time it gets to the callback because of memset in crypto layer.
>
> Anyways, fix this by holding the sk in the skb->sk field when the callback is
> set
> up and because the skb is already passed through to the callback handler via
> void* we can access it in the handler. Then in the handler we need to be
> careful to NULL the pointer again before kfree_skb. I added comments on
> both the setup (in tls_do_decryption) and when we clear it from the crypto
> callback handler tls_decrypt_done(). After this selftests pass again and fixes
> KASAN errors/warnings.
>
> Fixes: 94524d8fc965 ("net/tls: Add support for async decryption of tls
> records")
> Signed-off-by: John Fastabend
> ---
> include/net/tls.h |4
> net/tls/tls_sw.c | 39 +++
> 2 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/include/net/tls.h b/include/net/tls.h index cd0a65b..8630d28
> 100644
> --- a/include/net/tls.h
> +++ b/include/net/tls.h
> @@ -128,10 +128,6 @@ struct tls_sw_context_rx {
> bool async_notify;
> };
>
> -struct decrypt_req_ctx {
> - struct sock *sk;
> -};
> -
> struct tls_record_info {
> struct list_head list;
> u32 end_seq;
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index be4f2e9..cef69b6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -122,25 +122,32 @@ static int skb_nsg(struct sk_buff *skb, int offset,
> int len) static void tls_decrypt_done(struct crypto_async_request *req, int
> err) {
> struct aead_request *aead_req = (struct aead_request *)req;
> - struct decrypt_req_ctx *req_ctx =
> - (struct decrypt_req_ctx *)(aead_req + 1);
> -
> struct scatterlist *sgout = aead_req->dst;
> -
> - struct tls_context *tls_ctx = tls_get_ctx(req_ctx->sk);
> - struct tls_sw_context_rx *ctx = tls_sw_ctx_rx(tls_ctx);
> - int pending = atomic_dec_return(>decrypt_pending);
> + struct tls_sw_context_rx *ctx;
> + struct tls_context *tls_ctx;
> struct scatterlist *sg;
> + struct sk_buff *skb;
> unsigned int pages;
> + int pending;
> +
> + skb = (struct sk_buff *)req->data;
> + tls_ctx = tls_get_ctx(skb->sk);
> + ctx = tls_sw_ctx_rx(tls_ctx);
> + pending = atomic_dec_return(>decrypt_pending);
>
> /* Propagate if there was an err */
> if (err) {
> ctx->async_wait.err = err;
> - tls_err_abort(req_ctx->sk, err);
> + tls_err_abort(skb->sk, err);
> }
>
> + /* After using skb->sk to propagate sk through crypto async callback
> + * we need to NULL it again.
> + */
> + skb->sk = NULL;
> +
> /* Release the skb, pages and memory allocated for crypto req */
> - kfree_skb(req->data);
> + kfree_skb(skb);
>
> /* Skip the first S/G entry as it points to AAD */
> for_each_sg(sg_next(sgout), sg, UINT_MAX, pages) { @@ -175,11
> +182,13 @@ static int tls_do_decryption(struct sock *sk,
> (u8 *)iv_recv);
>
> if (async) {
> - struct decrypt_req_ctx *req_ctx;
> -
> - req_ctx = (struct decrypt_req_ctx *)(aead_req + 1);
> - req_ctx->sk = sk;
> -
> + /* Using skb->sk to push sk through to crypto async callback
> +
RE: [PATCH net-next v2] net/tls: Add support for async decryption of tls records
> -Original Message-
> From: John Fastabend
> Sent: Saturday, September 15, 2018 1:10 AM
> To: Vakul Garg ; netdev@vger.kernel.org
> Cc: bor...@mellanox.com; avia...@mellanox.com; davejwat...@fb.com;
> da...@davemloft.net
> Subject: Re: [PATCH net-next v2] net/tls: Add support for async decryption of
> tls records
>
> On 08/29/2018 02:56 AM, Vakul Garg wrote:
> > When tls records are decrypted using asynchronous acclerators such as
> > NXP CAAM engine, the crypto apis return -EINPROGRESS. Presently, on
> > getting -EINPROGRESS, the tls record processing stops till the time
> > the crypto accelerator finishes off and returns the result. This
> > incurs a context switch and is not an efficient way of accessing the
> > crypto accelerators. Crypto accelerators work efficient when they are
> > queued with multiple crypto jobs without having to wait for the
> > previous ones to complete.
> >
> > The patch submits multiple crypto requests without having to wait for
> > for previous ones to complete. This has been implemented for records
> > which are decrypted in zero-copy mode. At the end of recvmsg(), we
> > wait for all the asynchronous decryption requests to complete.
> >
> > The references to records which have been sent for async decryption
> > are dropped. For cases where record decryption is not possible in
> > zero-copy mode, asynchronous decryption is not used and we wait for
> > decryption crypto api to complete.
> >
> > For crypto requests executing in async fashion, the memory for
> > aead_request, sglists and skb etc is freed from the decryption
> > completion handler. The decryption completion handler wakesup the
> > sleeping user context when recvmsg() flags that it has done sending
> > all the decryption requests and there are no more decryption requests
> > pending to be completed.
> >
> > Signed-off-by: Vakul Garg
> > Reviewed-by: Dave Watson
> > ---
>
> [...]
>
>
> > @@ -1271,6 +1377,8 @@ int tls_set_sw_offload(struct sock *sk, struct
> tls_context *ctx, int tx)
> > goto free_aead;
> >
> > if (sw_ctx_rx) {
> > + (*aead)->reqsize = sizeof(struct decrypt_req_ctx);
> > +
>
> This is not valid and may cause GPF or best case only a KASAN warning.
> 'reqsize' should probably not be mangled outside the internal crypto APIs but
> the real reason is the reqsize is used to determine how much space is needed
> at the end of the aead_request for crypto private ctx use in encrypt/decrypt.
> After this patch when we submit an aead_request the crypto layer will think it
> has room for its private structs at the end but now only 8B will be there and
> crypto layer will happily memset some arbitrary memory for you amongst
> other things.
>
> Anyways testing a fix now will post shortly.
I am aware that the space in question after aead_request is used internally
by cryptoapi. My plan was to inflate this space by an extra sizeof(struct
decrypt_req_ctx).
(*aead)->reqsize += sizeof(struct decrypt_req_ctx);
Somehow, I missed it.
But your piece of change look a better solution.
Thanks for submitting the fix.
>
> Thanks,
> John
