[PATCH 0/2] crypto: talitos: Add AES-XTS mode

[PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

This adds the AES-XTS mode, supported by the Freescale SEC 3.3.2.

One of the nice things about this hardware is that it knows how to deal
with encrypt/decrypt requests that are larger than sector size, but that 
also requires that that the sector size be passed into the crypto engine
as an XTS cipher context parameter.

When a request is larger than the sector size the sector number is
incremented by the talitos engine and the tweak key is re-calculated
for the new sector.

I've tested this with 256bit and 512bit keys (tweak and data keys of 128bit
and 256bit) to ensure interoperability with the software AES-XTS
implementation.  All testing was done using dm-crypt/LUKS with
aes-xts-plain64.

Is there a better solution that just hard coding the sector size to
(1<<SECTOR_SHIFT)?  Maybe dm-crypt should be modified to pass the
sector size along with the plain/plain64 IV to an XTS algorithm?

Martin Hicks (2):
  crypto: talitos: Clean ups and comment fixes for ablkcipher commands
  crypto: talitos: Add AES-XTS Support

 drivers/crypto/talitos.c |   45 +++++++++++++++++++++++++++++++++++++--------
 drivers/crypto/talitos.h |    1 +
 2 files changed, 38 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] crypto: talitos: Clean ups and comment fixes for ablkcipher commands

[Martin Hicks]

This just cleans up some of the initializers, and improves the comments
should any other ablkcipher modes be added in the future.  The header
words 1 and 5 have more possibilities than just passing an IV.  These
are pointers to the Cipher Context in/out registers.

Signed-off-by: Martin Hicks <mort@bork.org>
---
 drivers/crypto/talitos.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 226654c..6b2a19a 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1377,11 +1377,9 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 	int sg_count, ret;
 
 	/* first DWORD empty */
-	desc->ptr[0].len = 0;
-	to_talitos_ptr(&desc->ptr[0], 0);
-	desc->ptr[0].j_extent = 0;
+	desc->ptr[0] = zero_entry;
 
-	/* cipher iv */
+	/* cipher context */
 	to_talitos_ptr(&desc->ptr[1], edesc->iv_dma);
 	desc->ptr[1].len = cpu_to_be16(ivsize);
 	desc->ptr[1].j_extent = 0;
@@ -1444,14 +1442,12 @@ static int common_nonsnoop(struct talitos_edesc *edesc,
 					   edesc->dma_len, DMA_BIDIRECTIONAL);
 	}
 
-	/* iv out */
+	/* cipher context out */
 	map_single_talitos_ptr(dev, &desc->ptr[5], ivsize, ctx->iv, 0,
 			       DMA_FROM_DEVICE);
 
 	/* last DWORD empty */
-	desc->ptr[6].len = 0;
-	to_talitos_ptr(&desc->ptr[6], 0);
-	desc->ptr[6].j_extent = 0;
+	desc->ptr[6] = zero_entry;
 
 	edesc->req.callback = callback;
 	edesc->req.context = areq;
-- 
1.7.10.4

[PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Martin Hicks]

The newer talitos hardware has support for AES in XTS mode.

Signed-off-by: Martin Hicks <mort@bork.org>
---
 drivers/crypto/talitos.c |   33 +++++++++++++++++++++++++++++++++
 drivers/crypto/talitos.h |    1 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6b2a19a..38cbde1 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -40,9 +40,11 @@
 #include <linux/spinlock.h>
 #include <linux/rtnetlink.h>
 #include <linux/slab.h>
+#include <linux/device-mapper.h>
 
 #include <crypto/algapi.h>
 #include <crypto/aes.h>
+#include <crypto/xts.h>
 #include <crypto/des.h>
 #include <crypto/sha.h>
 #include <crypto/md5.h>
