2018-11-29 06:39:46

by David CARLIER

Subject: [PATCH] crypto: use memzero_explicit instead of memset to clear contexts.

There might be a little performance drop but to make sure it stands
by it comments, we really wipe the whole context after usage.
---
crypto/chacha20poly1305.c | 3 ++-
crypto/md5.c | 2 +-
crypto/rmd128.c | 3 ++-
crypto/rmd160.c | 3 ++-
crypto/rmd256.c | 3 ++-
crypto/rmd320.c | 3 ++-
crypto/sha3_generic.c | 3 ++-
7 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c
index 600afa99941f..6e93d998109e 100644
--- a/crypto/chacha20poly1305.c
+++ b/crypto/chacha20poly1305.c
@@ -19,6 +19,7 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/string.h>

#include "internal.h"

@@ -388,7 +389,7 @@ static int poly_genkey(struct aead_request *req)
}

sg_init_table(creq->src, 1);
- memset(rctx->key, 0, sizeof(rctx->key));
+ memzero_explicit(rctx->key, sizeof(rctx->key));
sg_set_buf(creq->src, rctx->key, sizeof(rctx->key));

chacha_iv(creq->iv, req, 0);
diff --git a/crypto/md5.c b/crypto/md5.c
index 94dd78144ba3..00d384e8784c 100644
--- a/crypto/md5.c
+++ b/crypto/md5.c
@@ -197,7 +197,7 @@ static int md5_final(struct shash_desc *desc, u8 *out)
md5_transform(mctx->hash, mctx->block);
cpu_to_le32_array(mctx->hash, sizeof(mctx->hash) / sizeof(u32));
memcpy(out, mctx->hash, sizeof(mctx->hash));
- memset(mctx, 0, sizeof(*mctx));
+ memzero_explicit(mctx, sizeof(*mctx));

return 0;
}
diff --git a/crypto/rmd128.c b/crypto/rmd128.c
index 5f4472256e27..5e01cd7130c7 100644
--- a/crypto/rmd128.c
+++ b/crypto/rmd128.c
@@ -17,6 +17,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/mm.h>
+#include <linux/string.h>
#include <linux/types.h>
#include <asm/byteorder.h>

@@ -290,7 +291,7 @@ static int rmd128_final(struct shash_desc *desc, u8 *out)
dst[i] = cpu_to_le32p(&rctx->state[i]);

/* Wipe context */
- memset(rctx, 0, sizeof(*rctx));
+ memzero_explicit(rctx, sizeof(*rctx));

return 0;
}
diff --git a/crypto/rmd160.c b/crypto/rmd160.c
index 737645344d1c..2381d134443c 100644
--- a/crypto/rmd160.c
+++ b/crypto/rmd160.c
@@ -17,6 +17,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/mm.h>
+#include <linux/string.h>
#include <linux/types.h>
#include <asm/byteorder.h>

@@ -334,7 +335,7 @@ static int rmd160_final(struct shash_desc *desc, u8 *out)
dst[i] = cpu_to_le32p(&rctx->state[i]);

/* Wipe context */
- memset(rctx, 0, sizeof(*rctx));
+ memzero_explicit(rctx, sizeof(*rctx));

return 0;
}
diff --git a/crypto/rmd256.c b/crypto/rmd256.c
index 0e9d30676a01..3db0f6653607 100644
--- a/crypto/rmd256.c
+++ b/crypto/rmd256.c
@@ -17,6 +17,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/mm.h>
+#include <linux/string.h>
#include <linux/types.h>
#include <asm/byteorder.h>

@@ -309,7 +310,7 @@ static int rmd256_final(struct shash_desc *desc, u8 *out)
dst[i] = cpu_to_le32p(&rctx->state[i]);

/* Wipe context */
- memset(rctx, 0, sizeof(*rctx));
+ memzero_explicit(rctx, sizeof(*rctx));

return 0;
}
diff --git a/crypto/rmd320.c b/crypto/rmd320.c
index 3ae1df5bb48c..e0e1a71d0144 100644
--- a/crypto/rmd320.c
+++ b/crypto/rmd320.c
@@ -17,6 +17,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/mm.h>
+#include <linux/string.h>
#include <linux/types.h>
#include <asm/byteorder.h>

@@ -358,7 +359,7 @@ static int rmd320_final(struct shash_desc *desc, u8 *out)
dst[i] = cpu_to_le32p(&rctx->state[i]);

/* Wipe context */
- memset(rctx, 0, sizeof(*rctx));
+ memzero_explicit(rctx, sizeof(*rctx));

return 0;
}
diff --git a/crypto/sha3_generic.c b/crypto/sha3_generic.c
index 7ed98367d4fb..ae30a035d371 100644
--- a/crypto/sha3_generic.c
+++ b/crypto/sha3_generic.c
@@ -17,6 +17,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/types.h>
+#include <linux/string.h>
#include <crypto/sha3.h>
#include <asm/unaligned.h>

@@ -237,7 +238,7 @@ int crypto_sha3_final(struct shash_desc *desc, u8 *out)
if (digest_size & 4)
put_unaligned_le32(sctx->st[i], (__le32 *)digest);

- memset(sctx, 0, sizeof(*sctx));
+ memzero_explicit(sctx, sizeof(*sctx));
return 0;
}
EXPORT_SYMBOL(crypto_sha3_final);

