Re: [PATCH 0/6] crypto: arm64 - big endian fixes

[Herbert Xu]

On Mon, Oct 10, 2016 at 12:26:00PM +0100, Ard Biesheuvel wrote:
> 
> /* This piece of crap needs to disappear into per-type test hooks. */
> if (!((type ^ CRYPTO_ALG_TYPE_BLKCIPHER) &
>  CRYPTO_ALG_TYPE_BLKCIPHER_MASK) && !(type & CRYPTO_ALG_GENIV) &&
>((alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
> CRYPTO_ALG_TYPE_BLKCIPHER ? alg->cra_blkcipher.ivsize :
> alg->cra_ablkcipher.ivsize))
> type |= CRYPTO_ALG_TESTED;
> 
> This causes cbc(aes), ctr(aes) and xts(aes) to remain untested, unless
> I add CRYPTO_ALG_GENIV to their cra_flags. Is this expected behavior?
> What would be your recommended way to ensure these algos are covered
> by the boottime tests?

This is a leftover from the old blkcipher/ablkcipher interface.
I've got a patch pending which will remove this if clause.

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: Observed a ecryptFS crash

[Tyler Hicks]

On 09/29/2016 07:29 AM, liushuoran wrote:
> Hi Tyhicks,
> 
> We observed a ecryptFS crash occasionally in Linux kernel 4.1.18. The call 
> trace is attached below. Is it a known issue？ Look forward to hearing from 
> you. Thanks in advance!

It isn't known to me but I'm rarely testing eCryptfs with SELinux these
days. I don't recall any bug reports with a similar call trace, either.

You left out some potentially useful information below the "--[ cut here
]--" line and above the "Call trace:" line.

Tyler

> 
> [19314.529479s][pid:2694,cpu3,GAC_Executor[0]]Call trace:
> [19314.529510s][pid:2694,cpu3,GAC_Executor[0]][] 
> do_raw_spin_lock+0x20/0x200
> [19314.529510s][pid:2694,cpu3,GAC_Executor[0]][] 
> _raw_spin_lock+0x28/0x34
> [19314.529541s][pid:2694,cpu3,GAC_Executor[0]][] 
> selinux_inode_free_security+0x3c/0x94
> [19314.529541s][pid:2694,cpu3,GAC_Executor[0]][] 
> security_inode_free+0x2c/0x38
> [19314.529541s][pid:2694,cpu3,GAC_Executor[0]][] 
> __destroy_inode+0x2c/0x180
> [19314.529571s][pid:2694,cpu3,GAC_Executor[0]][] 
> destroy_inode+0x30/0xa0
> [19314.529571s][pid:2694,cpu3,GAC_Executor[0]][] 
> evict+0x108/0x1c0
> [19314.529571s][pid:2694,cpu3,GAC_Executor[0]][] 
> iput+0x184/0x258
> [19314.529602s][pid:2694,cpu3,GAC_Executor[0]][] 
> ecryptfs_evict_inode+0x30/0x3c
> [19314.529602s][pid:2694,cpu3,GAC_Executor[0]][] 
> evict+0xac/0x1c0
> [19314.529602s][pid:2694,cpu3,GAC_Executor[0]][] 
> dispose_list+0x44/0x5c
> [19314.529632s][pid:2694,cpu3,GAC_Executor[0]][] 
> evict_inodes+0xcc/0x12c
> [19314.529632s][pid:2694,cpu3,GAC_Executor[0]][] 
> generic_shutdown_super+0x58/0xe4
> [19314.529632s][pid:2694,cpu3,GAC_Executor[0]][] 
> kill_anon_super+0x30/0x74
> [19314.529663s][pid:2694,cpu3,GAC_Executor[0]][] 
> ecryptfs_kill_block_super+0x24/0x54
> [19314.529663s][pid:2694,cpu3,GAC_Executor[0]][] 
> deactivate_locked_super+0x60/0x8c
> [19314.529663s][pid:2694,cpu3,GAC_Executor[0]][] 
> deactivate_super+0x98/0xa4
> [19314.529693s][pid:2694,cpu3,GAC_Executor[0]][] 
> cleanup_mnt+0x50/0xd0
> [19314.529693s][pid:2694,cpu3,GAC_Executor[0]][] 
> __cleanup_mnt+0x20/0x2c
> [19314.529693s][pid:2694,cpu3,GAC_Executor[0]][] 
> task_work_run+0xbc/0xf8
> [19314.529724s][pid:2694,cpu3,GAC_Executor[0]][] 
> do_exit+0x2d4/0xa14
> [19314.529724s][pid:2694,cpu3,GAC_Executor[0]][] 
> do_group_exit+0x60/0xf8
> [19314.529724s][pid:2694,cpu3,GAC_Executor[0]][] 
> get_signal+0x284/0x598
> [19314.529754s][pid:2694,cpu3,GAC_Executor[0]][] 
> do_signal+0x170/0x5b8
> [19314.529754s][pid:2694,cpu3,GAC_Executor[0]][] 
> do_notify_resume+0x70/0x78
> [19314.529785s][pid:2694,cpu3,GAC_Executor[0]]Code: aa0003f3 aa1e03e0 
> 97fe7718 5289d5a0 (b9400661) 
> [19314.529907s][pid:2694,cpu3,GAC_Executor[0]]---[ end trace 382e4b6264b035b5 
> ]---
> [19314.529907s][pid:2694,cpu3,GAC_Executor[0]]Kernel panic - not syncing: 
> Fatal exception
> 
> Regards,
> Shuoran
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] crypto: cmac - fix alignment of 'consts'

