The variable 'keys_fit_inline' is initialised correctly to avoid using
its stale value while creating shared descriptor for decryption and
given-iv-encryption.
Signed-off-by: Vakul Garg <[email protected]>
---
drivers/crypto/caam/caamalg.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 5f89125..99fda94 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -209,7 +209,7 @@ static int aead_null_set_sh_desc(struct crypto_aead *aead)
struct aead_tfm *tfm = &aead->base.crt_aead;
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
- bool keys_fit_inline = false;
+ bool keys_fit_inline;
u32 *key_jump_cmd, *jump_cmd, *read_move_cmd, *write_move_cmd;
u32 *desc;
@@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct crypto_aead *aead)
if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
+ else
+ keys_fit_inline = false;
/* aead_encrypt shared descriptor */
desc = ctx->sh_desc_enc;
@@ -306,6 +308,8 @@ static int aead_null_set_sh_desc(struct crypto_aead *aead)
if (DESC_AEAD_NULL_DEC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
+ else
+ keys_fit_inline = false;
desc = ctx->sh_desc_dec;
@@ -399,7 +403,7 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
struct aead_tfm *tfm = &aead->base.crt_aead;
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
- bool keys_fit_inline = false;
+ bool keys_fit_inline;
u32 geniv, moveiv;
u32 *desc;
@@ -418,6 +422,9 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
ctx->split_key_pad_len + ctx->enckeylen <=
CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
+ else
+ keys_fit_inline = false;
+
/* aead_encrypt shared descriptor */
desc = ctx->sh_desc_enc;
@@ -476,6 +483,8 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
ctx->split_key_pad_len + ctx->enckeylen <=
CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
+ else
+ keys_fit_inline = false;
/* aead_decrypt shared descriptor */
desc = ctx->sh_desc_dec;
@@ -531,6 +540,8 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
ctx->split_key_pad_len + ctx->enckeylen <=
CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
+ else
+ keys_fit_inline = false;
/* aead_givencrypt shared descriptor */
desc = ctx->sh_desc_givenc;
--
1.8.1.4
Reviewed-by: Ruchika Gupta <[email protected]>
> -----Original Message-----
> From: Vakul Garg [mailto:[email protected]]
> Sent: Sunday, April 27, 2014 8:56 PM
> To: [email protected]
> Cc: [email protected]; Geanta Neag Horia Ioan-B05471; Gupta
> Ruchika-R66431; Porosanu Alexandru-B06830
> Subject: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors
>
> The variable 'keys_fit_inline' is initialised correctly to avoid using its
> stale value while creating shared descriptor for decryption and given-iv-
> encryption.
>
> Signed-off-by: Vakul Garg <[email protected]>
> ---
> drivers/crypto/caam/caamalg.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 5f89125..99fda94 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -209,7 +209,7 @@ static int aead_null_set_sh_desc(struct crypto_aead
> *aead)
> struct aead_tfm *tfm = &aead->base.crt_aead;
> struct caam_ctx *ctx = crypto_aead_ctx(aead);
> struct device *jrdev = ctx->jrdev;
> - bool keys_fit_inline = false;
> + bool keys_fit_inline;
> u32 *key_jump_cmd, *jump_cmd, *read_move_cmd, *write_move_cmd;
> u32 *desc;
>
> @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct crypto_aead
> *aead)
> if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
>
> /* aead_encrypt shared descriptor */
> desc = ctx->sh_desc_enc;
> @@ -306,6 +308,8 @@ static int aead_null_set_sh_desc(struct crypto_aead
> *aead)
> if (DESC_AEAD_NULL_DEC_LEN + DESC_JOB_IO_LEN +
> ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
>
> desc = ctx->sh_desc_dec;
>
> @@ -399,7 +403,7 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
> struct aead_tfm *tfm = &aead->base.crt_aead;
> struct caam_ctx *ctx = crypto_aead_ctx(aead);
> struct device *jrdev = ctx->jrdev;
> - bool keys_fit_inline = false;
> + bool keys_fit_inline;
> u32 geniv, moveiv;
> u32 *desc;
>
> @@ -418,6 +422,9 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
> ctx->split_key_pad_len + ctx->enckeylen <=
> CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
> +
>
> /* aead_encrypt shared descriptor */
> desc = ctx->sh_desc_enc;
> @@ -476,6 +483,8 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
> ctx->split_key_pad_len + ctx->enckeylen <=
> CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
>
> /* aead_decrypt shared descriptor */
> desc = ctx->sh_desc_dec;
> @@ -531,6 +540,8 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
> ctx->split_key_pad_len + ctx->enckeylen <=
> CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
>
> /* aead_givencrypt shared descriptor */
> desc = ctx->sh_desc_givenc;
> --
> 1.8.1.4
On Sun, 27 Apr 2014 11:26:14 -0400
Vakul Garg <[email protected]> wrote:
> The variable 'keys_fit_inline' is initialised correctly to avoid using
> its stale value while creating shared descriptor for decryption and
> given-iv-encryption.
you mean givencrypt? That's "generate an IV and encrypt".
> @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct crypto_aead *aead)
> if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
> + else
> + keys_fit_inline = false;
Can we do the easier to read:
keys_fit_inline = false;
if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
?
Thanks.
Kim
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Saturday, May 03, 2014 5:40 AM
> To: Garg Vakul-B16394
> Cc: [email protected]; [email protected]; Geanta
> Neag Horia Ioan-B05471; Gupta Ruchika-R66431; Porosanu Alexandru-B06830
> Subject: Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared
> descriptors
>
> On Sun, 27 Apr 2014 11:26:14 -0400
> Vakul Garg <[email protected]> wrote:
>
> > The variable 'keys_fit_inline' is initialised correctly to avoid using
> > its stale value while creating shared descriptor for decryption and
> > given-iv-encryption.
>
> you mean givencrypt? That's "generate an IV and encrypt".
I will modify it and submit next patch version.
>
> > @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct crypto_aead
> *aead)
> > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > keys_fit_inline = true;
> > + else
> > + keys_fit_inline = false;
>
> Can we do the easier to read:
>
> keys_fit_inline = false;
> if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> keys_fit_inline = true;
>
> ?
Why pre-init a variable with default value when it could be overwritten?
I think that the form I submitted is equally easy to read.
>
> Thanks.
>
> Kim
On Sat, 3 May 2014 06:44:39 -0500
Garg Vakul-B16394 <[email protected]> wrote:
> > From: Kim Phillips [mailto:[email protected]]
> > Sent: Saturday, May 03, 2014 5:40 AM
> >
> > On Sun, 27 Apr 2014 11:26:14 -0400
> > Vakul Garg <[email protected]> wrote:
> >
> > > @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct crypto_aead
> > *aead)
> > > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > > keys_fit_inline = true;
> > > + else
> > > + keys_fit_inline = false;
> >
> > Can we do the easier to read:
> >
> > keys_fit_inline = false;
> > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > keys_fit_inline = true;
> >
> > ?
>
> Why pre-init a variable with default value when it could be overwritten?
why not? compiler output doesn't differ in this regard.
> I think that the form I submitted is equally easy to read.
adding one line instead of two -> less lines overall -> more code
on one screen -> easier to read.
Kim
Hi Kim
> -----Original Message-----
> From: Kim Phillips [mailto:[email protected]]
> Sent: Tuesday, May 06, 2014 12:07 AM
> To: Garg Vakul-B16394
> Cc: Phillips Kim-R1AAHA; [email protected];
> [email protected]; Geanta Neag Horia Ioan-B05471; Gupta
> Ruchika-R66431; Porosanu Alexandru-B06830
> Subject: Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared
> descriptors
>
> On Sat, 3 May 2014 06:44:39 -0500
> Garg Vakul-B16394 <[email protected]> wrote:
>
> > > From: Kim Phillips [mailto:[email protected]]
> > > Sent: Saturday, May 03, 2014 5:40 AM
> > >
> > > On Sun, 27 Apr 2014 11:26:14 -0400
> > > Vakul Garg <[email protected]> wrote:
> > >
> > > > @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct
> > > > crypto_aead
> > > *aead)
> > > > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > > > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > > > keys_fit_inline = true;
> > > > + else
> > > > + keys_fit_inline = false;
> > >
> > > Can we do the easier to read:
> > >
> > > keys_fit_inline = false;
> > > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > > keys_fit_inline = true;
> > >
> > > ?
> >
> > Why pre-init a variable with default value when it could be
> overwritten?
>
> why not? compiler output doesn't differ in this regard.
>
Agree that compiler output doesn't differ.
But why depend upon compiler's optimization capability while writing code when we can be explicit?
> > I think that the form I submitted is equally easy to read.
>
> adding one line instead of two -> less lines overall -> more code on one
> screen -> easier to read.
>
I think that this is a matter of personal coding choice.
Both the approaches are fine and compliant to kernel coding guidelines.
> Kim
On Mon, 5 May 2014 22:39:09 -0500
Garg Vakul-B16394 <[email protected]> wrote:
> Hi Kim
Hi Vakul,
> > From: Kim Phillips [mailto:[email protected]]
> > Sent: Tuesday, May 06, 2014 12:07 AM
> >
> > On Sat, 3 May 2014 06:44:39 -0500
> > Garg Vakul-B16394 <[email protected]> wrote:
> >
> > > > From: Kim Phillips [mailto:[email protected]]
> > > > Sent: Saturday, May 03, 2014 5:40 AM
> > > >
> > > > On Sun, 27 Apr 2014 11:26:14 -0400
> > > > Vakul Garg <[email protected]> wrote:
> > > >
> > > > > @@ -220,6 +220,8 @@ static int aead_null_set_sh_desc(struct
> > > > > crypto_aead
> > > > *aead)
> > > > > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > > > > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > > > > keys_fit_inline = true;
> > > > > + else
> > > > > + keys_fit_inline = false;
> > > >
> > > > Can we do the easier to read:
> > > >
> > > > keys_fit_inline = false;
> > > > if (DESC_AEAD_NULL_ENC_LEN + DESC_JOB_IO_LEN +
> > > > ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
> > > > keys_fit_inline = true;
> > > >
> > > > ?
> > >
> > > Why pre-init a variable with default value when it could be
> > overwritten?
> >
> > why not? compiler output doesn't differ in this regard.
>
> Agree that compiler output doesn't differ.
> But why depend upon compiler's optimization capability while writing code when we can be explicit?
? with optimizations turned off, the explicit 'else' *adds* an
extra CoF..
> > > I think that the form I submitted is equally easy to read.
> >
> > adding one line instead of two -> less lines overall -> more code on one
> > screen -> easier to read.
>
> I think that this is a matter of personal coding choice.
> Both the approaches are fine and compliant to kernel coding guidelines.
I disagree, for the reasons already mentioned above.
Kim
On Tue, May 06, 2014 at 03:11:14PM -0500, Kim Phillips wrote:
>
> > > > I think that the form I submitted is equally easy to read.
> > >
> > > adding one line instead of two -> less lines overall -> more code on one
> > > screen -> easier to read.
> >
> > I think that this is a matter of personal coding choice.
> > Both the approaches are fine and compliant to kernel coding guidelines.
>
> I disagree, for the reasons already mentioned above.
I like Kim's version better. Kim, could you respin a patch in
the way you described?
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Vakul Garg <[email protected]>
Re-initialize keys_fit_inline to avoid using its stale encrypt() shared
descriptor value prior to building descriptors for the decrypt() and
givencrypt() cases.
Signed-off-by: Vakul Garg <[email protected]>
[reworded commit text, enhanced code readability]
Signed-off-by: Kim Phillips <[email protected]>
---
drivers/crypto/caam/caamalg.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 5f89125..4bf7634 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -303,6 +303,7 @@ static int aead_null_set_sh_desc(struct crypto_aead *aead)
* Job Descriptor and Shared Descriptors
* must all fit into the 64-word Descriptor h/w Buffer
*/
+ keys_fit_inline = false;
if (DESC_AEAD_NULL_DEC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len <= CAAM_DESC_BYTES_MAX)
keys_fit_inline = true;
@@ -472,6 +473,7 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
* Job Descriptor and Shared Descriptors
* must all fit into the 64-word Descriptor h/w Buffer
*/
+ keys_fit_inline = false;
if (DESC_AEAD_DEC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len + ctx->enckeylen <=
CAAM_DESC_BYTES_MAX)
@@ -527,6 +529,7 @@ static int aead_set_sh_desc(struct crypto_aead *aead)
* Job Descriptor and Shared Descriptors
* must all fit into the 64-word Descriptor h/w Buffer
*/
+ keys_fit_inline = false;
if (DESC_AEAD_GIVENC_LEN + DESC_JOB_IO_LEN +
ctx->split_key_pad_len + ctx->enckeylen <=
CAAM_DESC_BYTES_MAX)
--
1.9.2
On Fri, May 09, 2014 at 08:34:40PM -0500, Kim Phillips wrote:
> From: Vakul Garg <[email protected]>
>
> Re-initialize keys_fit_inline to avoid using its stale encrypt() shared
> descriptor value prior to building descriptors for the decrypt() and
> givencrypt() cases.
>
> Signed-off-by: Vakul Garg <[email protected]>
> [reworded commit text, enhanced code readability]
> Signed-off-by: Kim Phillips <[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