2018-11-29 17:53:34

by Herbert Xu

Subject: Re: [PATCH] crypto: use memzero_explicit instead of memset to clear contexts.

On Wed, Nov 28, 2018 at 07:36:50PM +0000, David CARLIER wrote:
> There might be a little performance drop but to make sure it stands
> by it comments, we really wipe the whole context after usage.
> ---
> crypto/chacha20poly1305.c | 3 ++-
> crypto/md5.c | 2 +-
> crypto/rmd128.c | 3 ++-
> crypto/rmd160.c | 3 ++-
> crypto/rmd256.c | 3 ++-
> crypto/rmd320.c | 3 ++-
> crypto/sha3_generic.c | 3 ++-
> 7 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c
> index 600afa99941f..6e93d998109e 100644
> --- a/crypto/chacha20poly1305.c
> +++ b/crypto/chacha20poly1305.c
> @@ -19,6 +19,7 @@
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/string.h>
>
> #include "internal.h"
>
> @@ -388,7 +389,7 @@ static int poly_genkey(struct aead_request *req)
> }
>
> sg_init_table(creq->src, 1);
> - memset(rctx->key, 0, sizeof(rctx->key));
> + memzero_explicit(rctx->key, sizeof(rctx->key));

Please explain the purpose of this patch. As it stands this
makes no sense.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2018-11-29 19:33:22

by Herbert Xu

Subject: Re: [PATCH] crypto: use memzero_explicit instead of memset to clear contexts.

On Thu, Nov 29, 2018 at 06:59:50AM +0000, David CARLIER wrote:
> Meant regardless how the kernel is compiled (ie optimisation level),
> the contexts are guaranteed to be wiped because of the memory fences
> used.

Please avoid top-posting.

> On Thu, 29 Nov 2018 at 06:49, Herbert Xu <[email protected]> wrote:
> >
> > On Wed, Nov 28, 2018 at 07:36:50PM +0000, David CARLIER wrote:
> > > There might be a little performance drop but to make sure it stands
> > > by it comments, we really wipe the whole context after usage.
> > > ---
> > > crypto/chacha20poly1305.c | 3 ++-
> > > crypto/md5.c | 2 +-
> > > crypto/rmd128.c | 3 ++-
> > > crypto/rmd160.c | 3 ++-
> > > crypto/rmd256.c | 3 ++-
> > > crypto/rmd320.c | 3 ++-
> > > crypto/sha3_generic.c | 3 ++-
> > > 7 files changed, 13 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c
> > > index 600afa99941f..6e93d998109e 100644
> > > --- a/crypto/chacha20poly1305.c
> > > +++ b/crypto/chacha20poly1305.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/init.h>
> > > #include <linux/kernel.h>
> > > #include <linux/module.h>
> > > +#include <linux/string.h>
> > >
> > > #include "internal.h"
> > >
> > > @@ -388,7 +389,7 @@ static int poly_genkey(struct aead_request *req)
> > > }
> > >
> > > sg_init_table(creq->src, 1);
> > > - memset(rctx->key, 0, sizeof(rctx->key));
> > > + memzero_explicit(rctx->key, sizeof(rctx->key));
> >
> > Please explain the purpose of this patch. As it stands this
> > makes no sense.

memzero_explicit is really only useful for stack memory. You need
to explain why it makes sense to use it in this context. For
example, by providing an example of a miscompile or how it is
even possible for this to happen.

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2018-11-29 18:04:23

by David CARLIER

Subject: Re: [PATCH] crypto: use memzero_explicit instead of memset to clear contexts.

Meant regardless how the kernel is compiled (ie optimisation level),
the contexts are guaranteed to be wiped because of the memory fences
used.

Kind regards.
On Thu, 29 Nov 2018 at 06:49, Herbert Xu <[email protected]> wrote:
>
> On Wed, Nov 28, 2018 at 07:36:50PM +0000, David CARLIER wrote:
> > There might be a little performance drop but to make sure it stands
> > by it comments, we really wipe the whole context after usage.
> > ---
> > crypto/chacha20poly1305.c | 3 ++-
> > crypto/md5.c | 2 +-
> > crypto/rmd128.c | 3 ++-
> > crypto/rmd160.c | 3 ++-
> > crypto/rmd256.c | 3 ++-
> > crypto/rmd320.c | 3 ++-
> > crypto/sha3_generic.c | 3 ++-
> > 7 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/crypto/chacha20poly1305.c b/crypto/chacha20poly1305.c
> > index 600afa99941f..6e93d998109e 100644
> > --- a/crypto/chacha20poly1305.c
> > +++ b/crypto/chacha20poly1305.c
> > @@ -19,6 +19,7 @@
> > #include <linux/init.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > +#include <linux/string.h>
> >
> > #include "internal.h"
> >
> > @@ -388,7 +389,7 @@ static int poly_genkey(struct aead_request *req)
> > }
> >
> > sg_init_table(creq->src, 1);
> > - memset(rctx->key, 0, sizeof(rctx->key));
> > + memzero_explicit(rctx->key, sizeof(rctx->key));
>
> Please explain the purpose of this patch. As it stands this
> makes no sense.
>
> Thanks,
> --
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt