Re: [RFC PATCH v2] crypto: Add IV generation algorithms

[Ondrej Mosnáček]

Hi Binoy,

2016-12-13 9:49 GMT+01:00 Binoy Jayan :
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains the in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. The iv generation algorithms implemented in geniv.c
> include plain, plain64, essiv, benbi, null, lmk and tcw.

I like what you are trying to achieve, however I don't think the
solution you are heading towards (passing sector number to a special
crypto template) would be the best approach here. Milan is currently
trying to add authenticated encryption support to dm-crypt (see [1])
and as part of this change, a new random IV mode would be introduced.
This mode generates a random IV for each sector write, includes it in
the authenticated data and stores it in the sector's metadata (in a
separate part of the disk). In this case dm-crypt will need to have
control over the IV generation (or at least be able to somehow
retrieve it after the crypto operation).

That said, I believe a different approach would be preferable here. I
would suggest, instead of moving the IV generation to the crypto
layer, to add a new type of request to skcipher API (let's call it
'skcipher_bulk_request'), which could be used to submit several
messages at once (together in a single sg list), each with their own
IV, to a skcipher. This would allow drivers to optimize handling of
such requests (e.g. the SIMD ciphers could call kernel_fpu_begin/end
just once for the whole request). It could be done in such a way, that
implementing this type of requests would be optional and a fallback
implementation, which would just split the request into regular
skcipher_requests, would be automatically set for the ciphers that do
not set it themselves. That way this would require no changes to
crypto drivers in the beginning and optimizations could be added
incrementally.

The advantage of this approach to handling such "bulk" requests is
that crypto drivers could just optimize regular algorithms (xts(aes),
cbc(aes), etc.) and wouldn't need to mess with dm-crypt-specific IV
generation. This also means that other users that could potentially
benefit from bulking requests (perhaps network stack?) could use the
same functionality.

I have been playing with this idea for some time now and I should have
an RFC patchset ready soon...

Binoy, Herbert, what do you think about such approach?

[1] https://www.redhat.com/archives/dm-devel/2017-January/msg00028.html

> When using multiple keys with the original dm-crypt, the key selection is
> made based on the sector number as:
>
> key_index = sector & (key_count - 1)
>
> This restricts the usage of the same key for encrypting/decrypting a
> single bio. One way to solve this is to move the key management code from
> dm-crypt to cryto layer. But this seems tricky when using template ciphers
> because, when multiple ciphers are instantiated from dm layer, each cipher
> instance set with a unique subkey (part of the bigger master key) and
> these instances themselves do not have access to each other's instances
> or contexts. This way, a single instance cannot encryt/decrypt a whole bio.
> This has to be fixed.

Please note that the "keycount" parameter was added to dm-crypt solely
for the purpose of implementing the loop-AES partition format. In
general, the security benefit gained by using keycount > 1 is
debatable, so it does not really make sense to use it for anything
else than accessing legacy loopAES partitions. Since Milan decided to
add it as a generic parameter, instead of hard-coding the
functionality for the LMK mode, it can be technically used also in
other combinations, but IMHO it is perfectly reasonable to just give
up on optimizing the cases when keycount > 1. I believe the loop-AES
partition support is just not that important :)

Thanks,
Ondrej
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Herbert,

On 2 January 2017 at 12:23, Herbert Xu  wrote:
> On Mon, Jan 02, 2017 at 12:16:45PM +0530, Binoy Jayan wrote:
>
> Right.  The actual number of underlying tfms that do the work
> won't change compared to the status quo.  We're just structuring
> it such that if the overall scheme is supported by the hardware
> then we can feed more than one sector at a time to it.

I was thinking of continuing to have the iv generation algorithms as template
ciphers instead of regular 'skcipher' as it is easier to inherit the parameters
from the underlying cipher (e.g. aes) like cra_blocksize, cra_alignmask,
ivsize, chunksize etc.

Usually, the underlying cipher for the template ciphers are instantiated
in the following function:

skcipher_instance:skcipher_alg:init()

Since the number of such cipher instances depend on the key count, which is
not known at the time of creation of the cipher (it's passed to as an argument
to the setkey api), the creation of those have to be delayed until the setkey
operation of the template cipher. But as Mark pointed out, the users of this
cipher may get confused if the creation of the underlying cipher fails while
trying to do a 'setkey' on the template cipher. I was wondering if I can create
a single instance of the cipher and assign it to tfms[0] and allocate the
remaining instances when the setkey operation is called later with the encoded
key_count so that errors during cipher creation are uncovered earlier.

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Gilad,

On 3 January 2017 at 19:53, Gilad Ben-Yossef  wrote:
> Good idea. I wanted to test the patch but alas it does not apply cleanly.
> You seem to have a blank line at the end of files and other small
> transgressions that makes checkpatch grumpy.