@@ -1464,9 +1466,22 @@ static struct talitos_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request *
 						    areq, bool encrypt)
 {
 	struct crypto_ablkcipher *cipher = crypto_ablkcipher_reqtfm(areq);
+	struct crypto_tfm *tfm = (struct crypto_tfm *)cipher;
 	struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
 	unsigned int ivsize = crypto_ablkcipher_ivsize(cipher);
 
+	/*
+	 * AES-XTS uses the first two AES Context registers for:
+	 *
+	 *     Register 1:   Sector Number (Little Endian)
+	 *     Register 2:   Sector Size   (Big Endian)
+	 *
+	 * Whereas AES-CBC uses registers 1/2 as a 16-byte IV.
+	 */
+	if (!strcmp(crypto_tfm_alg_name(tfm),"xts(aes)"))
+		/* Fixed sized sector */
+		*((u64 *)areq->info + 1) = cpu_to_be64((1<<SECTOR_SHIFT));
+
 	return talitos_edesc_alloc(ctx->dev, NULL, areq->src, areq->dst,
 				   areq->info, 0, areq->nbytes, 0, ivsize, 0,
 				   areq->base.flags, &areq->base, encrypt);
@@ -2192,6 +2207,24 @@ static struct talitos_alg_template driver_algs[] = {
 		                     DESC_HDR_MODE0_DEU_CBC |
 		                     DESC_HDR_MODE0_DEU_3DES,
 	},
+	{	.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
+		.alg.crypto = {
+			.cra_name = "xts(aes)",
+			.cra_driver_name = "xts-aes-talitos",
+			.cra_blocksize = XTS_BLOCK_SIZE,
+			.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER |
+                                     CRYPTO_ALG_ASYNC,
+			.cra_ablkcipher = {
+				.min_keysize = AES_MIN_KEY_SIZE * 2,
+				.max_keysize = AES_MAX_KEY_SIZE * 2,
+				.ivsize = XTS_BLOCK_SIZE,
+			}
+		},
+		.desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
+					DESC_HDR_SEL0_AESU |
+					DESC_HDR_MODE0_AESU_XTS,
+	},
+
 	/* AHASH algorithms. */
 	{	.type = CRYPTO_ALG_TYPE_AHASH,
 		.alg.hash = {
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index a6f73e2..735da82 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -316,6 +316,7 @@ extern int talitos_submit(struct device *dev, int ch, struct talitos_edesc *edes
 /* primary execution unit mode (MODE0) and derivatives */
 #define	DESC_HDR_MODE0_ENCRYPT		cpu_to_be32(0x00100000)
 #define	DESC_HDR_MODE0_AESU_CBC		cpu_to_be32(0x00200000)
+#define	DESC_HDR_MODE0_AESU_XTS		cpu_to_be32(0x04200000)
 #define	DESC_HDR_MODE0_DEU_CBC		cpu_to_be32(0x00400000)
 #define	DESC_HDR_MODE0_DEU_3DES		cpu_to_be32(0x00200000)
 #define	DESC_HDR_MODE0_MDEU_CONT	cpu_to_be32(0x08000000)
-- 
1.7.10.4

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Horia Geantă]

On 2/20/2015 7:00 PM, Martin Hicks wrote:
> The newer talitos hardware has support for AES in XTS mode.
> 
> Signed-off-by: Martin Hicks <mort@bork.org>
> ---

checkpatch complains about formatting, please check.

>  drivers/crypto/talitos.c |   33 +++++++++++++++++++++++++++++++++
>  drivers/crypto/talitos.h |    1 +
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
> index 6b2a19a..38cbde1 100644
> --- a/drivers/crypto/talitos.c
> +++ b/drivers/crypto/talitos.c
> @@ -40,9 +40,11 @@
>  #include <linux/spinlock.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/slab.h>
> +#include <linux/device-mapper.h>
>  
>  #include <crypto/algapi.h>
>  #include <crypto/aes.h>
> +#include <crypto/xts.h>
>  #include <crypto/des.h>
>  #include <crypto/sha.h>
>  #include <crypto/md5.h>
> @@ -1464,9 +1466,22 @@ static struct talitos_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request *
>  						    areq, bool encrypt)
>  {
>  	struct crypto_ablkcipher *cipher = crypto_ablkcipher_reqtfm(areq);
> +	struct crypto_tfm *tfm = (struct crypto_tfm *)cipher;
>  	struct talitos_ctx *ctx = crypto_ablkcipher_ctx(cipher);
>  	unsigned int ivsize = crypto_ablkcipher_ivsize(cipher);
>  
> +	/*
> +	 * AES-XTS uses the first two AES Context registers for:
> +	 *
> +	 *     Register 1:   Sector Number (Little Endian)
> +	 *     Register 2:   Sector Size   (Big Endian)
> +	 *
> +	 * Whereas AES-CBC uses registers 1/2 as a 16-byte IV.
> +	 */
> +	if (!strcmp(crypto_tfm_alg_name(tfm),"xts(aes)"))

I guess it would be better to use ctx->desc_hdr_template instead of
string comparison.

> +		/* Fixed sized sector */
> +		*((u64 *)areq->info + 1) = cpu_to_be64((1<<SECTOR_SHIFT));
> +
>  	return talitos_edesc_alloc(ctx->dev, NULL, areq->src, areq->dst,
>  				   areq->info, 0, areq->nbytes, 0, ivsize, 0,
>  				   areq->base.flags, &areq->base, encrypt);
> @@ -2192,6 +2207,24 @@ static struct talitos_alg_template driver_algs[] = {
>  		                     DESC_HDR_MODE0_DEU_CBC |
>  		                     DESC_HDR_MODE0_DEU_3DES,
>  	},
> +	{	.type = CRYPTO_ALG_TYPE_ABLKCIPHER,
> +		.alg.crypto = {
> +			.cra_name = "xts(aes)",
> +			.cra_driver_name = "xts-aes-talitos",
> +			.cra_blocksize = XTS_BLOCK_SIZE,
> +			.cra_flags = CRYPTO_ALG_TYPE_ABLKCIPHER |
> +                                     CRYPTO_ALG_ASYNC,
> +			.cra_ablkcipher = {
> +				.min_keysize = AES_MIN_KEY_SIZE * 2,
> +				.max_keysize = AES_MAX_KEY_SIZE * 2,
> +				.ivsize = XTS_BLOCK_SIZE,
> +			}
> +		},
> +		.desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
> +					DESC_HDR_SEL0_AESU |
> +					DESC_HDR_MODE0_AESU_XTS,
> +	},
> +
>  	/* AHASH algorithms. */
>  	{	.type = CRYPTO_ALG_TYPE_AHASH,
>  		.alg.hash = {
> diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
> index a6f73e2..735da82 100644
> --- a/drivers/crypto/talitos.h
> +++ b/drivers/crypto/talitos.h
> @@ -316,6 +316,7 @@ extern int talitos_submit(struct device *dev, int ch, struct talitos_edesc *edes
>  /* primary execution unit mode (MODE0) and derivatives */
>  #define	DESC_HDR_MODE0_ENCRYPT		cpu_to_be32(0x00100000)
>  #define	DESC_HDR_MODE0_AESU_CBC		cpu_to_be32(0x00200000)
> +#define	DESC_HDR_MODE0_AESU_XTS		cpu_to_be32(0x04200000)
>  #define	DESC_HDR_MODE0_DEU_CBC		cpu_to_be32(0x00400000)
>  #define	DESC_HDR_MODE0_DEU_3DES		cpu_to_be32(0x00200000)
>  #define	DESC_HDR_MODE0_MDEU_CONT	cpu_to_be32(0x08000000)
> 

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Kim Phillips]

On Fri, 20 Feb 2015 12:00:10 -0500
Martin Hicks <mort@bork.org> wrote:

> The newer talitos hardware has support for AES in XTS mode.

Assuming it's the same thing, AES-XCBC gets added with SEC v3.0
h/w.  Assuming hw_supports() doesn't already support this algorithm
combination (technically via the mode bit), this needs to be
reflected in the patch so the driver doesn't think SEC 2.x devices
can do XTS, too.

> +		.desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
> +					DESC_HDR_SEL0_AESU |
> +					DESC_HDR_MODE0_AESU_XTS,

...

>  /* primary execution unit mode (MODE0) and derivatives */
>  #define	DESC_HDR_MODE0_ENCRYPT		cpu_to_be32(0x00100000)
>  #define	DESC_HDR_MODE0_AESU_CBC		cpu_to_be32(0x00200000)
> +#define	DESC_HDR_MODE0_AESU_XTS		cpu_to_be32(0x04200000)

I'd prefer these defines be kept as single bit definitions, and keep
their names from the manual.  An XTS-specific definition can be
composed from them either after this, or manually in the
desc_hdr_template assignment above.

Thanks,

Kim

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Martin Hicks]

On Thu, Mar 5, 2015 at 7:16 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> On Fri, 20 Feb 2015 12:00:10 -0500
> Martin Hicks <mort@bork.org> wrote:
>
>> The newer talitos hardware has support for AES in XTS mode.
>
> Assuming it's the same thing, AES-XCBC gets added with SEC v3.0
> h/w.  Assuming hw_supports() doesn't already support this algorithm

AES-XCBC isn't the same thing as AES-XTS.

> combination (technically via the mode bit), this needs to be
> reflected in the patch so the driver doesn't think SEC 2.x devices
> can do XTS, too.

Right.  I hadn't looked into how exactly hw_supports() works.  It only
indicates which execution units are present (in this case the AES
unit).  I actually think XTS gets introduced in SEC v3.3.2.  I also
have an MPC8379 (sec3.3) and it does not have XTS.

Can you look internally to find out in which hardware it was
introduced?  Is there a SEC 3.3.1 that also has XTS?

I guess I just need a ->features flag to conditionally register the
XTS algorithm for 3.3.x and newer.  Adding anything more complicated
doesn't seem warranted at this time, although that could change if
someone came along and needed to add a whole whack more of the AES
modes that were introduced at various times in the different SEC
revisions.

>
>> +             .desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
>> +                                     DESC_HDR_SEL0_AESU |
>> +                                     DESC_HDR_MODE0_AESU_XTS,
>
> ...
>
>>  /* primary execution unit mode (MODE0) and derivatives */
>>  #define      DESC_HDR_MODE0_ENCRYPT          cpu_to_be32(0x00100000)
>>  #define      DESC_HDR_MODE0_AESU_CBC         cpu_to_be32(0x00200000)
>> +#define      DESC_HDR_MODE0_AESU_XTS         cpu_to_be32(0x04200000)
>
> I'd prefer these defines be kept as single bit definitions, and keep
> their names from the manual.  An XTS-specific definition can be
> composed from them either after this, or manually in the
> desc_hdr_template assignment above.

It doesn't look like there are divisible single-bit definitions for
the MODE0 bits. The AES Cipher modes are composed of 5 bits called
CipherMode, Extended CipherMode and Aux2.  Individually they don't
seem to mean anything.  Unless you want something like:

#define AES_MODE(cm, ecm, aux2)   cpu_to_be32(((ecm<<6) | (aux2<<5) |
(cm<<1)) << 20)

#define DESC_HDR_MODE0_AESU_CBC  AES_MODE(1, 0, 0)
#define DESC_HDR_MODE0_AESU_XTS   AES_MODE(1, 1, 0)

Thanks,
mh

-- 
Martin Hicks P.Eng.      |         mort@bork.org
Bork Consulting Inc.     |   +1 (613) 266-2296

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Martin Hicks]

Hi Kim,

On Fri, Mar 6, 2015 at 11:49 AM, Martin Hicks <mort@bork.org> wrote:
> On Thu, Mar 5, 2015 at 7:16 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> On Fri, 20 Feb 2015 12:00:10 -0500
>> Martin Hicks <mort@bork.org> wrote:
>>
>>> The newer talitos hardware has support for AES in XTS mode.
>>
>> Assuming it's the same thing, AES-XCBC gets added with SEC v3.0
>> h/w.  Assuming hw_supports() doesn't already support this algorithm
>
> AES-XCBC isn't the same thing as AES-XTS.
>
>> combination (technically via the mode bit), this needs to be
>> reflected in the patch so the driver doesn't think SEC 2.x devices
>> can do XTS, too.
>
> Right.  I hadn't looked into how exactly hw_supports() works.  It only
> indicates which execution units are present (in this case the AES
> unit).  I actually think XTS gets introduced in SEC v3.3.2.  I also
> have an MPC8379 (sec3.3) and it does not have XTS.
>

I'm wrong about the 8379.  It's SEC3.0.  So XTS was introduced
somewhere between 3.0 and 3.3.2

> Can you look internally to find out in which hardware it was
> introduced?  Is there a SEC 3.3.1 that also has XTS?

Can you still look into where XTS was added?  I'm not even sure how
many versions of hardware exist between 3.0 and 3.3.2

Thanks,
mh

>
> I guess I just need a ->features flag to conditionally register the
> XTS algorithm for 3.3.x and newer.  Adding anything more complicated
> doesn't seem warranted at this time, although that could change if
> someone came along and needed to add a whole whack more of the AES
> modes that were introduced at various times in the different SEC
> revisions.
>
>>
>>> +             .desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
>>> +                                     DESC_HDR_SEL0_AESU |
>>> +                                     DESC_HDR_MODE0_AESU_XTS,
>>
>> ...
>>
>>>  /* primary execution unit mode (MODE0) and derivatives */
>>>  #define      DESC_HDR_MODE0_ENCRYPT          cpu_to_be32(0x00100000)
>>>  #define      DESC_HDR_MODE0_AESU_CBC         cpu_to_be32(0x00200000)
>>> +#define      DESC_HDR_MODE0_AESU_XTS         cpu_to_be32(0x04200000)
>>
>> I'd prefer these defines be kept as single bit definitions, and keep
>> their names from the manual.  An XTS-specific definition can be
>> composed from them either after this, or manually in the
>> desc_hdr_template assignment above.
>
> It doesn't look like there are divisible single-bit definitions for
> the MODE0 bits. The AES Cipher modes are composed of 5 bits called
> CipherMode, Extended CipherMode and Aux2.  Individually they don't
> seem to mean anything.  Unless you want something like:
>
> #define AES_MODE(cm, ecm, aux2)   cpu_to_be32(((ecm<<6) | (aux2<<5) |
> (cm<<1)) << 20)
>
> #define DESC_HDR_MODE0_AESU_CBC  AES_MODE(1, 0, 0)
> #define DESC_HDR_MODE0_AESU_XTS   AES_MODE(1, 1, 0)
>
> Thanks,
> mh
>
> --
> Martin Hicks P.Eng.      |         mort@bork.org
> Bork Consulting Inc.     |   +1 (613) 266-2296



