Re: [PATCH v1] compiler: prevent dead store elimination
Arjan van de Ven writes:
On Sat, 27 Feb 2010 21:47:42 +0100
Roel Kluin roel.kl...@gmail.com wrote:
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
+}
+EXPORT_SYMBOL(secure_bzero);
please don't introduce bzero again to the kernel;
make it secure_memset() please.
In principle I would agree, but bzero() avoids the unfortunately
rather common mistake of swapping the int/size_t parameters to
memset(). I.e., people writing memset(p, n, 0) not memset(p, 0, n).
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] compiler: prevent dead store elimination
On Mon, Mar 1, 2010 at 11:32 AM, Mikael Pettersson mi...@it.uu.se wrote:
Arjan van de Ven writes:
On Sat, 27 Feb 2010 21:47:42 +0100
Roel Kluin roel.kl...@gmail.com wrote:
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
+}
+EXPORT_SYMBOL(secure_bzero);
please don't introduce bzero again to the kernel;
make it secure_memset() please.
What's so secure in this function? :^)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] compiler: prevent dead store elimination
On Sun, Feb 28, 2010 at 09:15:11PM -0800, Arjan van de Ven wrote:
On Sat, 27 Feb 2010 21:47:42 +0100
Roel Kluin roel.kl...@gmail.com wrote:
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
+}
+EXPORT_SYMBOL(secure_bzero);
please don't introduce bzero again to the kernel;
make it secure_memset() please.
Would there ever be any reason to set the key to something
else than 0?
IMHO bzero is less error prone. With memset there are regular
bugs when the two end arguments get exchanged.
You could call it differently if you have a problem with old BSD
names, but inherently there's nothing wrong with them. One possibility
would be the same name as VC++ uses.
-Andi
--
a...@linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] compiler: prevent dead store elimination
Every byte in the [p,p+n[ range must be used. If you only use the
first byte, via e.g. asm( :: m(*(char*)p)), then the compiler
_will_ skip scrubbing bytes beyond the first. This works with
gcc-3.2.3 up to gcc-4.4.3.
You forgot to credit Mikael who did all the hard work figuring
this out?
/*
+ * Dead store elimination (DSE) is an optimization that may remove a write to
+ * a buffer that is not used anymore. Use ARRAY_PREVENT_DSE after a write
when
+ * the scrub is required for security reasons.
+ */
+#define ARRAY_PREVENT_DSE(p, n) \
Maybe it's just me, but the name is ugly.
+ do {\
+ struct __scrub { char c[n]; }; \
Better typeof(*p)[n]
+++ b/include/linux/compiler-intel.h
@@ -14,9 +14,11 @@
* It uses intrinsics to do the equivalent things.
*/
#undef barrier
+#undef ARRAY_PREVENT_DSE
#undef RELOC_HIDE
#define barrier() __memory_barrier()
+#define ARRAY_PREVENT_DSE(p, n)
Who says the Intel compiler doesn't need this?
I'm sure it does dead store elimination too and it understands
gcc asm syntax.
+/**
+ * secure_bzero - Call memset to fill a region of memory with zeroes and
+ * ensure this memset is not removed due to dead store elimination.
+ * @p: Pointer to the start of the area.
+ * @n: The size of the area.
+ */
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
I think that's a candidate for a inline
-Andi
--
a...@linux.intel.com -- Speaking for myself only.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] compiler: prevent dead store elimination
Andi Kleen wrote:
Every byte in the [p,p+n[ range must be used. If you only use the
first byte, via e.g. asm( :: m(*(char*)p)), then the compiler
_will_ skip scrubbing bytes beyond the first. This works with
gcc-3.2.3 up to gcc-4.4.3.
You forgot to credit Mikael who did all the hard work figuring
this out?
/*
+ * Dead store elimination (DSE) is an optimization that may remove a write to
+ * a buffer that is not used anymore. Use ARRAY_PREVENT_DSE after a write when
+ * the scrub is required for security reasons.
+ */
+#define ARRAY_PREVENT_DSE(p, n)\
Maybe it's just me, but the name is ugly.
+ do {\
+ struct __scrub { char c[n]; }; \
Better typeof(*p)[n]
+++ b/include/linux/compiler-intel.h
@@ -14,9 +14,11 @@
* It uses intrinsics to do the equivalent things.
*/
#undef barrier
+#undef ARRAY_PREVENT_DSE
#undef RELOC_HIDE
#define barrier() __memory_barrier()
+#define ARRAY_PREVENT_DSE(p, n)
Who says the Intel compiler doesn't need this?
I'm sure it does dead store elimination too and it understands
gcc asm syntax.
According to the Intel forum, it not only doesn't, but a request for this as a
feature was rejected, so it won't. Or am I misreading this?
http://software.intel.com/en-us/forums/showthread.php?t=46770
--
Bill Davidsen david...@tmr.com
We have more to fear from the bungling of the incompetent than from
the machinations of the wicked. - from Slashdot
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] compiler: prevent dead store elimination
On Sat, 27 Feb 2010 21:47:42 +0100
Roel Kluin roel.kl...@gmail.com wrote:
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
+}
+EXPORT_SYMBOL(secure_bzero);
please don't introduce bzero again to the kernel;
make it secure_memset() please.
--
Arjan van de VenIntel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1] compiler: prevent dead store elimination
Due to optimization A call to memset() may be removed as a dead store when
the buffer is not used after its value is overwritten. The new function
secure_bzero() ensures a section of memory is padded with zeroes.
From the GCC manual, section 5.37:
If your assembler instructions access memory in an unpredictable
fashion, add `memory' to the list of clobbered registers. This will
cause GCC to not keep memory values cached in registers across the
assembler instruction and not optimize stores or loads to that memory.
Every byte in the [p,p+n[ range must be used. If you only use the
first byte, via e.g. asm( :: m(*(char*)p)), then the compiler
_will_ skip scrubbing bytes beyond the first. This works with
gcc-3.2.3 up to gcc-4.4.3.
Signed-off-by: Roel Kluin roel.kl...@gmail.com
---
include/linux/compiler-gcc.h | 11 +++
include/linux/compiler-intel.h |2 ++
include/linux/string.h |1 +
lib/string.c | 13 +
4 files changed, 27 insertions(+), 0 deletions(-)
Thanks all for the required information, checkpatch.pl and compile
tested. In my (non-kernel) testcase this prevents dead store elimination.
Comments?
Roel
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 73dcf80..0799938 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -12,6 +12,17 @@
#define barrier() __asm__ __volatile__(: : :memory)
/*
+ * Dead store elimination (DSE) is an optimization that may remove a write to
+ * a buffer that is not used anymore. Use ARRAY_PREVENT_DSE after a write when
+ * the scrub is required for security reasons.
+ */
+#define ARRAY_PREVENT_DSE(p, n)\
+ do {\
+ struct __scrub { char c[n]; }; \
+ asm( : : m(*(struct __scrub *)p)); \
+ } while (0)
+
+/*
* This macro obfuscates arithmetic on a variable address so that gcc
* shouldn't recognize the original var, and make assumptions about it.
*
diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
index d8e636e..e8e11f3 100644
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -14,9 +14,11 @@
* It uses intrinsics to do the equivalent things.
*/
#undef barrier
+#undef ARRAY_PREVENT_DSE
#undef RELOC_HIDE
#define barrier() __memory_barrier()
+#define ARRAY_PREVENT_DSE(p, n)
#define RELOC_HIDE(ptr, off) \
({ unsigned long __ptr; \
diff --git a/include/linux/string.h b/include/linux/string.h
index a716ee2..36213e6 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -118,6 +118,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
extern char *kstrdup(const char *s, gfp_t gfp);
extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
+extern void secure_bzero(void *, __kernel_size_t);
extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);
diff --git a/lib/string.c b/lib/string.c
index a1cdcfc..588ac31 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -559,6 +559,19 @@ void *memset(void *s, int c, size_t count)
EXPORT_SYMBOL(memset);
#endif
+/**
+ * secure_bzero - Call memset to fill a region of memory with zeroes and
+ * ensure this memset is not removed due to dead store elimination.
+ * @p: Pointer to the start of the area.
+ * @n: The size of the area.
+ */
+void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_PREVENT_DSE(p, n);
+}
+EXPORT_SYMBOL(secure_bzero);
+
#ifndef __HAVE_ARCH_MEMCPY
/**
* memcpy - Copy one area of memory to another
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