I think that is because there were some key structure changes in dm-crypt
after I sent out v2. I have resolved them while working on v3. Please wait for
the next version of the patchset. I'll send it probably by next week.
I wanted to incorporate a few changes suggested by Herbert before sending them.

> What is Not-signed-off-by ? :-)

It was just an RFC patch, not ready for merging.

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Gilad Ben-Yossef]

Hi Binoy,

On Tue, Dec 13, 2016 at 02:19:09PM +0530, Binoy Jayan wrote:
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains the in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. The iv generation algorithms implemented in geniv.c
> include plain, plain64, essiv, benbi, null, lmk and tcw.
>

Good idea. I wanted to test the patch but alas it does not apply cleanly.
You seem to have a blank line at the end of files and other small
transgressions that makes checkpatch grumpy.



Also...

>
> Not-signed-off-by: Binoy Jayan 


What is Not-signed-off-by ? :-)

Thanks,
Gilad Ben-Yossef
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

On 2 January 2017 at 12:23, Herbert Xu  wrote:
> On Mon, Jan 02, 2017 at 12:16:45PM +0530, Binoy Jayan wrote:
>>
>> Even if ciphers are allocated this way, all the encryption requests
>> for cbc should still go through IV generators? So that should mean,
>> create one instance of IV generator using 'crypto_alloc_skcipher'
>> and create tfms_count instances of the generator depending on the
>> number of keys.
>
> Right.  The actual number of underlying tfms that do the work
> won't change compared to the status quo.  We're just structuring
> it such that if the overall scheme is supported by the hardware
> then we can feed more than one sector at a time to it.

Thank you Herbert.
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Herbert Xu]

On Mon, Jan 02, 2017 at 12:16:45PM +0530, Binoy Jayan wrote:
> 
> Even if ciphers are allocated this way, all the encryption requests
> for cbc should still go through IV generators? So that should mean,
> create one instance of IV generator using 'crypto_alloc_skcipher'
> and create tfms_count instances of the generator depending on the
> number of keys.

Right.  The actual number of underlying tfms that do the work
won't change compared to the status quo.  We're just structuring
it such that if the overall scheme is supported by the hardware
then we can feed more than one sector at a time to it.

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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Herbert,

On 30 December 2016 at 15:57, Herbert Xu  wrote:

> This is just a matter of structuring the key for the IV generator.
> The IV generator's key in this case should be a combination of the
> key to the underlying CBC plus the set of all keys for the IV
> generator itself.  It should then allocate the required number of
> tfms as is currently done by crypt_alloc_tfms in dm-crypt.

Since I used template ciphers for the iv algorithms, I use
crypto_spawn_skcipher_alg and skcipher_register_instance
for creating the underlying cbc algorithm. I guess you suggest
to change that to make use of crypto_alloc_skcipher.

Even if ciphers are allocated this way, all the encryption requests
for cbc should still go through IV generators? So that should mean,
create one instance of IV generator using 'crypto_alloc_skcipher'
and create tfms_count instances of the generator depending on the
number of keys.

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Herbert Xu]

On Thu, Dec 29, 2016 at 02:53:25PM +0530, Binoy Jayan wrote:
>
> When we keep these in dm-crypt and if more than one key is used
> (it is actually more than one parts of the original key),
> there are more than one cipher instance created - one for each
> unique part of the key. Since the crypto requests are modelled
> to go through the template ciphers in the order:
> 
> "essiv -> cbc -> aes"
> 
> a particular cipher instance of the IV (essiv in this example) is
> responsible to encrypt an entire bigger block. If this bigger block
> is to be later split into 512 bytes blocks and then encrypted using
> the other cipher instance depending on the following formula:
> 
> key_index = sector & (key_count - 1)

This is just a matter of structuring the key for the IV generator.
The IV generator's key in this case should be a combination of the
key to the underlying CBC plus the set of all keys for the IV
generator itself.  It should then allocate the required number of
tfms as is currently done by crypt_alloc_tfms in dm-crypt.

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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Herbert,

Sorry for the delayed response, I was busy with testing dm-crypt
with bonnie++ for regressions. I tried to find some alternative
way to keep the IV algorithms' registration in the dm-crypt.
Also there were some changes done in dm-crypt keys structure too
recently.

c538f6e dm crypt: add ability to use keys from the kernel key retention service

On Thu, Dec 22, 2016 at 04:25:12PM +0530, Binoy Jayan wrote:
>
> > It doesn't have to live outside of dm-crypt.  You can register
> > these IV generators from there if you really want.
>
> Sorry, but I didn't understand this part.

What I mean is that moving the IV generators into the crypto API
does not mean the dm-crypt team giving up control over them.  You
could continue to keep them within the dm-crypt code base and
still register them through the normal crypto API mechanism