-- 
Martin Hicks P.Eng.      |         mort@bork.org
Bork Consulting Inc.     |   +1 (613) 266-2296

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Kim Phillips]

On Fri, 6 Mar 2015 11:49:43 -0500
Martin Hicks <mort@bork.org> wrote:

> On Thu, Mar 5, 2015 at 7:16 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> > On Fri, 20 Feb 2015 12:00:10 -0500
> > Martin Hicks <mort@bork.org> wrote:
> >
> >> The newer talitos hardware has support for AES in XTS mode.
> >
> > Assuming it's the same thing, AES-XCBC gets added with SEC v3.0
> > h/w.  Assuming hw_supports() doesn't already support this algorithm
> 
> AES-XCBC isn't the same thing as AES-XTS.

Thanks.

> > combination (technically via the mode bit), this needs to be
> > reflected in the patch so the driver doesn't think SEC 2.x devices
> > can do XTS, too.
> 
> Right.  I hadn't looked into how exactly hw_supports() works.  It only
> indicates which execution units are present (in this case the AES
> unit).  I actually think XTS gets introduced in SEC v3.3.2.  I also
> have an MPC8379 (sec3.3) and it does not have XTS.
> 
> Can you look internally to find out in which hardware it was
> introduced?  Is there a SEC 3.3.1 that also has XTS?

later MPC8315Es had a SEC v3.3.1, but AFAICT, it doesn't support
XTS, so, yes, it's likely v3.3.2 and above (if any).

> I guess I just need a ->features flag to conditionally register the
> XTS algorithm for 3.3.x and newer.  Adding anything more complicated
> doesn't seem warranted at this time, although that could change if
> someone came along and needed to add a whole whack more of the AES
> modes that were introduced at various times in the different SEC
> revisions.

OK.  Note: there's some SEC node fixup code in u-boot that could be
used for this job, too.

> >> +             .desc_hdr_template = DESC_HDR_TYPE_COMMON_NONSNOOP_NO_AFEU |
> >> +                                     DESC_HDR_SEL0_AESU |
> >> +                                     DESC_HDR_MODE0_AESU_XTS,
> >
> > ...
> >
> >>  /* primary execution unit mode (MODE0) and derivatives */
> >>  #define      DESC_HDR_MODE0_ENCRYPT          cpu_to_be32(0x00100000)
> >>  #define      DESC_HDR_MODE0_AESU_CBC         cpu_to_be32(0x00200000)
> >> +#define      DESC_HDR_MODE0_AESU_XTS         cpu_to_be32(0x04200000)
> >
> > I'd prefer these defines be kept as single bit definitions, and keep
> > their names from the manual.  An XTS-specific definition can be
> > composed from them either after this, or manually in the
> > desc_hdr_template assignment above.
> 
> It doesn't look like there are divisible single-bit definitions for
> the MODE0 bits. The AES Cipher modes are composed of 5 bits called
> CipherMode, Extended CipherMode and Aux2.  Individually they don't
> seem to mean anything.  Unless you want something like:
> 
> #define AES_MODE(cm, ecm, aux2)   cpu_to_be32(((ecm<<6) | (aux2<<5) |
> (cm<<1)) << 20)
> 
> #define DESC_HDR_MODE0_AESU_CBC  AES_MODE(1, 0, 0)
> #define DESC_HDR_MODE0_AESU_XTS   AES_MODE(1, 1, 0)

