Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 20, 2017 at 06:07:04PM +0100, Cyrille Pitchen wrote: > Hi all, > > Le 13/01/2017 à 12:39, Herbert Xu a écrit : > > On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote: > >> > >> I thought I understood that you would not want to see it in any > >> implementation. But, ok, if you want to leave it. > > > > If you remove it from authenc then authenc will be broken. > > > > Hence if the copy of the associated data is needed in the crypto/authenc.c > driver, then I should also keep this copy in the drivers/crypto/atmel-aes.c > for authenc(hmac(shaX),cbc-aes) algorithms [1], shouldn't I? > > If so, should I keep the current not optimized implementation of > atmel_aes_authenc_copy_assoc() or try to use the code extracted by Stephan > from crypto/authenc.c using the null cipher as proposed in this thread? > > As said earlier in this thread, copying the associated data is not a so big > deal when compared to the main crypto processing. Please just ignore this for now. We have not decided to require the copying of the AAD in the kernel API. Thanks, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 20. Januar 2017, 18:07:04 CET schrieb Cyrille Pitchen: Hi Cyrille, > > I've taken Stephan's other comments into account from his review of the > atmel-authenc driver so I'm preparing a new series but I don't know what to > do for the associated data copy. > > Please let me know what you recommend, thanks! The question is where the null context shall be saved. As Herbert mentioned, he may not want to have it in crypto_aead. Herbert, as this implementation also requires the copy, shall we leave the null context in crypto_aead? If so, Cyrille can simply use patch 01 from this series and apply the change to his code similar to what I did in patches 02 - 13. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Hi all, Le 13/01/2017 à 12:39, Herbert Xu a écrit : > On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote: >> >> I thought I understood that you would not want to see it in any >> implementation. But, ok, if you want to leave it. > > If you remove it from authenc then authenc will be broken. > Hence if the copy of the associated data is needed in the crypto/authenc.c driver, then I should also keep this copy in the drivers/crypto/atmel-aes.c for authenc(hmac(shaX),cbc-aes) algorithms [1], shouldn't I? If so, should I keep the current not optimized implementation of atmel_aes_authenc_copy_assoc() or try to use the code extracted by Stephan from crypto/authenc.c using the null cipher as proposed in this thread? As said earlier in this thread, copying the associated data is not a so big deal when compared to the main crypto processing. For instance, with IPSec ESP with AES in CBC mode, the associated data layout should be: Security Parameters Index (SPI): 4 bytes Sequence Number: 4 bytes AES IV: 16 bytes So it's only a 24 byte copy. I've taken Stephan's other comments into account from his review of the atmel-authenc driver so I'm preparing a new series but I don't know what to do for the associated data copy. Please let me know what you recommend, thanks! Best regards, Cyrille [1] https://lkml.org/lkml/2016/12/22/306 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Hi Stephan, this series of patches sounds like a good idea. I haven't tested it with the Atmel AES hardware yet but I have many dummy questions: Looking at some driver patches in the series, it seems you only add a call to crypto_aead_copy_ad() but I don't see any removal of the previous driver specific implementation to perform that copy. I take the Atmel gcm(aes) driver case aside since looking at the current code, it seems that I've forgotten to copy that assoc data from req->src into req->dst scatter list. Then I assume that I was lucky so when I tested with the test manager or IPSec connections, I always fell in the case where req->src == req->dst. I thought the test manager also covered the case req->src != req->dst but I don't remember very well and I haven't checked recently, sorry! Then I now look at the default drivers/gcm.c driver and, if I understand this driver correctly, it doesn't copy the associated data from req->src into req->dst when req->src != req->dst... What does it mean? Did I misunderstand the gmc.c driver or is it a bug? Is that copy the associated data needed after all? Also looking at the drivers/authenc.c driver, this one does copy the associated data. So now I understand why I didn't make the copy for gcm(aes) but did it for authenc(hmac(shaX),cbc(aes)) in atmel-aes.c: when I add new crypto algorithms support in the Atmel drivers, I always take the crypto/*.c drivers as reference. So finally, what shall we do? copy or not copy? That is my question! One last question: why do we copy those associated data only for encrypting requests but not for decrypting requests? The associated data might still be needed in the req->dst scatter list even when it only refers to plain data so no other crypto operation is needed after. However, let's take the example of an IPSec connection with ESP: the first 8 bytes of the ESP header (4-byte SPI + 4-byte sequence number) are used as associated data. They must be authenticated but they cannot be ciphered as we need the plain SPI value to attach some IP packet to the relevant IPSec session hence knowing the crypto algorithms to be used to process the network packet. However, once the received IPSec packet has been decrypted and authenticated, the sequence number part the the ESP header might still be needed in req->dst if for some reason the req->src is no longer available when performing the anti-replay check. Maybe the issue simply never occurs because req->src is always == req->dst or maybe because the anti-replay check is always performed before the crypto stuff. I dunno! So why not copying the associated data also when processing decrypt requests too? Sorry for those newbie questions! I try to improve my understanding and knowledge of the crypto subsystem and its interaction with the network subsystem without digging too much in the source code :p Best regards, Cyrille Le 10/01/2017 à 02:36, Stephan Müller a écrit : > Hi, > > to all driver maintainers: the patches I added are compile tested, but > I do not have the hardware to verify the code. May I ask the respective > hardware maintainers to verify that the code is appropriate and works > as intended? Thanks a lot. > > Herbert, this is my proprosal for our discussion around copying the > AAD for algif_aead. Instead of adding the code to algif_aead and wait > until it transpires to all cipher implementations, I thought it would > be more helpful to fix all cipher implementations. > > To do so, the AAD copy function found in authenc is extracted to a global > service function. Furthermore, the generic AEAD TFM initialization code > now allocates the null cipher too. This allows us now to only invoke > the AAD copy function in the various implementations without any additional > allocation logic. > > The code for x86 and the generic code was tested with libkcapi. > > The code for the drivers is compile tested for drivers applicable to > x86 only. All others are neither compile tested nor functionally tested. > > Stephan Mueller (13): > crypto: service function to copy AAD from src to dst > crypto: gcm_generic - copy AAD during encryption > crypto: ccm_generic - copy AAD during encryption > crypto: rfc4106-gcm-aesni - copy AAD during encryption > crypto: ccm-aes-ce - copy AAD during encryption > crypto: talitos - copy AAD during encryption > crypto: picoxcell - copy AAD during encryption > crypto: ixp4xx - copy AAD during encryption > crypto: atmel - copy AAD during encryption > crypto: caam - copy AAD during encryption > crypto: chelsio - copy AAD during encryption > crypto: nx - copy AAD during encryption > crypto: qat - copy AAD during encryption > > arch/arm64/crypto/aes-ce-ccm-glue.c | 4 > arch/x86/crypto/aesni-intel_glue.c | 5 + > crypto/Kconfig | 4 ++-- > crypto/aead.c| 36 > ++-- > crypto/authenc.c | 36 >
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 19:33:24 CET schrieb Herbert Xu: Hi Herbert, > On Fri, Jan 13, 2017 at 12:30:15PM +0100, Stephan Müller wrote: > > So, the patch set you want to see is: > > > > - remove the AAD copy operation from authenc and not add it to any AEAD > > implementations > > Why would you remove it from authenc? I thought I understood that you would not want to see it in any implementation. But, ok, if you want to leave it. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote: > > I thought I understood that you would not want to see it in any > implementation. But, ok, if you want to leave it. If you remove it from authenc then authenc will be broken. -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 13, 2017 at 12:30:15PM +0100, Stephan Müller wrote: > > So, the patch set you want to see is: > > - remove the AAD copy operation from authenc and not add it to any AEAD > implementations Why would you remove it from authenc? > - add the AAD copy operation to algif_aead No just copy everything and then do in-place crypto. > Shall the null cipher context we need for that be anchored in the crypto_aead > data structure as done in patch 01 of this series (this would allow an easy > addition to the copy call to every AEAD implementation if deemed needed) or > shall it be anchored within algif_aead? It should be in algif_aead. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 19:26:23 CET schrieb Herbert Xu: Hi Herbert, > On Fri, Jan 13, 2017 at 12:19:48PM +0100, Stephan Müller wrote: > > That is correct, but I thought that playing with pointers is always faster > > than doing memcpy. Are you saying that this assumption is not true when we > > somehow have the code to try to perform an in-place operation? > > If you're doing crypto the overhead in copying the AAD is the > last thing you need to worry about. So, the patch set you want to see is: - remove the AAD copy operation from authenc and not add it to any AEAD implementations - add the AAD copy operation to algif_aead Shall the null cipher context we need for that be anchored in the crypto_aead data structure as done in patch 01 of this series (this would allow an easy addition to the copy call to every AEAD implementation if deemed needed) or shall it be anchored within algif_aead? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 13, 2017 at 11:58:24AM +0100, Stephan Müller wrote: > > May I ask how the in-place code path can be invoked by algif_aead or > algif_skcipher? As far as I understand, this code path is only invoked when > the cipher implementation sees that the src and dst SGLs are identical. It's not right now but it isn't difficult to add the code to allow it by comparing SGLs. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 13, 2017 at 12:19:48PM +0100, Stephan Müller wrote: > > That is correct, but I thought that playing with pointers is always faster > than doing memcpy. Are you saying that this assumption is not true when we > somehow have the code to try to perform an in-place operation? If you're doing crypto the overhead in copying the AAD is the last thing you need to worry about. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 19:14:06 CET schrieb Herbert Xu: Hi Herbert, > On Fri, Jan 13, 2017 at 12:12:39PM +0100, Stephan Müller wrote: > > Adding such code should IMHO not be impaired by pointing to the AAD held > > in > > the src SGL by the dst SGL as offered with the older patch mentioned > > before. > The point is you're turning what could otherwise be a linear SGL > into a non-linear one by forcing the same AAD on both sides. That is correct, but I thought that playing with pointers is always faster than doing memcpy. Are you saying that this assumption is not true when we somehow have the code to try to perform an in-place operation? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Fri, Jan 13, 2017 at 12:12:39PM +0100, Stephan Müller wrote: > > Adding such code should IMHO not be impaired by pointing to the AAD held in > the src SGL by the dst SGL as offered with the older patch mentioned before. The point is you're turning what could otherwise be a linear SGL into a non-linear one by forcing the same AAD on both sides. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 19:04:17 CET schrieb Herbert Xu: Hi Herbert, > On Fri, Jan 13, 2017 at 11:58:24AM +0100, Stephan Müller wrote: > > May I ask how the in-place code path can be invoked by algif_aead or > > algif_skcipher? As far as I understand, this code path is only invoked > > when > > the cipher implementation sees that the src and dst SGLs are identical. > > It's not right now but it isn't difficult to add the code to allow > it by comparing SGLs. Adding such code should IMHO not be impaired by pointing to the AAD held in the src SGL by the dst SGL as offered with the older patch mentioned before. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 18:23:39 CET schrieb Herbert Xu: Hi Herbert, > On Thu, Jan 12, 2017 at 05:37:33PM +0100, Stephan Müller wrote: > > I would not understand that statement. > > > > With the patch mentioned above that I provided some weeks ago, we have the > > following scenario for an encryption (in case of decryption, it is almost > > identical, just the tag location is reversed): > > > > user calls sendmsg with data buffer/IOVEC: AAD || PT > > > > -> algif_aead turns this into the src SGL > > > > user calls recvmsg with data buffer/IOVEC: CT || Tag > > > > -> algif_aead creates the first SG entry in the dst SGL pointing to the > > > > AAD from the src SGL > > > > -> algif_aead appends the user buffers to the dst SGL > > > > -> algif_aead performs its operation and during that operation, only the > > > > CT and Tag parts are changed > > > > I.e. with the pre-pending of the SG pointing to the AAD from the src SGL > > to > > the dst SGL we have a clean invocation of the kernel API. > > But that means you can never invoke the in-place path of the kernel > API, which is the most optimised code path. May I ask how the in-place code path can be invoked by algif_aead or algif_skcipher? As far as I understand, this code path is only invoked when the cipher implementation sees that the src and dst SGLs are identical. However, both algif interfaces maintain separate src and dst SGLs and always invoke the cipher operation with these dissimilar SGLs. Thus, I would infer that even when the user invokes zerocopy, the src/dst SGLs are not identical and therefore the cipher implementations would not use the in-place code path. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Thu, Jan 12, 2017 at 05:37:33PM +0100, Stephan Müller wrote: > > I would not understand that statement. > > With the patch mentioned above that I provided some weeks ago, we have the > following scenario for an encryption (in case of decryption, it is almost > identical, just the tag location is reversed): > > user calls sendmsg with data buffer/IOVEC: AAD || PT > -> algif_aead turns this into the src SGL > > user calls recvmsg with data buffer/IOVEC: CT || Tag > -> algif_aead creates the first SG entry in the dst SGL pointing to the > AAD from the src SGL > -> algif_aead appends the user buffers to the dst SGL > > -> algif_aead performs its operation and during that operation, only > the > CT and Tag parts are changed > > I.e. with the pre-pending of the SG pointing to the AAD from the src SGL to > the dst SGL we have a clean invocation of the kernel API. But that means you can never invoke the in-place path of the kernel API, which is the most optimised code path. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Freitag, 13. Januar 2017, 00:06:29 CET schrieb Herbert Xu: Hi Herbert, > On Thu, Jan 12, 2017 at 05:01:13PM +0100, Stephan Müller wrote: > > I fully agree. Therefore, I was under the impression that disregarding the > > AAD in recvmsg entirely would be most appropriate as offered with the > > patch "crypto: AF_ALG - disregard AAD buffer space for output". In this > > case we would be fully POSIX compliant, the kernel would not copy the AAD > > (and thus perform multiple memcpy operations due to copy_from_user and > > copy_to_user round trips) and leave the AAD copy operation entirely to > > user space. > Yes but then you'd have to play nasty games to fit this through > the kernel API. I would not understand that statement. With the patch mentioned above that I provided some weeks ago, we have the following scenario for an encryption (in case of decryption, it is almost identical, just the tag location is reversed): user calls sendmsg with data buffer/IOVEC: AAD || PT -> algif_aead turns this into the src SGL user calls recvmsg with data buffer/IOVEC: CT || Tag -> algif_aead creates the first SG entry in the dst SGL pointing to the AAD from the src SGL -> algif_aead appends the user buffers to the dst SGL -> algif_aead performs its operation and during that operation, only the CT and Tag parts are changed I.e. with the pre-pending of the SG pointing to the AAD from the src SGL to the dst SGL we have a clean invocation of the kernel API. Yet, we avoid copy round trips of the AAD from src to dst in the kernel. > Besides, we could still do in-place crypto even > though you suggested that it's complicated. Is that crypto-in-place operation really a goal we want to achieve if we "buy" it with a memcpy of the data from the src SGL to the dst SGL before the crypto operation? Can we really call such implementation a crypto-in-place operation? > It's not really. I concur, for encryption the suggested memcpy is not difficult. Only for decryption, the memcpy is more challenging. > All we have to do is walk through the SG list and compare each > page/offset. For the common case it's going to be a single-entry > list. > > Cheers, Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Thu, Jan 12, 2017 at 04:50:46PM +0100, Stephan Müller wrote: > > So you say that we could remove it from authenc() entirely (this is currently > the only location where such copy operation is found for the encryption > direction)? > > I would concur that the kernel does not need that. authenc needs it for performance reasons. The kernel API as it stands basically says that it may or may not copy the AAD. If you choose to have a distinct AAD in the dst SG list then either you throw away the result or you copy it yourself. > If we only want to solve that for algif_aead, wouldn't it make more sense if > the user space caller takes care of that (such as libkcapi)? By tinkering > with > the SGLs and copying the data to the dst buffer before the cipher operation > takes place, I guess we will add performance degradation and more complexity > in the kernel. > > Having such logic in user space would keep the algif_aead cleaner IMHO. We need to have a sane kernel API that respects POSIX. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Thu, Jan 12, 2017 at 05:01:13PM +0100, Stephan Müller wrote: > > I fully agree. Therefore, I was under the impression that disregarding the > AAD > in recvmsg entirely would be most appropriate as offered with the patch > "crypto: AF_ALG - disregard AAD buffer space for output". In this case we > would be fully POSIX compliant, the kernel would not copy the AAD (and thus > perform multiple memcpy operations due to copy_from_user and copy_to_user > round trips) and leave the AAD copy operation entirely to user space. Yes but then you'd have to play nasty games to fit this through the kernel API. Besides, we could still do in-place crypto even though you suggested that it's complicated. It's not really. All we have to do is walk through the SG list and compare each page/offset. For the common case it's going to be a single-entry list. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 23:53:44 CET schrieb Herbert Xu: Hi Herbert, > > > If we only want to solve that for algif_aead, wouldn't it make more sense > > if the user space caller takes care of that (such as libkcapi)? By > > tinkering with the SGLs and copying the data to the dst buffer before the > > cipher operation takes place, I guess we will add performance degradation > > and more complexity in the kernel. > > > > Having such logic in user space would keep the algif_aead cleaner IMHO. > > We need to have a sane kernel API that respects POSIX. I fully agree. Therefore, I was under the impression that disregarding the AAD in recvmsg entirely would be most appropriate as offered with the patch "crypto: AF_ALG - disregard AAD buffer space for output". In this case we would be fully POSIX compliant, the kernel would not copy the AAD (and thus perform multiple memcpy operations due to copy_from_user and copy_to_user round trips) and leave the AAD copy operation entirely to user space. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 23:39:24 CET schrieb Herbert Xu: Hi Herbert, > On Thu, Jan 12, 2017 at 04:34:39PM +0100, Stephan Müller wrote: > > We would only be able to remove it if all AEAD implementations are > > converted. But for the conversion time, we do face that issue. > > It doesn't matter. Nobody in the kernel uses that. In fact I > wonder whether we should even do it for the kernel API. We only > need it for the user-space API because it goes through read/write. So you say that we could remove it from authenc() entirely (this is currently the only location where such copy operation is found for the encryption direction)? I would concur that the kernel does not need that. > > > Are you suggesting that the entire data in the src SGL is first copied to > > the dst SGL by algif_aead? If yes, that still requires significant > > src/dst SGL tinkering as we have the tag -- the src SGL for encrypt does > > not have the tag space where the dst SGL for encrypt is required to have > > the tag size. This is vice versa for the decryption operation. > > It's really only a problem for decryption. In that case you can > extend the dst SG list to include the tag. If we only want to solve that for algif_aead, wouldn't it make more sense if the user space caller takes care of that (such as libkcapi)? By tinkering with the SGLs and copying the data to the dst buffer before the cipher operation takes place, I guess we will add performance degradation and more complexity in the kernel. Having such logic in user space would keep the algif_aead cleaner IMHO. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 23:27:10 CET schrieb Herbert Xu: Hi Herbert, > On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote: > > As far as I understand, we have the following AAD copy operations that we > > discuss: > > > > - algif_aead: you suggested to add the AAD copy operation here > > > > - AEAD implementations: over time, the AEAD implementations shall receive > > the AAD copy operation. The AAD copy operation shall only take place if > > the src SGL != dst SGL > > If and when that happens we'd simply need to remove the copy from > algif_aead code. We would only be able to remove it if all AEAD implementations are converted. But for the conversion time, we do face that issue. > But even if we didn't you wouldn't have two copies > because algif_aead should invoke the in-place code-path after doing > a full copy of src to dst. Are you suggesting that the entire data in the src SGL is first copied to the dst SGL by algif_aead? If yes, that still requires significant src/dst SGL tinkering as we have the tag -- the src SGL for encrypt does not have the tag space where the dst SGL for encrypt is required to have the tag size. This is vice versa for the decryption operation. And to me the most elegant solution seems adding the copy operation to crypto_aead_[en|de]crypt. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote: > > As far as I understand, we have the following AAD copy operations that we > discuss: > > - algif_aead: you suggested to add the AAD copy operation here > > - AEAD implementations: over time, the AEAD implementations shall receive the > AAD copy operation. The AAD copy operation shall only take place if the src > SGL != dst SGL If and when that happens we'd simply need to remove the copy from algif_aead code. But even if we didn't you wouldn't have two copies because algif_aead should invoke the in-place code-path after doing a full copy of src to dst. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 22:57:28 CET schrieb Herbert Xu: Hi Herbert, > On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote: > > When addressing the issue in the algif_aead code, and expect that over > > time > > the AEAD implementations will gain the copy operation, eventually we will > > copy the AAD twice. Of course, this could be prevented, if the algif_aead > > code somehow uses the same SGL for the src and dst AAD. > > Why would you copy it twice? You copy everything before you start > and then just do in-place crypto. > As far as I understand, we have the following AAD copy operations that we discuss: - algif_aead: you suggested to add the AAD copy operation here - AEAD implementations: over time, the AEAD implementations shall receive the AAD copy operation. The AAD copy operation shall only take place if the src SGL != dst SGL The algif_aead code *always* will have the src SGL being different from the dst SGL. Thus, any existing AEAD implementation copy will always be performed which would be in addition to the algif_aead AAD copy operation. We can only avoid the AEAD implementation copy operation, if we add some code to algif_aead to make sure that the AAD data is pointed to by src/dst SGL that is identical. However, such logic to make src/dst SGL identical is quite complex compared to simply use one callback as suggested in the current patch set. In the followup email, I suggested to add the AAD copy function invocation into crypto_aead_encrypt. This way, we can drop the numerous patches to the AEAD implementations and yet we can avoid adding such copy operation and src/ dst SGL unification logic to algif_aead. > > > BTW, why are you only doing the copy for encryption? > > > > I was looking at the only AEAD implementation that does the copy > > operation: > > authenc. There, the copy operation is only performed for encryption. I was > > thinking a bit about why decryption was not covered. I think the answer is > > the following: for encryption, the AAD is definitely needed in the dst > > buffer as the dst buffer with the AAD must be sent to the recipient for > > decryption. The decryption and the associated authentication only works > > with the AAD. However, after decrypting, all the caller wants is the > > decrypted plaintext only. There is no further use of the AAD after the > > decryption step. Hence, copying the AAD to the dst buffer in the > > decryption step would not serve the caller. > That's just the current implementation. If we're going to make > this an API then we should do it for both directions. Considering the suggestion above to add the AAD copy call to crypto_aead_encrypt, we can add such copy call also to crypto_aead_decrypt. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote: > > When addressing the issue in the algif_aead code, and expect that over time > the AEAD implementations will gain the copy operation, eventually we will > copy > the AAD twice. Of course, this could be prevented, if the algif_aead code > somehow uses the same SGL for the src and dst AAD. Why would you copy it twice? You copy everything before you start and then just do in-place crypto. > > BTW, why are you only doing the copy for encryption? > > I was looking at the only AEAD implementation that does the copy operation: > authenc. There, the copy operation is only performed for encryption. I was > thinking a bit about why decryption was not covered. I think the answer is > the > following: for encryption, the AAD is definitely needed in the dst buffer as > the dst buffer with the AAD must be sent to the recipient for decryption. The > decryption and the associated authentication only works with the AAD. > However, > after decrypting, all the caller wants is the decrypted plaintext only. There > is no further use of the AAD after the decryption step. Hence, copying the > AAD > to the dst buffer in the decryption step would not serve the caller. That's just the current implementation. If we're going to make this an API then we should do it for both directions. Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 14:19:36 CET schrieb Herbert Xu: Hi Herbert, > > I think it's too much churn. Let's get the algif_aead code fixed > up first, and then pursue this later. To eliminate the extra churn of having all AEAD implementations changed to invoke copy operation, what about adding the callback to crypto_aead_encrypt? This way, all AEAD implementations benefit from it without having an extra call added to each of them? Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Am Donnerstag, 12. Januar 2017, 14:19:36 CET schrieb Herbert Xu: Hi Herbert, > On Tue, Jan 10, 2017 at 02:36:21AM +0100, Stephan Müller wrote: > > to all driver maintainers: the patches I added are compile tested, but > > I do not have the hardware to verify the code. May I ask the respective > > hardware maintainers to verify that the code is appropriate and works > > as intended? Thanks a lot. > > > > Herbert, this is my proprosal for our discussion around copying the > > AAD for algif_aead. Instead of adding the code to algif_aead and wait > > until it transpires to all cipher implementations, I thought it would > > be more helpful to fix all cipher implementations. > > I think it's too much churn. Let's get the algif_aead code fixed > up first, and then pursue this later. My idea with this patch set was to have only a minimal change to any AEAD implementation, i.e. one callback to address this issue. When addressing the issue in the algif_aead code, and expect that over time the AEAD implementations will gain the copy operation, eventually we will copy the AAD twice. Of course, this could be prevented, if the algif_aead code somehow uses the same SGL for the src and dst AAD. Some time back I published the patch "crypto: AF_ALG - disregard AAD buffer space for output". This patch tried changing the src and dst SGL to remove the AAD. Considering this patch trying to change the src and dst SGL structure, I expect that the patch to algif_aead to have a common src/dst SGL for the AAD to prevent the AAD copy from the AEAD implementations is similar in complexity. Weighing the complexity of such temporary band-aid for algif_aead with the addition of one callback to each AEAD implementation (which would need to be added some when anyway), I thought it is less complex to add the one callback to the AEAD implementations. > > BTW, why are you only doing the copy for encryption? I was looking at the only AEAD implementation that does the copy operation: authenc. There, the copy operation is only performed for encryption. I was thinking a bit about why decryption was not covered. I think the answer is the following: for encryption, the AAD is definitely needed in the dst buffer as the dst buffer with the AAD must be sent to the recipient for decryption. The decryption and the associated authentication only works with the AAD. However, after decrypting, all the caller wants is the decrypted plaintext only. There is no further use of the AAD after the decryption step. Hence, copying the AAD to the dst buffer in the decryption step would not serve the caller. Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
On Tue, Jan 10, 2017 at 02:36:21AM +0100, Stephan Müller wrote: > > to all driver maintainers: the patches I added are compile tested, but > I do not have the hardware to verify the code. May I ask the respective > hardware maintainers to verify that the code is appropriate and works > as intended? Thanks a lot. > > Herbert, this is my proprosal for our discussion around copying the > AAD for algif_aead. Instead of adding the code to algif_aead and wait > until it transpires to all cipher implementations, I thought it would > be more helpful to fix all cipher implementations. I think it's too much churn. Let's get the algif_aead code fixed up first, and then pursue this later. BTW, why are you only doing the copy for encryption? Cheers, -- Email: Herbert XuHome Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers
Hi, to all driver maintainers: the patches I added are compile tested, but I do not have the hardware to verify the code. May I ask the respective hardware maintainers to verify that the code is appropriate and works as intended? Thanks a lot. Herbert, this is my proprosal for our discussion around copying the AAD for algif_aead. Instead of adding the code to algif_aead and wait until it transpires to all cipher implementations, I thought it would be more helpful to fix all cipher implementations. To do so, the AAD copy function found in authenc is extracted to a global service function. Furthermore, the generic AEAD TFM initialization code now allocates the null cipher too. This allows us now to only invoke the AAD copy function in the various implementations without any additional allocation logic. The code for x86 and the generic code was tested with libkcapi. The code for the drivers is compile tested for drivers applicable to x86 only. All others are neither compile tested nor functionally tested. Stephan Mueller (13): crypto: service function to copy AAD from src to dst crypto: gcm_generic - copy AAD during encryption crypto: ccm_generic - copy AAD during encryption crypto: rfc4106-gcm-aesni - copy AAD during encryption crypto: ccm-aes-ce - copy AAD during encryption crypto: talitos - copy AAD during encryption crypto: picoxcell - copy AAD during encryption crypto: ixp4xx - copy AAD during encryption crypto: atmel - copy AAD during encryption crypto: caam - copy AAD during encryption crypto: chelsio - copy AAD during encryption crypto: nx - copy AAD during encryption crypto: qat - copy AAD during encryption arch/arm64/crypto/aes-ce-ccm-glue.c | 4 arch/x86/crypto/aesni-intel_glue.c | 5 + crypto/Kconfig | 4 ++-- crypto/aead.c| 36 ++-- crypto/authenc.c | 36 crypto/ccm.c | 10 + crypto/gcm.c | 17 +++ drivers/crypto/atmel-aes.c | 6 ++ drivers/crypto/caam/caamalg.c| 8 +++ drivers/crypto/chelsio/chcr_algo.c | 5 + drivers/crypto/ixp4xx_crypto.c | 6 ++ drivers/crypto/nx/nx-aes-ccm.c | 4 drivers/crypto/nx/nx-aes-gcm.c | 10 + drivers/crypto/picoxcell_crypto.c| 5 + drivers/crypto/qat/qat_common/qat_algs.c | 4 drivers/crypto/talitos.c | 5 + include/crypto/aead.h| 2 ++ include/crypto/internal/aead.h | 12 +++ 18 files changed, 143 insertions(+), 36 deletions(-) -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html