[PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Vakul Garg]

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 <vakul@freescale.com>
---
 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

RE: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Ruchika Gupta]

Reviewed-by: Ruchika Gupta <ruchika.gupta@freescale.com>

> -----Original Message-----
> From: Vakul Garg [mailto:vakul@freescale.com]
> Sent: Sunday, April 27, 2014 8:56 PM
> To: linux-crypto@vger.kernel.org
> Cc: herbert@gondor.apana.org.au; 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 <vakul@freescale.com>
> ---
>  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

Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Kim Phillips]

On Sun, 27 Apr 2014 11:26:14 -0400
Vakul Garg <vakul@freescale.com> 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

RE: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Vakul Garg]

> -----Original Message-----
> From: Kim Phillips [mailto:kim.phillips@freescale.com]
> Sent: Saturday, May 03, 2014 5:40 AM
> To: Garg Vakul-B16394
> Cc: linux-crypto@vger.kernel.org; herbert@gondor.apana.org.au; 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 <vakul@freescale.com> 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

Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Kim Phillips]

On Sat, 3 May 2014 06:44:39 -0500
Garg Vakul-B16394 <vakul@freescale.com> wrote:

> > From: Kim Phillips [mailto:kim.phillips@freescale.com]
> > Sent: Saturday, May 03, 2014 5:40 AM
> > 
> > On Sun, 27 Apr 2014 11:26:14 -0400
> > Vakul Garg <vakul@freescale.com> 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

RE: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Vakul Garg]

Hi Kim

> -----Original Message-----
> From: Kim Phillips [mailto:kim.phillips@freescale.com]
> Sent: Tuesday, May 06, 2014 12:07 AM
> To: Garg Vakul-B16394
> Cc: Phillips Kim-R1AAHA; linux-crypto@vger.kernel.org;
> herbert@gondor.apana.org.au; 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 <vakul@freescale.com> wrote:
> 
> > > From: Kim Phillips [mailto:kim.phillips@freescale.com]
> > > Sent: Saturday, May 03, 2014 5:40 AM
> > >
> > > On Sun, 27 Apr 2014 11:26:14 -0400
> > > Vakul Garg <vakul@freescale.com> 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

Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Kim Phillips]

On Mon, 5 May 2014 22:39:09 -0500
Garg Vakul-B16394 <vakul@freescale.com> wrote:

> Hi Kim

Hi Vakul,

> > From: Kim Phillips [mailto:kim.phillips@freescale.com]
> > Sent: Tuesday, May 06, 2014 12:07 AM
> > 
> > On Sat, 3 May 2014 06:44:39 -0500
> > Garg Vakul-B16394 <vakul@freescale.com> wrote:
> > 
> > > > From: Kim Phillips [mailto:kim.phillips@freescale.com]
> > > > Sent: Saturday, May 03, 2014 5:40 AM
> > > >
> > > > On Sun, 27 Apr 2014 11:26:14 -0400
> > > > Vakul Garg <vakul@freescale.com> 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

Re: [PATCH] crypto: caam - Fix key inlining in AEAD shared descriptors

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] crypto: caam - reinitialize keys_fit_inline for decrypt and givencrypt

[Kim Phillips]

From: Vakul Garg <vakul@freescale.com>

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 <vakul@freescale.com>
[reworded commit text, enhanced code readability]
Signed-off-by: Kim Phillips <kim.phillips@freescale.com>
---
 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

Re: [PATCH] crypto: caam - reinitialize keys_fit_inline for decrypt and givencrypt

[Herbert Xu]

On Fri, May 09, 2014 at 08:34:40PM -0500, Kim Phillips wrote:
> From: Vakul Garg <vakul@freescale.com>
> 
> 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 <vakul@freescale.com>
> [reworded commit text, enhanced code readability]
> Signed-off-by: Kim Phillips <kim.phillips@freescale.com>

Patch applied.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt