Re: [PATCH v2] compiler: prevent dead store elimination
> > >> +#define ARRAY_PREVENT_DSE(p, n) > > > > Who says the Intel compiler doesn't need this? > > There was a comment in include/linux/compiler-intel.h that it's not supported. That's true for the ia64 version, but not for the x86 version which supports gcc compatible inline assembler. So on x86 you can use it. On Itanium it probably would need some other compiler built in. Also in any case some form of memory clobber is needed because it'll surely do dead-store-optimization. -Andi -- 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 v2] compiler: prevent dead store elimination
> > + do {\
> > + struct __scrub { typeof(*p) c[n]; };\
>
> The typeof(*p) suggestion doesn't work. It would require p to always be
> a pointer type with an accurate (for memset) sizeof(*p). In general however
> we'll memset some array described by a void*/size_t pair, and typeof in
> that case is useless. The original'struct __scrub { char c[n]; };' was Ok.
I just suggested it because of the original array name of the macro
and without it it would have only worked for char arrays. With the new
naming it's ok I guess.
It could be made to work with macros and builtin_type_match I guess,
but it would be fairly ugly and not worth it.
-Andi
--
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 v2] compiler: prevent dead store elimination
Roel Kluin writes:
> Due to optimization A call to memset() may be removed as a dead store when
___^ lower-case a
> 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.
This paragraph, while accurate, is unrelated to the contents of this
patch. Note that in an asm(), a "memory" clobber (see barrier()) is
not at all the same thing as a memory operand "m"(...), which is what
we're using here. Just drop this bit.
> 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.
and then
> This works with
> gcc-3.2.3 up to gcc-4.4.3.
You've edited my comments and put them together in a way that doesn't
quite make sense. In particular, "This works" doesn't refer to the
previous text but the local-struct-of-variable-size trick. The text
should read something like:
To prevent a memset() from being eliminated, the compiler must belive
that the memory area is used after the memset(). Also, every byte in
the memory area must be used. If only the first byte is used, via e.g.
asm("" :: "m"(*(char*)p)), then the compiler _will_ skip scrubbing
bytes beyond the first.
The trick is to define a local type of the same size as the memory
area, and use it for the "m" operand in a dummy asm():
static inline void fake_memory_use(void *p, size_t n)
{
struct __scrub { char c[n]; }
asm("" : : "m"(*(struct __scrub *)p));
}
This looks strange, n being a variable, but has been confirmed to
work with gcc-3.2.3 up to gcc-4.4.3.
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 188fcae..2f14199 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -302,4 +302,15 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
> int val, int expect);
> */
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>
> +/*
> + * Dead store elimination is an optimization that may remove a write to a
> + * buffer that is not used anymore. Use ARRAY_KEEP_STORE after a write when
> + * the scrub is required for security reasons.
> + */
> +#define ARRAY_KEEP_STORE(p, n) \
1. This doesn't need to be a macro. Please make it a static inline function.
2. Its function is to "fake" a use of a memory area, which allows us to prevent
unwanted DSE elsewhere. So this should be fake_memory_use() or something.
> +do {\
> +struct __scrub { typeof(*p) c[n]; };\
The typeof(*p) suggestion doesn't work. It would require p to always be
a pointer type with an accurate (for memset) sizeof(*p). In general however
we'll memset some array described by a void*/size_t pair, and typeof in
that case is useless. The original'struct __scrub { char c[n]; };' was Ok.
> 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);
Why is this __kernel_size_t and not just plain size_t? It's not
a user-space API.
--
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 v2] 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.
Thanks to Mikael Pettersson for this information.
Signed-off-by: Roel Kluin
---
> You forgot to credit Mikael
Not intended, I first put it in the changelog but then decided to thank all
involved in my comments. Do credits belong in the changelog?
>> +#define ARRAY_PREVENT_DSE(p, n) \
>
> Maybe it's just me, but the name is ugly.
Ok, changed it to ARRAY_KEEP_STORE() or do you have a suggestion?
>> +do {\
>> +struct __scrub { char c[n]; }; \
>
> Better typeof(*p)[n]
I guess you meant `struct __scrub { typeof(*p) c[n]; };' ?
In the current implementation p is a void pointer. Is it ok to declare an array
of voids? In kernel it compiles, in my testcase it produces an error.
>> +++ b/include/linux/compiler-intel.h
>> +#define ARRAY_PREVENT_DSE(p, n)
>
> Who says the Intel compiler doesn't need this?
There was a comment in include/linux/compiler-intel.h that it's not supported.
> I'm sure it does dead store elimination too and it understands
> gcc asm syntax.
Ok, should I put it in include/linux/compiler.h or where?
>> +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
You mean the secure_bzero() function, right?
Thanks for comments, Roel
include/linux/compiler.h | 11 +++
include/linux/string.h |1 +
lib/string.c | 13 +
3 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 188fcae..2f14199 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -302,4 +302,15 @@ void ftrace_likely_update(struct ftrace_branch_data *f,
int val, int expect);
*/
#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+/*
+ * Dead store elimination is an optimization that may remove a write to a
+ * buffer that is not used anymore. Use ARRAY_KEEP_STORE after a write when
+ * the scrub is required for security reasons.
+ */
+#define ARRAY_KEEP_STORE(p, n) \
+ do {\
+ struct __scrub { typeof(*p) c[n]; };\
+ asm("" : : "m"(*(struct __scrub *)p)); \
+ } while (0)
+
#endif /* __LINUX_COMPILER_H */
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..5576161 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.
+ */
+inline void secure_bzero(void *p, size_t n)
+{
+ memset(p, 0, n);
+ ARRAY_KEEP_STORE(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