[Eric Biggers]

On Mon, Oct 10, 2016 at 10:51:14AM -0700, Joe Perches wrote:
> 
> Hey Eric.
> 
> I don't see any PTR_ALIGN uses in crypto/ or drivers/crypto/ that
> use a bitwise or, just mask + 1, but I believe the effect is the
> same.  Anyway, your choice, but I think using min is clearer.
> 
> cheers, Joe

Usually the bitwise OR is used when setting cra_alignmask in the 'struct
crypto_alg'.  Indeed, the problem could be solved by setting

inst->alg.base.cra_alignmask = alg->cra_alignmask | 
(__alignof__(__be64) - 1);

I decided against that because it would always force 8-byte alignment for CMAC
input buffers and keys, when in fact they don't need that level of alignment
unless the underlying block cipher requires it.

Eric
--
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] crypto: cmac - fix alignment of 'consts'

[Joe Perches]

On Mon, 2016-10-10 at 10:37 -0700, Eric Biggers wrote:
> On Mon, Oct 10, 2016 at 10:29:55AM -0700, Joe Perches wrote:
> > On Mon, 2016-10-10 at 10:15 -0700, Eric Biggers wrote:
> > > The per-transform 'consts' array is accessed as __be64 in
> > > crypto_cmac_digest_setkey() but was only guaranteed to be aligned to
> > > __alignof__(long).  Fix this by aligning it to __alignof__(__be64).
> > []
> > > diff --git a/crypto/cmac.c b/crypto/cmac.c
> > []
> > > @@ -57,7 +57,8 @@ static int crypto_cmac_digest_setkey(struct 
> > > crypto_shash *parent,
> > >   unsigned long alignmask = crypto_shash_alignmask(parent);
> > >   struct cmac_tfm_ctx *ctx = crypto_shash_ctx(parent);
> > >   unsigned int bs = crypto_shash_blocksize(parent);
> > > - __be64 *consts = PTR_ALIGN((void *)ctx->ctx, alignmask + 1);
> > > + __be64 *consts = PTR_ALIGN((void *)ctx->ctx,
> > > +(alignmask | (__alignof__(__be64) - 1)) + 1);
> > 
> > 
> > Using a bitwise or looks very odd there.  Perhaps:
> > 
> > min(alignmask + 1, __alignof__(__be64))
> > 
> 
> 
> Alignment has to be a power of 2.  From the code I've read, crypto drivers 
> work
> with alignment a lot and use bitwise OR to mean "the more restrictive of these
> alignmasks".  So I believe the way it's written is the preferred style.
> 
> Eric

Hey Eric.

I don't see any PTR_ALIGN uses in crypto/ or drivers/crypto/ that
use a bitwise or, just mask + 1, but I believe the effect is the
same.  Anyway, your choice, but I think using min is clearer.

cheers, Joe
--
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] crypto: cmac - fix alignment of 'consts'

