| Message ID | 1420394744-20268-1-git-send-email-me@mortis.eu (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Herbert Xu |
| Headers | show |
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,
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.)
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>
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,
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.
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,
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; }