the way you had it seems to be ok for now.

Kim

Re: [PATCH 2/2] crypto: talitos: Add AES-XTS Support

[Horia Geantă]

On 3/7/2015 3:16 AM, Kim Phillips wrote:
> On Fri, 6 Mar 2015 11:49:43 -0500
> Martin Hicks <mort@bork.org> wrote:
> 
>> On Thu, Mar 5, 2015 at 7:16 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>>> On Fri, 20 Feb 2015 12:00:10 -0500
>>> Martin Hicks <mort@bork.org> wrote:
>>>
>>>> The newer talitos hardware has support for AES in XTS mode.
>>>
>>> Assuming it's the same thing, AES-XCBC gets added with SEC v3.0
>>> h/w.  Assuming hw_supports() doesn't already support this algorithm
>>
>> AES-XCBC isn't the same thing as AES-XTS.
> 
> Thanks.
> 
>>> combination (technically via the mode bit), this needs to be
>>> reflected in the patch so the driver doesn't think SEC 2.x devices
>>> can do XTS, too.
>>
>> Right.  I hadn't looked into how exactly hw_supports() works.  It only
>> indicates which execution units are present (in this case the AES
>> unit).  I actually think XTS gets introduced in SEC v3.3.2.  I also
>> have an MPC8379 (sec3.3) and it does not have XTS.
>>
>> Can you look internally to find out in which hardware it was
>> introduced?  Is there a SEC 3.3.1 that also has XTS?
> 
> later MPC8315Es had a SEC v3.3.1, but AFAICT, it doesn't support
> XTS, so, yes, it's likely v3.3.2 and above (if any).

There's a public application note on freescale.com:
"AN3645 - SEC 2x/3x Descriptor Programmer's Guide" (Rev.3/2010)

"Table 4 - EUs Supported in Each SEC Version" summarizes which
algorithms / modes are supported for every talitos version.
Unfortunately this goes up to SEC 3.3.1.
Since XTS doesn't show up, 3.3.2 would be the first supporting it.

Horia

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Horia Geantă]

On 2/20/2015 7:00 PM, Martin Hicks wrote:
> This adds the AES-XTS mode, supported by the Freescale SEC 3.3.2.
> 
> One of the nice things about this hardware is that it knows how to deal
> with encrypt/decrypt requests that are larger than sector size, but that 
> also requires that that the sector size be passed into the crypto engine
> as an XTS cipher context parameter.
> 
> When a request is larger than the sector size the sector number is
> incremented by the talitos engine and the tweak key is re-calculated
> for the new sector.
> 
> I've tested this with 256bit and 512bit keys (tweak and data keys of 128bit
> and 256bit) to ensure interoperability with the software AES-XTS
> implementation.  All testing was done using dm-crypt/LUKS with
> aes-xts-plain64.
> 
> Is there a better solution that just hard coding the sector size to
> (1<<SECTOR_SHIFT)?  Maybe dm-crypt should be modified to pass the
> sector size along with the plain/plain64 IV to an XTS algorithm?

AFAICT, SW implementation of xts mode in kernel (crypto/xts.c) is not
aware of a sector size ("data unit size" in IEEE P1619 terminology):
There's a hidden assumption that all the data send to xts in one request
belongs to a single sector. Even more, it's supposed that the first
16-byte block in the request is "block 0" in the sector. These can be
seen from the way the tweak ("T") value is computed.
(Side note: there's no support of ciphertext stealing in crypto/xts.c -
i.e. sector sizes must be a multiple of underlying block cipher size -
that is 16B.)

If dm-crypt would be modified to pass sector size somehow, all in-kernel
xts implementations would have to be made aware of the change.
I have nothing against this, but let's see what crypto maintainers have
to say...

BTW, there were some discussions back in 2013 wrt. being able to
configure / increase sector size, smth. crypto engines would benefit from:
http://www.saout.de/pipermail/dm-crypt/2013-January/003125.html
(experimental patch)
http://www.saout.de/pipermail/dm-crypt/2013-March/003202.html

The experimental patch sends sector size as the req->nbytes - hidden
assumption: data size sent in an xts crypto request equals a sector.

I am not sure if there was a follow-up though...
Adding Milan - maybe he could shed some light.

Thanks,
Horia

> 
> Martin Hicks (2):
>   crypto: talitos: Clean ups and comment fixes for ablkcipher commands
>   crypto: talitos: Add AES-XTS Support
> 
>  drivers/crypto/talitos.c |   45 +++++++++++++++++++++++++++++++++++++--------
>  drivers/crypto/talitos.h |    1 +
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Milan Broz]

On 03/02/2015 02:25 PM, Horia Geantă wrote:
> On 2/20/2015 7:00 PM, Martin Hicks wrote:
>> This adds the AES-XTS mode, supported by the Freescale SEC 3.3.2.
>>
>> One of the nice things about this hardware is that it knows how to deal
>> with encrypt/decrypt requests that are larger than sector size, but that 
>> also requires that that the sector size be passed into the crypto engine
>> as an XTS cipher context parameter.
>>
>> When a request is larger than the sector size the sector number is
>> incremented by the talitos engine and the tweak key is re-calculated
>> for the new sector.
>>
>> I've tested this with 256bit and 512bit keys (tweak and data keys of 128bit
>> and 256bit) to ensure interoperability with the software AES-XTS
>> implementation.  All testing was done using dm-crypt/LUKS with
>> aes-xts-plain64.
>>
>> Is there a better solution that just hard coding the sector size to
>> (1<<SECTOR_SHIFT)?  Maybe dm-crypt should be modified to pass the
>> sector size along with the plain/plain64 IV to an XTS algorithm?
> 
> AFAICT, SW implementation of xts mode in kernel (crypto/xts.c) is not
> aware of a sector size ("data unit size" in IEEE P1619 terminology):
> There's a hidden assumption that all the data send to xts in one request
> belongs to a single sector. Even more, it's supposed that the first
> 16-byte block in the request is "block 0" in the sector. These can be
> seen from the way the tweak ("T") value is computed.
> (Side note: there's no support of ciphertext stealing in crypto/xts.c -
> i.e. sector sizes must be a multiple of underlying block cipher size -
> that is 16B.)
> 
> If dm-crypt would be modified to pass sector size somehow, all in-kernel
> xts implementations would have to be made aware of the change.
> I have nothing against this, but let's see what crypto maintainers have
> to say...
> 
> BTW, there were some discussions back in 2013 wrt. being able to
> configure / increase sector size, smth. crypto engines would benefit from:
> http://www.saout.de/pipermail/dm-crypt/2013-January/003125.html
> (experimental patch)
> http://www.saout.de/pipermail/dm-crypt/2013-March/003202.html
> 
> The experimental patch sends sector size as the req->nbytes - hidden
> assumption: data size sent in an xts crypto request equals a sector.