When we keep these in dm-crypt and if more than one key is used
(it is actually more than one parts of the original key),
there are more than one cipher instance created - one for each
unique part of the key. Since the crypto requests are modelled
to go through the template ciphers in the order:

"essiv -> cbc -> aes"

a particular cipher instance of the IV (essiv in this example) is
responsible to encrypt an entire bigger block. If this bigger block
is to be later split into 512 bytes blocks and then encrypted using
the other cipher instance depending on the following formula:

key_index = sector & (key_count - 1)

it is not possible as the cipher instances do not have access to
each other's instances. So, number of keys used is crucial while
performing encryption.

If there was only a single key, it should not have been a problem.
But if there are more than one key, then encrypting a bigger block
with a single key would cause backward incompatibility.
I was wondering if this is acceptable.

bigger block: What I mean by bigger block here is the set of 512-byte
blocks that dm-crypt can be optimized to process at once.

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Herbert Xu]

On Thu, Dec 22, 2016 at 04:25:12PM +0530, Binoy Jayan wrote:
>
> > It doesn't have to live outside of dm-crypt.  You can register
> > these IV generators from there if you really want.
> 
> Sorry, but I didn't understand this part.

What I mean is that moving the IV generators into the crypto API
does not mean the dm-crypt team giving up control over them.  You
could continue to keep them within the dm-crypt code base and
still register them through the normal crypto API mechanism.

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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Herbert,

On 22 December 2016 at 14:25, Herbert Xu  wrote:
> On Tue, Dec 13, 2016 at 11:01:08AM +0100, Milan Broz wrote:
>>
>> By the move everything to cryptoAPI we are basically introducing some 
>> strange mix
>> of IV and modes there, I wonder how this is going to be maintained.
>> Anyway, Herbert should say if it is ok...
>
> Well there is precedent in how do the IPsec IV generation.  In
> that case the IV generators too are completely specific to that
> application, i.e., IPsec.  However, the way structured it allowed
> us to have one single entry path from the IPsec stack into the
> crypto layer regardless of whether you are using AEAD or traditional
> encryption/hashing algorithms.
>
> For IPsec we make the IV generators behave like normal AEAD
> algorithms, except that they take the sequence number as the IV.
>
> The goal here are obviously different.  However, by employing
> the same method as we do in IPsec, it appears to me that you
> can effectively process multiple blocks at once instead of having
> to supply one block at a time due to the IV generation issue.

Thank you for clarifying that part.:)
So, I hope we can consider algorithms like lmk and tcw too as IV generation
algorithms, even though they manipulate encrypted data directly?

>> I really do not think the disk encryption key management should be moved
>> outside of dm-crypt. We cannot then change key structure later easily.

I agree with this too, only problem with this is when multiple keys are involved
(where the master key is split into 2 or more), and the key selection is made
with a modular division of the (512-byte) sector number by the number of keys.

> It doesn't have to live outside of dm-crypt.  You can register
> these IV generators from there if you really want.

Sorry, but I didn't understand this part.

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Herbert Xu]

On Tue, Dec 13, 2016 at 11:01:08AM +0100, Milan Broz wrote:
>
> By the move everything to cryptoAPI we are basically introducing some strange 
> mix
> of IV and modes there, I wonder how this is going to be maintained.
> Anyway, Herbert should say if it is ok...

Well there is precedent in how do the IPsec IV generation.  In
that case the IV generators too are completely specific to that
application, i.e., IPsec.  However, the way structured it allowed
us to have one single entry path from the IPsec stack into the
crypto layer regardless of whether you are using AEAD or traditional
encryption/hashing algorithms.

For IPsec we make the IV generators behave like normal AEAD
algorithms, except that they take the sequence number as the IV.

The goal here are obviously different.  However, by employing
the same method as we do in IPsec, it appears to me that you
can effectively process multiple blocks at once instead of having
to supply one block at a time due to the IV generation issue.
 
> I really do not think the disk encryption key management should be moved
> outside of dm-crypt. We cannot then change key structure later easily.

It doesn't have to live outside of dm-crypt.  You can register
these IV generators from there if you really want.

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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Milan,

On 13 December 2016 at 15:31, Milan Broz  wrote:

> I think that IV generators should not modify or read encrypted data directly,
> it should only generate IV.

I was trying to find more information about what you said and how a
iv generator should be written. I saw two examples of IV generators
too used with AEAD ciphers (crypto/seqiv.c and crypto/echainiv.c)

Excerpt from crypto api doc:
http://www.chronox.de/crypto-API/crypto/architecture.html#crypto-api-cipher-references-and-priority