[Eric Biggers]

On Mon, Oct 10, 2016 at 10:29:55AM -0700, Joe Perches wrote:
> On Mon, 2016-10-10 at 10:15 -0700, Eric Biggers wrote:
> > The per-transform 'consts' array is accessed as __be64 in
> > crypto_cmac_digest_setkey() but was only guaranteed to be aligned to
> > __alignof__(long).  Fix this by aligning it to __alignof__(__be64).
> []
> > diff --git a/crypto/cmac.c b/crypto/cmac.c
> []
> > @@ -57,7 +57,8 @@ static int crypto_cmac_digest_setkey(struct crypto_shash 
> > *parent,
> > unsigned long alignmask = crypto_shash_alignmask(parent);
> > struct cmac_tfm_ctx *ctx = crypto_shash_ctx(parent);
> > unsigned int bs = crypto_shash_blocksize(parent);
> > -   __be64 *consts = PTR_ALIGN((void *)ctx->ctx, alignmask + 1);
> > +   __be64 *consts = PTR_ALIGN((void *)ctx->ctx,
> > +  (alignmask | (__alignof__(__be64) - 1)) + 1);
> 
> Using a bitwise or looks very odd there.  Perhaps:
> 
>   min(alignmask + 1, __alignof__(__be64))
> 

Alignment has to be a power of 2.  From the code I've read, crypto drivers work
with alignment a lot and use bitwise OR to mean "the more restrictive of these
alignmasks".  So I believe the way it's written is the preferred style.

Eric
--
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] crypto: cmac - fix alignment of 'consts'

[Joe Perches]