There was no follow-up but the idea is not yet abandoned :-)

Dmcrypt will always use "sector" as a minimal unit
(and I believe sectors will by always multiple of block size so
no need for ciphertext stealing in XTS.)

For now, dmcrypt always use 512 bytes sector size.

If crypto API allows to encrypt more sectors in one run
(handling IV internally) dmcrypt can be modified of course.

But do not forget we can use another IV (not only sequential number)
e.g. ESSIV with XTS as well (even if it doesn't make much sense, some people
are using it).

Maybe the following question would be if the dmcrypt sector IV algorithms
should moved into crypto API as well.
(But because I misused dmcrypt IVs hooks for some additional operations
for loopAES and old Truecrypt CBC mode, it is not so simple...)

Milan

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

On Mon, Mar 02, 2015 at 03:37:28PM +0100, Milan Broz wrote:
> 
> If crypto API allows to encrypt more sectors in one run
> (handling IV internally) dmcrypt can be modified of course.
> 
> But do not forget we can use another IV (not only sequential number)
> e.g. ESSIV with XTS as well (even if it doesn't make much sense, some people
> are using it).

Interesting, I'd not considered using XTS with an IV other than plain/64.
The talitos hardware would not support aes/xts in any mode other than
plain/plain64 I don't think...Although perhaps you could push in an 8-byte
IV and the hardware would interpret it as the sector #.

> Maybe the following question would be if the dmcrypt sector IV algorithms
> should moved into crypto API as well.
> (But because I misused dmcrypt IVs hooks for some additional operations
> for loopAES and old Truecrypt CBC mode, it is not so simple...)

Speaking again with talitos in mind, there would be no advantage for this
hardware.  Although larger requests are possible only a single IV can be
provided per request, so for algorithms like AES-CBC and dm-crypt 512byte IOs
are the only option (short of switching to 4kB block size).

mh

-- 
Martin Hicks P.Eng.    |      mort@bork.org
Bork Consulting Inc.   |  +1 (613) 266-2296

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Horia Geantă]

On 3/3/2015 12:09 AM, Martin Hicks wrote:
> 
> On Mon, Mar 02, 2015 at 03:37:28PM +0100, Milan Broz wrote:
>>
>> If crypto API allows to encrypt more sectors in one run
>> (handling IV internally) dmcrypt can be modified of course.
>>
>> But do not forget we can use another IV (not only sequential number)
>> e.g. ESSIV with XTS as well (even if it doesn't make much sense, some people
>> are using it).
> 
> Interesting, I'd not considered using XTS with an IV other than plain/64.
> The talitos hardware would not support aes/xts in any mode other than
> plain/plain64 I don't think...Although perhaps you could push in an 8-byte
> IV and the hardware would interpret it as the sector #.
> 

For talitos, there are two cases:

1. request data size is <= data unit / sector size
talitos can handle any IV / tweak scheme

2. request data size > sector size
since talitos internally generates the IV for the next sector by
incrementing the previous IV, only IV schemes that allocate consecutive
IV to consecutive sectors will function correctly.

Let's not forget what XTS standard says about IVs / tweak values:
- each data unit (sector in this case) is assigned a non-negative tweak
value and
- tweak values are assigned *consecutively*, starting from an arbitrary
non-negative value
- there's no requirement for tweak values to be unpredictable

Thus, in theory ESSIV is not supposed to be used with XTS mode: the IVs
for consecutive sectors are not consecutive values.
In practice, as Milan said, the combination is sometimes used. It
functions correctly in SW (and also in talitos as long as req. data size
<= sector size).

>> Maybe the following question would be if the dmcrypt sector IV algorithms
>> should moved into crypto API as well.
>> (But because I misused dmcrypt IVs hooks for some additional operations
>> for loopAES and old Truecrypt CBC mode, it is not so simple...)
> 
> Speaking again with talitos in mind, there would be no advantage for this
> hardware.  Although larger requests are possible only a single IV can be
> provided per request, so for algorithms like AES-CBC and dm-crypt 512byte IOs
> are the only option (short of switching to 4kB block size).

Right, as explained above talitos does what the XTS mode standard
mandates. So it won't work properly in case of cbc-aes:essiv with
request sizes larger than sector size.

Still, in SW at least, XTS could be improved to process more sectors in
one shot, regardless of the IV scheme used - as long as there's a
IV.next() function and both data size and sector size are known.

Horia

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

On Tue, Mar 3, 2015 at 10:44 AM, Horia Geant=C4=83
<horia.geanta@freescale.com> wrote:
> On 3/3/2015 12:09 AM, Martin Hicks wrote:
>>
>> On Mon, Mar 02, 2015 at 03:37:28PM +0100, Milan Broz wrote:
>>>
>>> If crypto API allows to encrypt more sectors in one run
>>> (handling IV internally) dmcrypt can be modified of course.
>>>
>>> But do not forget we can use another IV (not only sequential number)
>>> e.g. ESSIV with XTS as well (even if it doesn't make much sense, some p=
eople
>>> are using it).
>>
>> Interesting, I'd not considered using XTS with an IV other than plain/64=
.
>> The talitos hardware would not support aes/xts in any mode other than
>> plain/plain64 I don't think...Although perhaps you could push in an 8-by=
te
>> IV and the hardware would interpret it as the sector #.
>>
>
> For talitos, there are two cases:
>
> 1. request data size is <=3D data unit / sector size
> talitos can handle any IV / tweak scheme
>
> 2. request data size > sector size
> since talitos internally generates the IV for the next sector by
> incrementing the previous IV, only IV schemes that allocate consecutive
> IV to consecutive sectors will function correctly.
>

it's not clear to me that #1 is right.  I guess it could be, but the
IV length would be limited to 8 bytes.

This also points out that claiming that the XTS IV size is 16 bytes,
as my current patch does, could be problematic.  It's handy because
the first 8 bytes should contain a plain64 sector #, and the second
u64 can be used to encode the sector size but it would be a mistake
for someone to use the second 8 bytes for the rest of a 16byte IV.

mh

--=20
Martin Hicks P.Eng.      |         mort@bork.org
Bork Consulting Inc.     |   +1 (613) 266-2296

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Horia Geantă]

