netdev - RE: [PATCH net-next v2] net/tls: Add support for async decryption of tls records

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DB7PR04MB42526D44504171F6245A6CF78B180@DB7PR04MB4252.eurprd04.prod.outlook.com>
Date:   Sat, 15 Sep 2018 12:11:24 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     John Fastabend <john.fastabend@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "borisp@...lanox.com" <borisp@...lanox.com>,
        "aviadye@...lanox.com" <aviadye@...lanox.com>,
        "davejwatson@...com" <davejwatson@...com>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: RE: [PATCH net-next v2] net/tls: Add support for async decryption of
 tls records



> -----Original Message-----
> From: John Fastabend <john.fastabend@...il.com>
> Sent: Saturday, September 15, 2018 1:10 AM
> To: Vakul Garg <vakul.garg@....com>; netdev@...r.kernel.org
> Cc: borisp@...lanox.com; aviadye@...lanox.com; davejwatson@...com;
> davem@...emloft.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 <vakul.garg@....com>
> > Reviewed-by: Dave Watson <davejwatson@...com>
> > ---
> 
> [...]
> 
> 
> > @@ -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