2. Now, SEQIV uses the AEAD API function calls to invoke the associated
AEAD cipher. In our case, during the instantiation of SEQIV, the cipher
handle for GCM is provided to SEQIV. This means that SEQIV invokes
AEAD cipher operations with the GCM cipher handle.

Here, it says seqiv invokes cipher operations. However the code crypto/seqiv.c
does not look similar to how the modes are implemented which is confusing. I
was looking for an example of an IV generator used with a regular block cipher
and not a AEAD cipher. Could you point me out to some?

Thanks,
Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Binoy Jayan]

Hi Milan,

Thank you for the reply.

On 13 December 2016 at 15:31, Milan Broz  wrote:

> I really do not think the disk encryption key management should be moved
> outside of dm-crypt. We cannot then change key structure later easily.

Yes, I agree. but the key selection based on sector number restricts the
option of having a larger block size used for encryption.

>> + unsigned int key_size;
>> + unsigned int key_extra_size;
>> + unsigned int key_parts;  /* independent parts in key buffer */
>
> ^^^ these key sizes you probably mean by key management.

Yes, I mean splitting the keys into subkeys based on the keycount
parameter (as mentioned below) to the dm-crypt.

cipher[:keycount]-mode-iv:ivopts
aes:2-cbc-essiv:sha256

> It is based on way how the key is currently sent into kernel
> (one hexa string in ioctl that needs to be split) and have to be changed in 
> future.

-Binoy
--
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: [RFC PATCH v2] crypto: Add IV generation algorithms

[Milan Broz]

On 12/13/2016 09:49 AM, Binoy Jayan wrote:
> Currently, the iv generation algorithms are implemented in dm-crypt.c.
> The goal is to move these algorithms from the dm layer to the kernel
> crypto layer by implementing them as template ciphers so they can be
> implemented in hardware for performance. As part of this patchset, the
> iv-generation code is moved from the dm layer to the crypto layer and
> adapt the dm-layer to send a whole 'bio' (as defined in the block layer)
> at a time. Each bio contains the in memory representation of physically
> contiguous disk blocks. The dm layer sets up a chained scatterlist of
> these blocks split into physically contiguous segments in memory so that
> DMA can be performed. The iv generation algorithms implemented in geniv.c
> include plain, plain64, essiv, benbi, null, lmk and tcw.
...

Just few notes...

The lmk (loopAES) and tcw (old TrueCrypt mode) IVs are in fact hacks,
it is combination of IV and modification of CBC mode (in post calls).

It is used only in these disk-encryption systems, it does not make sense
to use it elsewhere (moreover tcw IV is not safe, it is here only
for compatibility reasons).

I think that IV generators should not modify or read encrypted data directly,
it should only generate IV.

By the move everything to cryptoAPI we are basically introducing some strange 
mix
of IV and modes there, I wonder how this is going to be maintained.
Anyway, Herbert should say if it is ok...

But I would imagine that cryptoAPI should implement only "real" IV generators
and these disk-encryption add-ons keep inside dmcrypt.
(and dmcrypt should probably use normal IVs through crypto API and
build some wrapper around for hacks...)

...

> 
> When using multiple keys with the original dm-crypt, the key selection is
> made based on the sector number as:
> 
> key_index = sector & (key_count - 1)
> 
> This restricts the usage of the same key for encrypting/decrypting a
> single bio. One way to solve this is to move the key management code from
> dm-crypt to cryto layer. But this seems tricky when using template ciphers
> because, when multiple ciphers are instantiated from dm layer, each cipher
> instance set with a unique subkey (part of the bigger master key) and
> these instances themselves do not have access to each other's instances
> or contexts. This way, a single instance cannot encryt/decrypt a whole bio.
> This has to be fixed.

I really do not think the disk encryption key management should be moved
outside of dm-crypt. We cannot then change key structure later easily.

But it is not only key management, you are basically moving much more internals
outside of dm-crypt:

> +struct geniv_ctx {
> + struct crypto_skcipher *child;
> + unsigned int tfms_count;
> + char *ivmode;
> + unsigned int iv_size;
> + char *ivopts;
> + char *cipher;
> + struct geniv_operations *iv_gen_ops;
> + union {
> + struct geniv_essiv_private essiv;
> + struct geniv_benbi_private benbi;
> + struct geniv_lmk_private lmk;
> + struct geniv_tcw_private tcw;
> + } iv_gen_private;
> + void *iv_private;
> + struct crypto_skcipher *tfm;
> + unsigned int key_size;
> + unsigned int key_extra_size;
> + unsigned int key_parts;  /* independent parts in key buffer */

^^^ these key sizes you probably mean by key management.
It is based on way how the key is currently sent into kernel
(one hexa string in ioctl that needs to be split) and have to be changed in 
future.

> + enum setkey_op keyop;
> + char *msg;
> + u8 *key;

Milan

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