On 3/3/2015 7:44 PM, Martin Hicks wrote:
> On Tue, Mar 3, 2015 at 10:44 AM, Horia Geantă
> <horia.geanta@freescale.com> wrote:
>> On 3/3/2015 12:09 AM, Martin Hicks wrote:
>>>
>>> On Mon, Mar 02, 2015 at 03:37:28PM +0100, Milan Broz wrote:
>>>>
>>>> If crypto API allows to encrypt more sectors in one run
>>>> (handling IV internally) dmcrypt can be modified of course.
>>>>
>>>> But do not forget we can use another IV (not only sequential number)
>>>> e.g. ESSIV with XTS as well (even if it doesn't make much sense, some people
>>>> are using it).
>>>
>>> Interesting, I'd not considered using XTS with an IV other than plain/64.
>>> The talitos hardware would not support aes/xts in any mode other than
>>> plain/plain64 I don't think...Although perhaps you could push in an 8-byte
>>> IV and the hardware would interpret it as the sector #.
>>>
>>
>> For talitos, there are two cases:
>>
>> 1. request data size is <= data unit / sector size
>> talitos can handle any IV / tweak scheme
>>
>> 2. request data size > sector size
>> since talitos internally generates the IV for the next sector by
>> incrementing the previous IV, only IV schemes that allocate consecutive
>> IV to consecutive sectors will function correctly.
>>
> 
> it's not clear to me that #1 is right.  I guess it could be, but the
> IV length would be limited to 8 bytes.

Yes, there's a limitation in talitos wrt. XTS IV / tweak size - it's up
to 8 bytes.
So I guess ESSIV won't work with talitos-xts, since the encrypted IV
output is 16 bytes.
But as previously said, ESSIV breaks the XTS standard requirement for
having a consecutive IV for consecutive blocks. ESSIV should really be
used only with disk-level encryption schemes that require an
unpredictable IV.

> 
> This also points out that claiming that the XTS IV size is 16 bytes,
> as my current patch does, could be problematic.  It's handy because
> the first 8 bytes should contain a plain64 sector #, and the second
> u64 can be used to encode the sector size but it would be a mistake
> for someone to use the second 8 bytes for the rest of a 16byte IV.

XTS IV *is* 16 bytes. The fact that xts-talitos can handle only 8 bytes
is a problem indeed, but for plain and plain64 should not matter.

Horia

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

On Mon, Mar 9, 2015 at 6:16 AM, Horia Geant=C4=83 <horia.geanta@freescale.c=
om> wrote:
> On 3/3/2015 7:44 PM, Martin Hicks wrote:
>> On Tue, Mar 3, 2015 at 10:44 AM, Horia Geant=C4=83
>> <horia.geanta@freescale.com> wrote:
>>>
>>> For talitos, there are two cases:
>>>
>>> 1. request data size is <=3D data unit / sector size
>>> talitos can handle any IV / tweak scheme
>>>
>>> 2. request data size > sector size
>>> since talitos internally generates the IV for the next sector by
>>> incrementing the previous IV, only IV schemes that allocate consecutive
>>> IV to consecutive sectors will function correctly.
>>>
>>
>> it's not clear to me that #1 is right.  I guess it could be, but the
>> IV length would be limited to 8 bytes.
>
> Yes, there's a limitation in talitos wrt. XTS IV / tweak size - it's up
> to 8 bytes.
> So I guess ESSIV won't work with talitos-xts, since the encrypted IV
> output is 16 bytes.
> But as previously said, ESSIV breaks the XTS standard requirement for
> having a consecutive IV for consecutive blocks. ESSIV should really be
> used only with disk-level encryption schemes that require an
> unpredictable IV.

Ok.  I'll verify that the second half of the IV is zeroed.

One last thing that I'm not sure of is what string to place in
cra_ablkcipher.geniv field.   "eseqiv" seems wrong if plain/plain64
are the IVs that XTS is designed for.

Thanks,
mh

--=20
Martin Hicks P.Eng.      |         mort@bork.org
Bork Consulting Inc.     |   +1 (613) 266-2296

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Horia Geantă]

On 3/9/2015 5:08 PM, Martin Hicks wrote:
> On Mon, Mar 9, 2015 at 6:16 AM, Horia Geantă <horia.geanta@freescale.com> wrote:
>> On 3/3/2015 7:44 PM, Martin Hicks wrote:
>>> On Tue, Mar 3, 2015 at 10:44 AM, Horia Geantă
>>> <horia.geanta@freescale.com> wrote:
>>>>
>>>> For talitos, there are two cases:
>>>>
>>>> 1. request data size is <= data unit / sector size
>>>> talitos can handle any IV / tweak scheme
>>>>
>>>> 2. request data size > sector size
>>>> since talitos internally generates the IV for the next sector by
>>>> incrementing the previous IV, only IV schemes that allocate consecutive
>>>> IV to consecutive sectors will function correctly.
>>>>
>>>
>>> it's not clear to me that #1 is right.  I guess it could be, but the
>>> IV length would be limited to 8 bytes.
>>
>> Yes, there's a limitation in talitos wrt. XTS IV / tweak size - it's up
>> to 8 bytes.
>> So I guess ESSIV won't work with talitos-xts, since the encrypted IV
>> output is 16 bytes.
>> But as previously said, ESSIV breaks the XTS standard requirement for
>> having a consecutive IV for consecutive blocks. ESSIV should really be
>> used only with disk-level encryption schemes that require an
>> unpredictable IV.
> 
> Ok.  I'll verify that the second half of the IV is zeroed.
> 
> One last thing that I'm not sure of is what string to place in
> cra_ablkcipher.geniv field.   "eseqiv" seems wrong if plain/plain64
> are the IVs that XTS is designed for.

Right. But since currently dm-crypt does not use .givencrypt and deals
with IV generation by itself, we're safe.
When dm-crypt IV generation will be moved to crypto, we'll have to
revisit this.

While here: note that xts-talitos supports only two key lengths - 256
and 512 bits. There are tcrypt speed tests that check also for 384-bit
keys (which is out-of-spec, but still...), leading to a "Key Size Error"
- see below (KSE bit in AESU Interrupt Status Register is set)

testing speed of async xts(aes) (xts-aes-talitos) encryption
[...]
test 5 (384 bit key, 16 byte blocks):
talitos ffe30000.crypto: CDPR is NULL, giving up search for offending
descriptor
talitos ffe30000.crypto: AESUISR 0x00000000_00000200
talitos ffe30000.crypto: DESCBUF 0x64300011_00000000
talitos ffe30000.crypto: DESCBUF 0x00000000_00000000
talitos ffe30000.crypto: DESCBUF 0x00100000_00000000
talitos ffe30000.crypto: DESCBUF 0x00300000_00000000
talitos ffe30000.crypto: DESCBUF 0x00100000_00000000
talitos ffe30000.crypto: DESCBUF 0x00100000_00000000
talitos ffe30000.crypto: DESCBUF 0x00100000_00000000
talitos ffe30000.crypto: DESCBUF 0x00000000_00000000
encryption() failed flags=0

So for xts, driver must enforce 256/512 bit key lengths and return
CRYPTO_TFM_RES_BAD_KEY_LEN in all other cases.
Or a SW fallback could be used for the other cases, but I don't think
it's worth the effort since these are non-standard.

Horia

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

Hi Horia,

On Wed, Mar 11, 2015 at 11:48 AM, Horia Geant=C4=83
<horia.geanta@freescale.com> wrote:
>
> While here: note that xts-talitos supports only two key lengths - 256
> and 512 bits. There are tcrypt speed tests that check also for 384-bit
> keys (which is out-of-spec, but still...), leading to a "Key Size Error"
> - see below (KSE bit in AESU Interrupt Status Register is set)