On Mon, 2016-10-10 at 10:15 -0700, Eric Biggers wrote:
> The per-transform 'consts' array is accessed as __be64 in
> crypto_cmac_digest_setkey() but was only guaranteed to be aligned to
> __alignof__(long).  Fix this by aligning it to __alignof__(__be64).
[]
> diff --git a/crypto/cmac.c b/crypto/cmac.c
[]
> @@ -57,7 +57,8 @@ static int crypto_cmac_digest_setkey(struct crypto_shash 
> *parent,
>   unsigned long alignmask = crypto_shash_alignmask(parent);
>   struct cmac_tfm_ctx *ctx = crypto_shash_ctx(parent);
>   unsigned int bs = crypto_shash_blocksize(parent);
> - __be64 *consts = PTR_ALIGN((void *)ctx->ctx, alignmask + 1);
> + __be64 *consts = PTR_ALIGN((void *)ctx->ctx,
> +(alignmask | (__alignof__(__be64) - 1)) + 1);

Using a bitwise or looks very odd there.  Perhaps:

min(alignmask + 1, __alignof__(__be64))

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: cmac - fix alignment of 'consts'

[Eric Biggers]

The per-transform 'consts' array is accessed as __be64 in
crypto_cmac_digest_setkey() but was only guaranteed to be aligned to
__alignof__(long).  Fix this by aligning it to __alignof__(__be64).

Signed-off-by: Eric Biggers 
---
 crypto/cmac.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/crypto/cmac.c b/crypto/cmac.c
index b6c4059..04080dc 100644
--- a/crypto/cmac.c
+++ b/crypto/cmac.c
@@ -57,7 +57,8 @@ static int crypto_cmac_digest_setkey(struct crypto_shash 
*parent,
unsigned long alignmask = crypto_shash_alignmask(parent);
struct cmac_tfm_ctx *ctx = crypto_shash_ctx(parent);
unsigned int bs = crypto_shash_blocksize(parent);
-   __be64 *consts = PTR_ALIGN((void *)ctx->ctx, alignmask + 1);
+   __be64 *consts = PTR_ALIGN((void *)ctx->ctx,
+  (alignmask | (__alignof__(__be64) - 1)) + 1);
u64 _const[2];
int i, err = 0;
u8 msb_mask, gfmask;
@@ -173,7 +174,8 @@ static int crypto_cmac_digest_final(struct shash_desc 
*pdesc, u8 *out)
struct cmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
struct crypto_cipher *tfm = tctx->child;
int bs = crypto_shash_blocksize(parent);
-   u8 *consts = PTR_ALIGN((void *)tctx->ctx, alignmask + 1);
+   u8 *consts = PTR_ALIGN((void *)tctx->ctx,
+  (alignmask | (__alignof__(__be64) - 1)) + 1);
u8 *odds = PTR_ALIGN((void *)ctx->ctx, alignmask + 1);
u8 *prev = odds + bs;
unsigned int offset = 0;
@@ -258,7 +260,8 @@ static int cmac_create(struct crypto_template *tmpl, struct 
rtattr **tb)
if (err)
goto out_free_inst;
 
-   alignmask = alg->cra_alignmask | (sizeof(long) - 1);
+   /* We access the data as u32s when xoring. */
+   alignmask = alg->cra_alignmask | (__alignof__(u32) - 1);
inst->alg.base.cra_alignmask = alignmask;
inst->alg.base.cra_priority = alg->cra_priority;
inst->alg.base.cra_blocksize = alg->cra_blocksize;
@@ -270,7 +273,9 @@ static int cmac_create(struct crypto_template *tmpl, struct 
rtattr **tb)
+ alg->cra_blocksize * 2;
 
inst->alg.base.cra_ctxsize =
-   ALIGN(sizeof(struct cmac_tfm_ctx), alignmask + 1)
+   ALIGN(sizeof(struct cmac_tfm_ctx), crypto_tfm_ctx_alignment())
+   + ((alignmask | (__alignof__(__be64) - 1)) &
+  ~(crypto_tfm_ctx_alignment() - 1))
+ alg->cra_blocksize * 2;
 
inst->alg.base.cra_init = cmac_init_tfm;
-- 
2.8.0.rc3.226.g39d4020

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: cmac - return -EINVAL if block size is unsupported

[Eric Biggers]

cmac_create() previously returned 0 if a cipher with a block size other
than 8 or 16 bytes was specified.  It should return -EINVAL instead.
Granted, this doesn't actually change any behavior because cryptomgr
currently ignores any return value other than -EAGAIN from template
->create() functions.

Signed-off-by: Eric Biggers 
---
 crypto/cmac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/cmac.c b/crypto/cmac.c
index 7a8bfbd..b6c4059 100644
--- a/crypto/cmac.c
+++ b/crypto/cmac.c
@@ -243,6 +243,7 @@ static int cmac_create(struct crypto_template *tmpl, struct 
rtattr **tb)
case 8:
break;
default:
+   err = -EINVAL;
goto out_put_alg;
}
 
-- 
2.8.0.rc3.226.g39d4020

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: ccp - change bitfield type to unsigned ints

[Gary R Hook]

Bit fields are not sensitive to endianness, so use
a transparent standard data type

Signed-off-by: Gary R Hook 
---
 drivers/crypto/ccp/ccp-dev.h |   42 +-
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h
index da5f4a6..0d996fe 100644
--- a/drivers/crypto/ccp/ccp-dev.h
+++ b/drivers/crypto/ccp/ccp-dev.h
@@ -541,23 +541,23 @@ static inline u32 ccp_addr_hi(struct ccp_dma_info *info)
  * word 7: upper 16 bits of key pointer; key memory type
  */
 struct dword0 {
-   __le32 soc:1;
-   __le32 ioc:1;
-   __le32 rsvd1:1;
-   __le32 init:1;
-   __le32 eom:1;   /* AES/SHA only */
-   __le32 function:15;
-   __le32 engine:4;
-   __le32 prot:1;
-   __le32 rsvd2:7;
+   unsigned int soc:1;
+   unsigned int ioc:1;
+   unsigned int rsvd1:1;
+   unsigned int init:1;
+   unsigned int eom:1; /* AES/SHA only */
+   unsigned int function:15;
+   unsigned int engine:4;
+   unsigned int prot:1;
+   unsigned int rsvd2:7;
 };
 
 struct dword3 {
-   __le32 src_hi:16;
-   __le32 src_mem:2;
-   __le32 lsb_cxt_id:8;
-   __le32 rsvd1:5;
-   __le32 fixed:1;
+   unsigned int  src_hi:16;
+   unsigned int  src_mem:2;
+   unsigned int  lsb_cxt_id:8;
+   unsigned int  rsvd1:5;
+   unsigned int  fixed:1;
 };
 
 union dword4 {
@@ -567,18 +567,18 @@ union dword4 {
 
 union dword5 {
struct {
-   __le32 dst_hi:16;
-   __le32 dst_mem:2;
-   __le32 rsvd1:13;
-   __le32 fixed:1;
+   unsigned int  dst_hi:16;
+   unsigned int  dst_mem:2;
+   unsigned int  rsvd1:13;
+   unsigned int  fixed:1;
} fields;
__le32 sha_len_hi;
 };
 
 struct dword7 {
-   __le32 key_hi:16;
-   __le32 key_mem:2;
-   __le32 rsvd1:14;
+   unsigned int  key_hi:16;
+   unsigned int  key_mem:2;
+   unsigned int  rsvd1:14;
 };
 
 struct ccp5_desc {

--
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 0/6] crypto: arm64 - big endian fixes

[Ard Biesheuvel]

On 9 October 2016 at 18:42, Ard Biesheuvel  wrote:
> As it turns out, none of the accelerated crypto routines under 
> arch/arm64/crypto
> currently work, or have ever worked correctly when built for big endian. So 
> this
> series fixes all of them.
>
> Each of these patches carries a fixes tag, and could be backported to stable.
> However, for patches #1 and #5, the fixes tag denotes the oldest commit that 
> the
> fix is compatible with, not the patch that introduced the algorithm. This is 
> due
> to the fact that the key schedules are incompatible between generic AES and 
> the
> arm64 Crypto Extensions implementation (but only when building for big endian)
> This is not a problem in practice, but it does mean that the AES-CCM and AES 
> in
> EBC/CBC/CTR/XTS mode implementations before v3.19 require a different fix, 
> i.e.,
> one that is compatible with the generic AES key schedule generation code 
> (which
> it currently no longer uses)
>
> In any case, please apply with cc to stable.
>

Herbert,

I have an additional fix to add to this series, and a couple for
32-bit ARM as well. They escaped my attention due to this code (in
algboss.c:250)

/* This piece of crap needs to disappear into per-type test hooks. */
if (!((type ^ CRYPTO_ALG_TYPE_BLKCIPHER) &
 CRYPTO_ALG_TYPE_BLKCIPHER_MASK) && !(type & CRYPTO_ALG_GENIV) &&
   ((alg->cra_flags & CRYPTO_ALG_TYPE_MASK) ==
CRYPTO_ALG_TYPE_BLKCIPHER ? alg->cra_blkcipher.ivsize :
alg->cra_ablkcipher.ivsize))
type |= CRYPTO_ALG_TESTED;

This causes cbc(aes), ctr(aes) and xts(aes) to remain untested, unless
I add CRYPTO_ALG_GENIV to their cra_flags. Is this expected behavior?
What would be your recommended way to ensure these algos are covered
by the boottime tests?

Thanks,
Ard.
--
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