All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-04 18:05 Giel van Schijndel
       [not found] ` <1420394744-20268-1-git-send-email-me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org>
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-04 18:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Giel van Schijndel, Herbert Xu, David S. Miller, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 18:05 [PATCH] Use memzero_explicit to clear local buffers Giel van Schijndel
@ 2015-01-04 21:35     ` Herbert Xu
  2015-01-04 23:05   ` Giel van Schijndel
  2015-01-06 21:37 ` [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer Giel van Schijndel
  2 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-04 21:35 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-04 21:35     ` Herbert Xu
  0 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-04 21:35 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 21:35     ` Herbert Xu
@ 2015-01-04 22:49       ` Giel van Schijndel
  -1 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-04 22:49 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-04 22:49       ` Giel van Schijndel
  0 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-04 22:49 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 18:05 [PATCH] Use memzero_explicit to clear local buffers Giel van Schijndel
@ 2015-01-04 23:05   ` Giel van Schijndel
  2015-01-04 23:05   ` Giel van Schijndel
  2015-01-06 21:37 ` [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer Giel van Schijndel
  2 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-04 23:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Herbert Xu, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

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 <me@mortis.eu>

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-04 23:05   ` Giel van Schijndel
  0 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-04 23:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Herbert Xu, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

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

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 <me@mortis.eu>

-- 
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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 22:49       ` Giel van Schijndel
@ 2015-01-04 23:36           ` Herbert Xu
  -1 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-04 23:36 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-04 23:36           ` Herbert Xu
  0 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-04 23:36 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 23:36           ` Herbert Xu
  (?)
@ 2015-01-05 17:36           ` Daniel Borkmann
  -1 siblings, 0 replies; 21+ messages in thread
From: Daniel Borkmann @ 2015-01-05 17:36 UTC (permalink / raw)
  To: Herbert Xu
  Cc: open list:COMMON INTERNET F...,
	Randy Dunlap, Giel van Schijndel, Greg Kroah-Hartman,
	maintainer:X86 ARCHITECTURE...,
	moderated list:COMMON INTERNET F...,
	linux-kernel, Steve French, Julia Lawall, Thomas Pugliese,
	Ingo Molnar, Rahul Bedarkar, CRYPTO API, H. Peter Anvin,
	Thomas Gleixner, open list:CERTIFIED WIRELES...,
	David S. Miller

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-04 23:36           ` Herbert Xu
@ 2015-01-06 19:42             ` Giel van Schijndel
  -1 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-06 19:42 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-06 19:42             ` Giel van Schijndel
  0 siblings, 0 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-06 19:42 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
  2015-01-06 19:42             ` Giel van Schijndel
@ 2015-01-06 20:54                 ` Herbert Xu
  -1 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-06 20:54 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] Use memzero_explicit to clear local buffers
@ 2015-01-06 20:54                 ` Herbert Xu
  0 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-06 20:54 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel, David S. Miller, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, maintainer:X86 ARCHITECTURE...,
	Greg Kroah-Hartman, Steve French, Rahul Bedarkar,
	Thomas Pugliese, Randy Dunlap, Julia Lawall,
	open list:CRYPTO API, open list:CERTIFIED WIRELES...,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	Daniel Borkmann

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
  2015-01-04 18:05 [PATCH] Use memzero_explicit to clear local buffers Giel van Schijndel
       [not found] ` <1420394744-20268-1-git-send-email-me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org>
  2015-01-04 23:05   ` Giel van Schijndel
@ 2015-01-06 21:37 ` Giel van Schijndel
  2015-01-06 22:59     ` Herbert Xu
  2015-01-09 18:53     ` Steve French
  2 siblings, 2 replies; 21+ messages in thread
From: Giel van Schijndel @ 2015-01-06 21:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Herbert Xu, Giel van Schijndel, Steve French,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

When leaving a function use memzero_explicit instead of memset(0) to
clear stack allocated buffers. memset(0) may be optimized away.

This particular buffer is highly likely to contain sensitive data which
we shouldn't leak (it's named 'passwd' after all).

Signed-off-by: Giel van Schijndel <me@mortis.eu>
Reported-at: http://www.viva64.com/en/b/0299/
Reported-by: Andrey Karpov
Reported-by: Svyatoslav Razmyslov
---
 fs/cifs/smbencrypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
  2015-01-06 21:37 ` [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer Giel van Schijndel
@ 2015-01-06 22:59     ` Herbert Xu
  2015-01-09 18:53     ` Steve French
  1 sibling, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-06 22:59 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel, Steve French, open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
> 
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
> 
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
@ 2015-01-06 22:59     ` Herbert Xu
  0 siblings, 0 replies; 21+ messages in thread