Ok.  I've limited the keysize to 32 or 64 bytes for AES-XTS in the
talitos driver.

This was my first experiments with the tcrypt module.  It also brought
up another issue related to the IV limitations of this hardware.  The
latest patch that I have returns an error when there is a non-zero
value in the second 8 bytes of the IV:

+       /*
+        * AES-XTS uses the first two AES Context registers for:
+        *
+        *     Register 1:   Sector Number (Little Endian)
+        *     Register 2:   Sector Size   (Big Endian)
+        *
+        * Whereas AES-CBC uses registers 1/2 as a 16-byte IV.
+        */
+       if ((ctx->desc_hdr_template &
+            (DESC_HDR_SEL0_MASK | DESC_HDR_MODE0_MASK)) =3D=3D
+            (DESC_HDR_SEL0_AESU | DESC_HDR_MODE0_AESU_XTS)) {
+               u64 *aesctx2 =3D (u64 *)areq->info + 1;
+
+               if (*aesctx2 !=3D 0) {
+                       dev_err(ctx->dev,
+                               "IV length limited to the first 8 bytes.");
+                       return ERR_PTR(-EINVAL);
+               }
+
+               /* Fixed sized sector */
+               *aesctx2 =3D cpu_to_be64(1 << SECTOR_SHIFT);
+       }


This approach causes the tcrypt tests to fail because tcrypt sets all
16 bytes of the IV to 0xff.  I think returning an error is the right
approach for the talitos module, but it would be nice if tcrypt still
worked.  Should tcrypt just set the IV bytes to 0 instead of 0xff?
Isn't one IV just as good as another?  I think adding exceptions to
the tcrypt code would be ugly, but maybe one should be made for XTS
since the standard dictates that the IV should be plain or plain64?

Thanks,
mh

--=20
Martin Hicks P.Eng.      |         mort@bork.org
Bork Consulting Inc.     |   +1 (613) 266-2296

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Horia Geantă]

On 3/13/2015 4:08 PM, Martin Hicks wrote:
> Hi Horia,
> 
> On Wed, Mar 11, 2015 at 11:48 AM, Horia Geantă
> <horia.geanta@freescale.com> wrote:
>>
>> While here: note that xts-talitos supports only two key lengths - 256
>> and 512 bits. There are tcrypt speed tests that check also for 384-bit
>> keys (which is out-of-spec, but still...), leading to a "Key Size Error"
>> - see below (KSE bit in AESU Interrupt Status Register is set)
> 
> Ok.  I've limited the keysize to 32 or 64 bytes for AES-XTS in the
> talitos driver.
> 
> This was my first experiments with the tcrypt module.  It also brought
> up another issue related to the IV limitations of this hardware.  The
> latest patch that I have returns an error when there is a non-zero
> value in the second 8 bytes of the IV:
> 
> +       /*
> +        * AES-XTS uses the first two AES Context registers for:
> +        *
> +        *     Register 1:   Sector Number (Little Endian)
> +        *     Register 2:   Sector Size   (Big Endian)
> +        *
> +        * Whereas AES-CBC uses registers 1/2 as a 16-byte IV.
> +        */
> +       if ((ctx->desc_hdr_template &
> +            (DESC_HDR_SEL0_MASK | DESC_HDR_MODE0_MASK)) ==
> +            (DESC_HDR_SEL0_AESU | DESC_HDR_MODE0_AESU_XTS)) {
> +               u64 *aesctx2 = (u64 *)areq->info + 1;
> +
> +               if (*aesctx2 != 0) {
> +                       dev_err(ctx->dev,
> +                               "IV length limited to the first 8 bytes.");
> +                       return ERR_PTR(-EINVAL);
> +               }
> +
> +               /* Fixed sized sector */
> +               *aesctx2 = cpu_to_be64(1 << SECTOR_SHIFT);
> +       }
> 
> 
> This approach causes the tcrypt tests to fail because tcrypt sets all
> 16 bytes of the IV to 0xff.  I think returning an error is the right
> approach for the talitos module, but it would be nice if tcrypt still
> worked.  Should tcrypt just set the IV bytes to 0 instead of 0xff?
> Isn't one IV just as good as another?  I think adding exceptions to
> the tcrypt code would be ugly, but maybe one should be made for XTS
> since the standard dictates that the IV should be plain or plain64?

AFAICT xts-aes standard does not mandate for plain or plain64.
The requirements are the following (below IV = tweak value, sector =
data unit):
-IV size: 16 bytes
-IV format: little endian byte array
-IV values: non-negative; consecutive IV values for consecutive sectors

In practice, an 8-byte IV should be enough to represent the sector index
even for large capacity storage devices.
However, dm-crypt has support for a user-provided iv_offset that is
added to the sector index: IV = sector_index + iv_offset.
While in most of the cases user would choose iv_offset = 0, in theory
anything is possible.

IMHO the correct approach would be to use a fallback tfm that would
handle all the requests with IVs > 8 bytes.
We can take this off-list if you prefer.

Horia

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

On Mon, Mar 02, 2015 at 03:25:56PM +0200, Horia Geantă wrote:
> On 2/20/2015 7:00 PM, Martin Hicks wrote:
> > This adds the AES-XTS mode, supported by the Freescale SEC 3.3.2.
> > 
> > One of the nice things about this hardware is that it knows how to deal
> > with encrypt/decrypt requests that are larger than sector size, but that 
> > also requires that that the sector size be passed into the crypto engine
> > as an XTS cipher context parameter.
> > 
> > When a request is larger than the sector size the sector number is
> > incremented by the talitos engine and the tweak key is re-calculated
> > for the new sector.
> > 
> > I've tested this with 256bit and 512bit keys (tweak and data keys of 128bit
> > and 256bit) to ensure interoperability with the software AES-XTS
> > implementation.  All testing was done using dm-crypt/LUKS with
> > aes-xts-plain64.
> > 
> > Is there a better solution that just hard coding the sector size to
> > (1<<SECTOR_SHIFT)?  Maybe dm-crypt should be modified to pass the
> > sector size along with the plain/plain64 IV to an XTS algorithm?
> 
> AFAICT, SW implementation of xts mode in kernel (crypto/xts.c) is not
> aware of a sector size ("data unit size" in IEEE P1619 terminology):
> There's a hidden assumption that all the data send to xts in one request
> belongs to a single sector. Even more, it's supposed that the first
> 16-byte block in the request is "block 0" in the sector. These can be
> seen from the way the tweak ("T") value is computed.
> (Side note: there's no support of ciphertext stealing in crypto/xts.c -
> i.e. sector sizes must be a multiple of underlying block cipher size -
> that is 16B.)
> 
> If dm-crypt would be modified to pass sector size somehow, all in-kernel
> xts implementations would have to be made aware of the change.
> I have nothing against this, but let's see what crypto maintainers have
> to say...

