[PATCH RESEND] crypto: make sure *blkcipher_walk_init properly initialises walk

[PATCH RESEND] crypto: make sure *blkcipher_walk_init properly initialises walk

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

blkcipher_walk_init and ablkcipher_walk_init functions are called
to initialise a walk structure allocated on stack, which is not
initialised by the caller.  This means, that the fields of the
structure contain garbage when *_init is run.

The *_init functions do not initialise all of the fields though,
and in particular leave flags field as is.  This results in field
containing unspecified value.

Zeroing the whole structure makes sure that all of the fields
are initialised to the same value regardless of the values stored
on the stack prior to the call to the *_init function.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 crypto/blkcipher.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/blkcipher.c b/crypto/blkcipher.c
index a79e7e9..3fb99d8 100644
--- a/crypto/blkcipher.c
+++ b/crypto/blkcipher.c
@@ -305,7 +305,7 @@ static inline int blkcipher_copy_iv(struct blkcipher_walk *walk,
 int blkcipher_walk_virt(struct blkcipher_desc *desc,
 			struct blkcipher_walk *walk)
 {
-	walk->flags &= ~BLKCIPHER_WALK_PHYS;
+	walk->flags = 0;
 	walk->blocksize = crypto_blkcipher_blocksize(desc->tfm);
 	return blkcipher_walk_first(desc, walk);
 }
@@ -314,7 +314,7 @@ EXPORT_SYMBOL_GPL(blkcipher_walk_virt);
 int blkcipher_walk_phys(struct blkcipher_desc *desc,
 			struct blkcipher_walk *walk)
 {
-	walk->flags |= BLKCIPHER_WALK_PHYS;
+	walk->flags = BLKCIPHER_WALK_PHYS;
 	walk->blocksize = crypto_blkcipher_blocksize(desc->tfm);
 	return blkcipher_walk_first(desc, walk);
 }
@@ -352,7 +352,7 @@ int blkcipher_walk_virt_block(struct blkcipher_desc *desc,
 			      struct blkcipher_walk *walk,
 			      unsigned int blocksize)
 {
-	walk->flags &= ~BLKCIPHER_WALK_PHYS;
+	walk->flags = 0;
 	walk->blocksize = blocksize;
 	return blkcipher_walk_first(desc, walk);
 }
-- 
1.8.3.2

Re: [PATCH RESEND] crypto: make sure *blkcipher_walk_init properly initialises walk

[Herbert Xu]

On Sun, Nov 10, 2013 at 07:38:01PM +0100, Michal Nazarewicz wrote:
> From: Michal Nazarewicz <mina86@mina86.com>
> 
> blkcipher_walk_init and ablkcipher_walk_init functions are called
> to initialise a walk structure allocated on stack, which is not
> initialised by the caller.  This means, that the fields of the
> structure contain garbage when *_init is run.
> 
> The *_init functions do not initialise all of the fields though,
> and in particular leave flags field as is.  This results in field
> containing unspecified value.
> 
> Zeroing the whole structure makes sure that all of the fields
> are initialised to the same value regardless of the values stored
> on the stack prior to the call to the *_init function.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Nack.  The field flags is used as a bit-field and all bits other
than those initialised that you see are used internally by the
walker function and will be initialised on demand.

Please do not just rely on tools such as coverity and actually
read the code when submitting patches.

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

Re: [PATCH RESEND] crypto: make sure *blkcipher_walk_init properly initialises walk

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Mon, Nov 11 2013, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> Nack.  The field flags is used as a bit-field and all bits other
> than those initialised that you see are used internally by the
> walker function and will be initialised on demand.
>
> Please do not just rely on tools such as coverity and actually
> read the code when submitting patches.

I have read the code which is why I concluded that it is safe to replace
the bit operations with a simple assignment.  Since, as you described,
all other bit fields are initialised on demand anyway, there is no harm
in setting them to zero here.  Especially since I see no advantages of
the current approach, but instead see two disadvantages: longer machine
code (load-modiy-store vs. store) and confusion of tools such as
Coverity.

But of course, if you want it as it is, I won't be bothering you.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]