Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Cyrille Pitchen]

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

[Cyrille Pitchen]

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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Stephan Müller]

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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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

[Stephan Müller]

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

[Stephan Müller]

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

[Herbert Xu]

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 Xu 
Home 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