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 <[email protected]>
---
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
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))
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
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
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
On Mon, Oct 10, 2016 at 10:15:15AM -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).
>
> Signed-off-by: Eric Biggers <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt