When leaving a function use memzero_explicit instead of memset(0) to
clear locally allocated/owned buffers. memset(0) may be optimized away.
All of the affected buffers contain sensitive data, key material or
derivatives of one of those two.
---
arch/x86/crypto/sha256_ssse3_glue.c | 2 +-
drivers/usb/wusbcore/security.c | 2 +-
fs/cifs/smbencrypt.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
index 8fad72f..b616e63 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c
+++ b/arch/x86/crypto/sha256_ssse3_glue.c
@@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
dst[i] = cpu_to_be32(sctx->state[i]);
/* Wipe context */
- memset(sctx, 0, sizeof(*sctx));
+ memzero_explicit(sctx, sizeof(*sctx));
return 0;
}
diff --git a/drivers/usb/wusbcore/security.c b/drivers/usb/wusbcore/security.c
index b66faaf..a25f4fe 100644
--- a/drivers/usb/wusbcore/security.c
+++ b/drivers/usb/wusbcore/security.c
@@ -521,7 +521,7 @@ error_wusbhc_set_ptk:
error_hs3:
error_hs2:
error_hs1:
- memset(hs, 0, 3*sizeof(hs[0]));
+ memzero_explicit(hs, 3*sizeof(hs[0]));
memzero_explicit(&keydvt_out, sizeof(keydvt_out));
memzero_explicit(&keydvt_in, sizeof(keydvt_in));
memzero_explicit(&ccm_n, sizeof(ccm_n));
diff --git a/fs/cifs/smbencrypt.c b/fs/cifs/smbencrypt.c
index 6c15663..a4232ec 100644
--- a/fs/cifs/smbencrypt.c
+++ b/fs/cifs/smbencrypt.c
@@ -221,7 +221,7 @@ E_md4hash(const unsigned char *passwd, unsigned char *p16,
}
rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
- memset(wpwd, 0, 129 * sizeof(__le16));
+ memzero_explicit(wpwd, sizeof(wpwd));
return rc;
}
--
2.1.4
On Sun, Jan 04, 2015 at 07:05:40PM +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear locally allocated/owned buffers. memset(0) may be optimized away.
>
> All of the affected buffers contain sensitive data, key material or
> derivatives of one of those two.
Nack.
> diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
> index 8fad72f..b616e63 100644
> --- a/arch/x86/crypto/sha256_ssse3_glue.c
> +++ b/arch/x86/crypto/sha256_ssse3_glue.c
> @@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
> dst[i] = cpu_to_be32(sctx->state[i]);
>
> /* Wipe context */
> - memset(sctx, 0, sizeof(*sctx));
> + memzero_explicit(sctx, sizeof(*sctx));
sctx does not point to stack memory so this is bogus.
Only stack memory cleared just before it goes out of scope needs
memzero_explicit.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Jan 05, 2015 at 08:35:38 +1100, Herbert Xu wrote:
> On Sun, Jan 04, 2015 at 07:05:40PM +0100, Giel van Schijndel wrote:
>> When leaving a function use memzero_explicit instead of memset(0) to
>> clear locally allocated/owned buffers. memset(0) may be optimized away.
>>
>> All of the affected buffers contain sensitive data, key material or
>> derivatives of one of those two.
>
> Nack.
Do you mean that the sample below doesn't contain sensitive data?
Or is there another buffer(s) in my patch that you believe doesn't
contain that?
(I contain a hash derived from secret material to be a "derivative of one of
those two", leaking of which could lead to a confirmation-attack).
>> diff --git a/arch/x86/crypto/sha256_ssse3_glue.c b/arch/x86/crypto/sha256_ssse3_glue.c
>> index 8fad72f..b616e63 100644
>> --- a/arch/x86/crypto/sha256_ssse3_glue.c
>> +++ b/arch/x86/crypto/sha256_ssse3_glue.c
>> @@ -164,7 +164,7 @@ static int sha256_ssse3_final(struct shash_desc *desc, u8 *out)
>> dst[i] = cpu_to_be32(sctx->state[i]);
>>
>> /* Wipe context */
>> - memset(sctx, 0, sizeof(*sctx));
>> + memzero_explicit(sctx, sizeof(*sctx));
>
> sctx does not point to stack memory so this is bogus.
>
> Only stack memory cleared just before it goes out of scope needs
> memzero_explicit.
Is that because the compiler can't safely optimize memset(0) away for a
variable with greater-than-local scope?
Because I think using memzero_explicit() as an indicator that said
buffer contains data that really *needs* to be destroyed is enough of a
reason already. I believe any overhead is negligable because there's
only a single extra call involved and that's cheap for the extra clarity
it buys (i.e. "this piece of memory *really* needs to be destroyed
beyond this statement"). (Though this approach is only valid for memory
that can contain security-sensitive data IMO.)
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Always code as if the guy who ends up maintaining your code will be a
violent psychopath who knows where you live."
-- Rick Osborne
On Sun, Jan 04, 2015 at 19:05:40 +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear locally allocated/owned buffers. memset(0) may be optimized away.
>
> All of the affected buffers contain sensitive data, key material or
> derivatives of one of those two.
> ---
Forgot to:
Signed-off-by: Giel van Schijndel <[email protected]>
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"Question: what do you call your programming methodology?
Answer: Faith based development. You code and then pray that it works."
-- John Spelner
On Sun, Jan 04, 2015 at 11:49:09PM +0100, Giel van Schijndel wrote:
>
> > sctx does not point to stack memory so this is bogus.
> >
> > Only stack memory cleared just before it goes out of scope needs
> > memzero_explicit.
>
> Is that because the compiler can't safely optimize memset(0) away for a
> variable with greater-than-local scope?
Exactly. memzero_explicit is not a marker for sensitive data.
Its only purpose is to prevent the compiler from optimising away
zeroing that occurs at the end of a scope.
Daniel, we should add a comment so that people stop sending bogus
patches with memzero_explicit.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Jan 05, 2015 at 10:36:37 +1100, Herbert Xu wrote:
> On Sun, Jan 04, 2015 at 11:49:09PM +0100, Giel van Schijndel wrote:
>>
>>> sctx does not point to stack memory so this is bogus.
>>>
>>> Only stack memory cleared just before it goes out of scope needs
>>> memzero_explicit.
>>
>> Is that because the compiler can't safely optimize memset(0) away for a
>> variable with greater-than-local scope?
>
> Exactly. memzero_explicit is not a marker for sensitive data.
> Its only purpose is to prevent the compiler from optimising away
> zeroing that occurs at the end of a scope.
Question: are you sure the compiler won't optimize the call to memset(0)
way if it's immediately followed by kfree()?
Because one of my changes concerns that situation.
Another actually does change a stack-allocated buffer, I'll split that
one off right away.
--
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"When all you have is a hammer, everything starts to look like a nail."
-- Abraham Maslow
On Tue, Jan 06, 2015 at 08:42:26PM +0100, Giel van Schijndel wrote:
>
> Question: are you sure the compiler won't optimize the call to memset(0)
> way if it's immediately followed by kfree()?
Yes it won't be optimised away. However, you could use kzfree.
> Another actually does change a stack-allocated buffer, I'll split that
> one off right away.
OK.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 01/05/2015 12:36 AM, Herbert Xu wrote:
...
> Daniel, we should add a comment so that people stop sending bogus
> patches with memzero_explicit.
I'll send a patch adding a comment, sure.
Thanks,
Daniel