From: Herbert Xu @ 2015-01-06 22:59 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: linux-kernel, Steve French, open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
> 
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
> 
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
  2015-01-06 21:37 ` [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer Giel van Schijndel
@ 2015-01-09 18:53     ` Steve French
  2015-01-09 18:53     ` Steve French
  1 sibling, 0 replies; 21+ messages in thread
From: Steve French @ 2015-01-09 18:53 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: Steve French, open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...,
	LKML, Herbert Xu

Looks fine to me - will merge into cifs-2.6.git

On Tue, Jan 6, 2015 at 3:37 PM, Giel van Schijndel <me@mortis.eu> wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
>
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
>
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov
> ---
>  fs/cifs/smbencrypt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
@ 2015-01-09 18:53     ` Steve French
  0 siblings, 0 replies; 21+ messages in thread
From: Steve French @ 2015-01-09 18:53 UTC (permalink / raw)
  To: Giel van Schijndel
  Cc: LKML, Herbert Xu, Steve French, open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

Looks fine to me - will merge into cifs-2.6.git

On Tue, Jan 6, 2015 at 3:37 PM, Giel van Schijndel <me@mortis.eu> wrote:
> When leaving a function use memzero_explicit instead of memset(0) to
> clear stack allocated buffers. memset(0) may be optimized away.
>
> This particular buffer is highly likely to contain sensitive data which
> we shouldn't leak (it's named 'passwd' after all).
>
> Signed-off-by: Giel van Schijndel <me@mortis.eu>
> Reported-at: http://www.viva64.com/en/b/0299/
> Reported-by: Andrey Karpov
> Reported-by: Svyatoslav Razmyslov
> ---
>  fs/cifs/smbencrypt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
  2015-01-06 22:59     ` Herbert Xu
@ 2015-01-13  0:12       ` Steve French
  -1 siblings, 0 replies; 21+ messages in thread
From: Steve French @ 2015-01-13  0:12 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Giel van Schijndel, LKML, Steve French,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

merged into cifs-2.6.git for-next

On Tue, Jan 6, 2015 at 4:59 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
>> When leaving a function use memzero_explicit instead of memset(0) to
>> clear stack allocated buffers. memset(0) may be optimized away.
>>
>> This particular buffer is highly likely to contain sensitive data which
>> we shouldn't leak (it's named 'passwd' after all).
>>
>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>> Reported-at: http://www.viva64.com/en/b/0299/
>> Reported-by: Andrey Karpov
>> Reported-by: Svyatoslav Razmyslov
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> 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



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer
@ 2015-01-13  0:12       ` Steve French
  0 siblings, 0 replies; 21+ messages in thread
From: Steve French @ 2015-01-13  0:12 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Giel van Schijndel, LKML, Steve French,
	open list:COMMON INTERNET F...,
	moderated list:COMMON INTERNET F...

merged into cifs-2.6.git for-next

On Tue, Jan 6, 2015 at 4:59 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Jan 06, 2015 at 10:37:00PM +0100, Giel van Schijndel wrote:
>> When leaving a function use memzero_explicit instead of memset(0) to
>> clear stack allocated buffers. memset(0) may be optimized away.
>>
>> This particular buffer is highly likely to contain sensitive data which
>> we shouldn't leak (it's named 'passwd' after all).
>>
>> Signed-off-by: Giel van Schijndel <me@mortis.eu>
>> Reported-at: http://www.viva64.com/en/b/0299/
>> Reported-by: Andrey Karpov
>> Reported-by: Svyatoslav Razmyslov
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> 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



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-01-13  0:13 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-04 18:05 [PATCH] Use memzero_explicit to clear local buffers Giel van Schijndel
     [not found] ` <1420394744-20268-1-git-send-email-me-sZ9Uef1cvPWHXe+LvDLADg@public.gmane.org>
2015-01-04 21:35   ` Herbert Xu
2015-01-04 21:35     ` Herbert Xu
2015-01-04 22:49     ` Giel van Schijndel
2015-01-04 22:49       ` Giel van Schijndel
     [not found]       ` <20150104224909.GB4806-zsKMh+JvXepbNiWYSlF1AbANlwIBtoSN@public.gmane.org>
2015-01-04 23:36         ` Herbert Xu
2015-01-04 23:36           ` Herbert Xu
2015-01-05 17:36           ` Daniel Borkmann
2015-01-06 19:42           ` Giel van Schijndel
2015-01-06 19:42             ` Giel van Schijndel
     [not found]             ` <20150106194226.GM4806-zsKMh+JvXepbNiWYSlF1AbANlwIBtoSN@public.gmane.org>
2015-01-06 20:54               ` Herbert Xu
2015-01-06 20:54                 ` Herbert Xu
2015-01-04 23:05 ` Giel van Schijndel
2015-01-04 23:05   ` Giel van Schijndel
2015-01-06 21:37 ` [PATCH RESEND] cifs: use memzero_explicit to clear stack buffer Giel van Schijndel
2015-01-06 22:59   ` Herbert Xu
2015-01-06 22:59     ` Herbert Xu
2015-01-13  0:12     ` Steve French
2015-01-13  0:12       ` Steve French
2015-01-09 18:53   ` Steve French
2015-01-09 18:53     ` Steve French

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.