Right.  Additionally, there may be some requirement for the encryption
implementation to broadcast the maximum size that can be handled in a single
request.  For example Talitos could handle XTS encrypt/decrypt requests of
up to 64kB (regardless of the block device's sector size).

> BTW, there were some discussions back in 2013 wrt. being able to
> configure / increase sector size, smth. crypto engines would benefit from:
> http://www.saout.de/pipermail/dm-crypt/2013-January/003125.html
> (experimental patch)
> http://www.saout.de/pipermail/dm-crypt/2013-March/003202.html
> 
> The experimental patch sends sector size as the req->nbytes - hidden
> assumption: data size sent in an xts crypto request equals a sector.

I found this last week, and used it as a starting point for some testing.  I
modified it to keep the underlying sector size of the dm-crypt mapping as
512byte, but allowed the code to combine requests in IOs up to 4kB.  Doing
greater request sizes would require allocating additional pages...I plan to
implement that to see how much extra performance can be squeezed out.

patch below...

With regards to performance, with my low-powered Freescale P1022 board, I see
performance numbers like this on ext4, as measured by bonnie++.

			Write (MB/s)	Read (MB/s)
Unencrypted		140		176
aes-xts-plain64 512b	113		115
aes-xts-plain64 4kB	71		56

The more detailed bonnie++ output is here:
http://www.bork.org/~mort/dm-crypt-enc-blksize.html

The larger IO sizes is a huge win for this board.

The patch I'm using to send IOs up to 4kB to talitos follows.

Thanks,
mh


diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 08981be..88e95b5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -42,6 +42,7 @@ struct convert_context {
 	struct bvec_iter iter_out;
 	sector_t cc_sector;
 	atomic_t cc_pending;
+	unsigned int block_size;
 	struct ablkcipher_request *req;
 };
 
@@ -142,6 +143,8 @@ struct crypt_config {
 	sector_t iv_offset;
 	unsigned int iv_size;
 
+	unsigned int block_size;
+
 	/* ESSIV: struct crypto_cipher *essiv_tfm */
 	void *iv_private;
 	struct crypto_ablkcipher **tfms;
@@ -801,10 +804,17 @@ static void crypt_convert_init(struct crypt_config *cc,
 {
 	ctx->bio_in = bio_in;
 	ctx->bio_out = bio_out;
-	if (bio_in)
+	ctx->block_size = 0;
+	if (bio_in) {
 		ctx->iter_in = bio_in->bi_iter;
-	if (bio_out)
+		ctx->block_size = max(ctx->block_size, bio_cur_bytes(bio_in));
+	}
+	if (bio_out) {
 		ctx->iter_out = bio_out->bi_iter;
+		ctx->block_size = max(ctx->block_size, bio_cur_bytes(bio_out));
+	}
+	if (ctx->block_size > cc->block_size)
+		ctx->block_size = cc->block_size;
 	ctx->cc_sector = sector + cc->iv_offset;
 	init_completion(&ctx->restart);
 }
@@ -844,15 +854,15 @@ static int crypt_convert_block(struct crypt_config *cc,
 	dmreq->iv_sector = ctx->cc_sector;
 	dmreq->ctx = ctx;
 	sg_init_table(&dmreq->sg_in, 1);
-	sg_set_page(&dmreq->sg_in, bv_in.bv_page, 1 << SECTOR_SHIFT,
+	sg_set_page(&dmreq->sg_in, bv_in.bv_page, ctx->block_size,
 		    bv_in.bv_offset);
 
 	sg_init_table(&dmreq->sg_out, 1);
-	sg_set_page(&dmreq->sg_out, bv_out.bv_page, 1 << SECTOR_SHIFT,
+	sg_set_page(&dmreq->sg_out, bv_out.bv_page, ctx->block_size,
 		    bv_out.bv_offset);
 
-	bio_advance_iter(ctx->bio_in, &ctx->iter_in, 1 << SECTOR_SHIFT);
-	bio_advance_iter(ctx->bio_out, &ctx->iter_out, 1 << SECTOR_SHIFT);
+	bio_advance_iter(ctx->bio_in, &ctx->iter_in, ctx->block_size);
+	bio_advance_iter(ctx->bio_out, &ctx->iter_out, ctx->block_size);
 
 	if (cc->iv_gen_ops) {
 		r = cc->iv_gen_ops->generator(cc, iv, dmreq);
@@ -861,7 +871,7 @@ static int crypt_convert_block(struct crypt_config *cc,
 	}
 
 	ablkcipher_request_set_crypt(req, &dmreq->sg_in, &dmreq->sg_out,
-				     1 << SECTOR_SHIFT, iv);
+				     ctx->block_size, iv);
 
 	if (bio_data_dir(ctx->bio_in) == WRITE)
 		r = crypto_ablkcipher_encrypt(req);
@@ -926,13 +936,13 @@ static int crypt_convert(struct crypt_config *cc,
 			/* fall through*/
 		case -EINPROGRESS:
 			ctx->req = NULL;
-			ctx->cc_sector++;
+			ctx->cc_sector += ctx->block_size / (1<<SECTOR_SHIFT);
 			continue;
 
 		/* sync */
 		case 0:
 			atomic_dec(&ctx->cc_pending);
-			ctx->cc_sector++;
+			ctx->cc_sector += ctx->block_size / (1<<SECTOR_SHIFT);
 			cond_resched();
 			continue;
 
@@ -1814,6 +1824,9 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		goto bad;
 	}
 
+	/* PAGE_SIZE? */
+	cc->block_size = 4096;
+
 	ti->num_flush_bios = 1;
 	ti->discard_zeroes_data_unsupported = true;
 
@@ -1974,9 +1987,16 @@ static int crypt_iterate_devices(struct dm_target *ti,
 	return fn(ti, cc->dev, cc->start, ti->len, data);
 }
 
+static void crypt_io_hints(struct dm_target *ti,
+                           struct queue_limits *limits)
+{
+	/* PAGE_SIZE? */
+	blk_limits_io_min(limits, 4096);
+}
+
 static struct target_type crypt_target = {
 	.name   = "crypt",
-	.version = {1, 13, 0},
+	.version = {1, 14, 0},
 	.module = THIS_MODULE,
 	.ctr    = crypt_ctr,
 	.dtr    = crypt_dtr,
@@ -1988,6 +2008,7 @@ static struct target_type crypt_target = {
 	.message = crypt_message,
 	.merge  = crypt_merge,
 	.iterate_devices = crypt_iterate_devices,
+	.io_hints = crypt_io_hints,
 };
 
 static int __init dm_crypt_init(void)

-- 
Martin Hicks P.Eng.    |      mort@bork.org
Bork Consulting Inc.   |  +1 (613) 266-2296

Re: [PATCH 0/2] crypto: talitos: Add AES-XTS mode

[Martin Hicks]

On Mon, Mar 02, 2015 at 04:44:19PM -0500, Martin Hicks wrote:
> 
> 			Write (MB/s)	Read (MB/s)
> Unencrypted		140		176
> aes-xts-plain64 512b	113		115
> aes-xts-plain64 4kB	71		56

I got the two AES lines backwards.  Sorry about that.

mh

-- 
Martin Hicks P.Eng.    |      mort@bork.org
Bork Consulting Inc.   |  +1 (613) 266-2296