[kernel-hardening] [patch] lib: check for strcpy() overflows to fixed length buffers

[kernel-hardening] [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

There are sometimes where we know that we are doing an strcpy() into a
fixed length buffer.  In those cases, we could verify that the strcpy()
doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
if you want to check for that.  The downside is that it makes strcpy
slower.

I introduced __compiletime_size() to make this work.  It returns the
size of the destination buffer or zero if the size isn't known.  The
__compiletime_object_size() is similar but if you pass it a struct
member then it returns the size of everything from there to the end of
the struct.  Another difference is __compiletime_object_size() returns
-1 for unknown sizes.

If you pass a char pointer to __compiletime_size() then it returns zero.

The strcpy() check ignores buffers with just one byte because people
often use those for variable length strings at the end of a struct.

I have tested this patch lightly and created some bugs for it to detect
but I have not detected any real life overflows.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
index e863dd5..5e0fc2b 100644
--- a/include/acpi/platform/acenv.h
+++ b/include/acpi/platform/acenv.h
@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 2507fd2..1fb7fd0 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -16,6 +16,9 @@
 #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif
+#if GCC_VERSION > 40600
+# define __compiletime_size(obj) __builtin_object_size(obj, 3)
+#endif
 
 #if GCC_VERSION >= 40300
 /* Mark functions as cold. gcc will assume any path leading to a call
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ee7239e..b615964 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #ifndef __compiletime_object_size
 # define __compiletime_object_size(obj) -1
 #endif
+#ifndef __compiletime_size
+# define __compiletime_size(obj) 0
+#endif
 #ifndef __compiletime_warning
 # define __compiletime_warning(message)
 #endif
diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5..fc126a1 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
 	return tail ? tail + 1 : path;
 }
 
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {						\
+	int len = __compiletime_size(dest);				\
+	if (len > 1 && strlen(src) >= len)				\
+		WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);	\
+	strcpy(dest, src);						\
+} while (0)
+#endif
+
 #endif /* _LINUX_STRING_H_ */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 819ac51..94db086 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
 
 	  If unsure, say N.
 
+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+	bool "Strict checks for strcpy() overflows"
+	depends on DEBUG_KERNEL
+	help
+	  Enabling this option adds some extra sanity checks when strcpy() is
+	  called().  This will slow down the kernel a bit.
+
+	  If unsure, say N.
+
 source kernel/trace/Kconfig
 
 menu "Runtime Testing"

[patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

There are sometimes where we know that we are doing an strcpy() into a
fixed length buffer.  In those cases, we could verify that the strcpy()
doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
if you want to check for that.  The downside is that it makes strcpy
slower.

I introduced __compiletime_size() to make this work.  It returns the
size of the destination buffer or zero if the size isn't known.  The
__compiletime_object_size() is similar but if you pass it a struct
member then it returns the size of everything from there to the end of
the struct.  Another difference is __compiletime_object_size() returns
-1 for unknown sizes.

If you pass a char pointer to __compiletime_size() then it returns zero.

The strcpy() check ignores buffers with just one byte because people
often use those for variable length strings at the end of a struct.

I have tested this patch lightly and created some bugs for it to detect
but I have not detected any real life overflows.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
index e863dd5..5e0fc2b 100644
--- a/include/acpi/platform/acenv.h
+++ b/include/acpi/platform/acenv.h
@@ -320,7 +320,7 @@
 #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
 #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
 #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
-#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
+#define ACPI_STRCPY(d,s)        strcpy((d), (s))
 #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
 #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
 #define ACPI_STRCMP(d,s)        strcmp((d), (s))
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 2507fd2..1fb7fd0 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -16,6 +16,9 @@
 #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
 # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 #endif
+#if GCC_VERSION > 40600
+# define __compiletime_size(obj) __builtin_object_size(obj, 3)
+#endif
 
 #if GCC_VERSION >= 40300
 /* Mark functions as cold. gcc will assume any path leading to a call
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index ee7239e..b615964 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 #ifndef __compiletime_object_size
 # define __compiletime_object_size(obj) -1
 #endif
+#ifndef __compiletime_size
+# define __compiletime_size(obj) 0
+#endif
 #ifndef __compiletime_warning
 # define __compiletime_warning(message)
 #endif
diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5..fc126a1 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
 	return tail ? tail + 1 : path;
 }
 
+#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
+#define strcpy(dest, src) do {						\
+	int len = __compiletime_size(dest);				\
+	if (len > 1 && strlen(src) >= len)				\
+		WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);	\
+	strcpy(dest, src);						\
+} while (0)
+#endif
+
 #endif /* _LINUX_STRING_H_ */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 819ac51..94db086 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
 
 	  If unsure, say N.
 
+config DEBUG_STRICT_SLOW_STRCPY_CHECKS
+	bool "Strict checks for strcpy() overflows"
+	depends on DEBUG_KERNEL
+	help
+	  Enabling this option adds some extra sanity checks when strcpy() is
+	  called().  This will slow down the kernel a bit.
+
+	  If unsure, say N.
+
 source kernel/trace/Kconfig
 
 menu "Runtime Testing"

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Kees Cook]

On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.  The downside is that it makes strcpy
> slower.
>
> I introduced __compiletime_size() to make this work.  It returns the
> size of the destination buffer or zero if the size isn't known.  The
> __compiletime_object_size() is similar but if you pass it a struct
> member then it returns the size of everything from there to the end of
> the struct.  Another difference is __compiletime_object_size() returns
> -1 for unknown sizes.
>
> If you pass a char pointer to __compiletime_size() then it returns zero.
>
> The strcpy() check ignores buffers with just one byte because people
> often use those for variable length strings at the end of a struct.
>
> I have tested this patch lightly and created some bugs for it to detect
> but I have not detected any real life overflows.

Cool, we should absolutely have this. And that's good news that it
didn't trip anything. :)

>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> index e863dd5..5e0fc2b 100644
> --- a/include/acpi/platform/acenv.h
> +++ b/include/acpi/platform/acenv.h
> @@ -320,7 +320,7 @@
>  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
>  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
>  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
>  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
>  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
>  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 2507fd2..1fb7fd0 100644
> --- a/include/linux/compiler-gcc4.h
> +++ b/include/linux/compiler-gcc4.h
> @@ -16,6 +16,9 @@
>  #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
>  # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>  #endif
> +#if GCC_VERSION > 40600
> +# define __compiletime_size(obj) __builtin_object_size(obj, 3)
> +#endif
>
>  #if GCC_VERSION >= 40300
>  /* Mark functions as cold. gcc will assume any path leading to a call
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index ee7239e..b615964 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  #ifndef __compiletime_object_size
>  # define __compiletime_object_size(obj) -1
>  #endif
> +#ifndef __compiletime_size
> +# define __compiletime_size(obj) 0
> +#endif
>  #ifndef __compiletime_warning
>  # define __compiletime_warning(message)
>  #endif
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ac889c5..fc126a1 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
>         return tail ? tail + 1 : path;
>  }
>
> +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
> +#define strcpy(dest, src) do {                                         \
> +       int len = __compiletime_size(dest);                             \
> +       if (len > 1 && strlen(src) >= len)                              \
> +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \

This should probably BUG. An overflowing strcpy shouldn't be allowed
to continue.

> +       strcpy(dest, src);                                              \
> +} while (0)
> +#endif
> +
>  #endif /* _LINUX_STRING_H_ */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 819ac51..94db086 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
>
>           If unsure, say N.
>
> +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
> +       bool "Strict checks for strcpy() overflows"
> +       depends on DEBUG_KERNEL
> +       help
> +         Enabling this option adds some extra sanity checks when strcpy() is
> +         called().  This will slow down the kernel a bit.

Isn't this an entirely compile-time check? I would expect it to be
entirely optimized by the compiler. In fact, could this be turned into
a build failure instead?

> +
> +         If unsure, say N.
> +
>  source kernel/trace/Kconfig
>
>  menu "Runtime Testing"

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Kees Cook]

On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.  The downside is that it makes strcpy
> slower.
>
> I introduced __compiletime_size() to make this work.  It returns the
> size of the destination buffer or zero if the size isn't known.  The
> __compiletime_object_size() is similar but if you pass it a struct
> member then it returns the size of everything from there to the end of
> the struct.  Another difference is __compiletime_object_size() returns
> -1 for unknown sizes.
>
> If you pass a char pointer to __compiletime_size() then it returns zero.
>
> The strcpy() check ignores buffers with just one byte because people
> often use those for variable length strings at the end of a struct.
>
> I have tested this patch lightly and created some bugs for it to detect
> but I have not detected any real life overflows.

Cool, we should absolutely have this. And that's good news that it
didn't trip anything. :)

>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> index e863dd5..5e0fc2b 100644
> --- a/include/acpi/platform/acenv.h
> +++ b/include/acpi/platform/acenv.h
> @@ -320,7 +320,7 @@
>  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
>  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
>  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
>  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
>  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
>  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 2507fd2..1fb7fd0 100644
> --- a/include/linux/compiler-gcc4.h
> +++ b/include/linux/compiler-gcc4.h
> @@ -16,6 +16,9 @@
>  #if GCC_VERSION >= 40100 && GCC_VERSION < 40600
>  # define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>  #endif
> +#if GCC_VERSION > 40600
> +# define __compiletime_size(obj) __builtin_object_size(obj, 3)
> +#endif
>
>  #if GCC_VERSION >= 40300
>  /* Mark functions as cold. gcc will assume any path leading to a call
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index ee7239e..b615964 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -318,6 +318,9 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>  #ifndef __compiletime_object_size
>  # define __compiletime_object_size(obj) -1
>  #endif
> +#ifndef __compiletime_size
> +# define __compiletime_size(obj) 0
> +#endif
>  #ifndef __compiletime_warning
>  # define __compiletime_warning(message)
>  #endif
> diff --git a/include/linux/string.h b/include/linux/string.h
> index ac889c5..fc126a1 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -154,4 +154,13 @@ static inline const char *kbasename(const char *path)
>         return tail ? tail + 1 : path;
>  }
>
> +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
> +#define strcpy(dest, src) do {                                         \
> +       int len = __compiletime_size(dest);                             \
> +       if (len > 1 && strlen(src) >= len)                              \
> +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \

This should probably BUG. An overflowing strcpy shouldn't be allowed
to continue.

> +       strcpy(dest, src);                                              \
> +} while (0)
> +#endif
> +
>  #endif /* _LINUX_STRING_H_ */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 819ac51..94db086 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1431,6 +1431,15 @@ config DEBUG_STRICT_USER_COPY_CHECKS
>
>           If unsure, say N.
>
> +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
> +       bool "Strict checks for strcpy() overflows"
> +       depends on DEBUG_KERNEL
> +       help
> +         Enabling this option adds some extra sanity checks when strcpy() is
> +         called().  This will slow down the kernel a bit.

Isn't this an entirely compile-time check? I would expect it to be
entirely optimized by the compiler. In fact, could this be turned into
a build failure instead?

> +
> +         If unsure, say N.
> +
>  source kernel/trace/Kconfig
>
>  menu "Runtime Testing"

-Kees

-- 
Kees Cook
Chrome OS Security

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > +#define strcpy(dest, src) do {                                         \
> > +       int len = __compiletime_size(dest);                             \
> > +       if (len > 1 && strlen(src) >= len)                              \
> > +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
> 
> This should probably BUG. An overflowing strcpy shouldn't be allowed
> to continue.

I was worried about false positives.

Speaking of false positives the STRICT checks on copy_from_user() have
been disabled for a year now because of a three year old GCC bug.  I
wonder if the GCC people realize the security impact it has.  See
commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > +       bool "Strict checks for strcpy() overflows"
> > +       depends on DEBUG_KERNEL
> > +       help
> > +         Enabling this option adds some extra sanity checks when strcpy() is
> > +         called().  This will slow down the kernel a bit.
> 
> Isn't this an entirely compile-time check? I would expect it to be
> entirely optimized by the compiler. In fact, could this be turned into
> a build failure instead?

No.  The problem is when we don't know the size of the src string.  Also
if GCC is able to find the compile time size of both the src and
dest string then Smatch and other static checkers are able to as well so
I'm not very concerned about that case because we already catch them.

regards,
dan carpenter

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > +#define strcpy(dest, src) do {                                         \
> > +       int len = __compiletime_size(dest);                             \
> > +       if (len > 1 && strlen(src) >= len)                              \
> > +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
> 
> This should probably BUG. An overflowing strcpy shouldn't be allowed
> to continue.

I was worried about false positives.

Speaking of false positives the STRICT checks on copy_from_user() have
been disabled for a year now because of a three year old GCC bug.  I
wonder if the GCC people realize the security impact it has.  See
commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > +       bool "Strict checks for strcpy() overflows"
> > +       depends on DEBUG_KERNEL
> > +       help
> > +         Enabling this option adds some extra sanity checks when strcpy() is
> > +         called().  This will slow down the kernel a bit.
> 
> Isn't this an entirely compile-time check? I would expect it to be
> entirely optimized by the compiler. In fact, could this be turned into
> a build failure instead?

No.  The problem is when we don't know the size of the src string.  Also
if GCC is able to find the compile time size of both the src and
dest string then Smatch and other static checkers are able to as well so
I'm not very concerned about that case because we already catch them.

regards,
dan carpenter

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Kees Cook]

On Wed, Apr 30, 2014 at 9:19 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
>> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +#define strcpy(dest, src) do {                                         \
>> > +       int len = __compiletime_size(dest);                             \
>> > +       if (len > 1 && strlen(src) >= len)                              \
>> > +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
>>
>> This should probably BUG. An overflowing strcpy shouldn't be allowed
>> to continue.
>
> I was worried about false positives.

Sure, good to be initially cautious but I think if this goes in, it should BUG.

> Speaking of false positives the STRICT checks on copy_from_user() have
> been disabled for a year now because of a three year old GCC bug.  I
> wonder if the GCC people realize the security impact it has.  See
> commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
> 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

Yeah, lots of badness here. I'll see if I can find someone to look at
solutions for this.

>
>> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +       bool "Strict checks for strcpy() overflows"
>> > +       depends on DEBUG_KERNEL
>> > +       help
>> > +         Enabling this option adds some extra sanity checks when strcpy() is
>> > +         called().  This will slow down the kernel a bit.
>>
>> Isn't this an entirely compile-time check? I would expect it to be
>> entirely optimized by the compiler. In fact, could this be turned into
>> a build failure instead?
>
> No.  The problem is when we don't know the size of the src string.  Also
> if GCC is able to find the compile time size of both the src and
> dest string then Smatch and other static checkers are able to as well so
> I'm not very concerned about that case because we already catch them.

Ah, right, the source. But that shouldn't make it "slow". How about
naming this DEBUG_STRICT_STRCPY_SIZE_CHECKS or something? I can't
imagine the performance from adding a single compare to be bad. You
can even branch-hint it with "if (unlikely(...."

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Kees Cook]

On Wed, Apr 30, 2014 at 9:19 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, Apr 30, 2014 at 08:33:21AM -0700, Kees Cook wrote:
>> On Wed, Apr 30, 2014 at 8:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> > +#if CONFIG_DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +#define strcpy(dest, src) do {                                         \
>> > +       int len = __compiletime_size(dest);                             \
>> > +       if (len > 1 && strlen(src) >= len)                              \
>> > +               WARN_ONCE(1, "strcpy() overflow copying \"%s\"\n", src);        \
>>
>> This should probably BUG. An overflowing strcpy shouldn't be allowed
>> to continue.
>
> I was worried about false positives.

Sure, good to be initially cautious but I think if this goes in, it should BUG.

> Speaking of false positives the STRICT checks on copy_from_user() have
> been disabled for a year now because of a three year old GCC bug.  I
> wonder if the GCC people realize the security impact it has.  See
> commit 2fb0815c9ee6 ('gcc4: disable __compiletime_object_size for GCC
> 4.6+') and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

Yeah, lots of badness here. I'll see if I can find someone to look at
solutions for this.

>
>> > +config DEBUG_STRICT_SLOW_STRCPY_CHECKS
>> > +       bool "Strict checks for strcpy() overflows"
>> > +       depends on DEBUG_KERNEL
>> > +       help
>> > +         Enabling this option adds some extra sanity checks when strcpy() is
>> > +         called().  This will slow down the kernel a bit.
>>
>> Isn't this an entirely compile-time check? I would expect it to be
>> entirely optimized by the compiler. In fact, could this be turned into
>> a build failure instead?
>
> No.  The problem is when we don't know the size of the src string.  Also
> if GCC is able to find the compile time size of both the src and
> dest string then Smatch and other static checkers are able to as well so
> I'm not very concerned about that case because we already catch them.

Ah, right, the source. But that shouldn't make it "slow". How about
naming this DEBUG_STRICT_STRCPY_SIZE_CHECKS or something? I can't
imagine the performance from adding a single compare to be bad. You
can even branch-hint it with "if (unlikely(...."

-Kees

-- 
Kees Cook
Chrome OS Security

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Rafael J. Wysocki]

On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.  The downside is that it makes strcpy
> slower.
> 
> I introduced __compiletime_size() to make this work.  It returns the
> size of the destination buffer or zero if the size isn't known.  The
> __compiletime_object_size() is similar but if you pass it a struct
> member then it returns the size of everything from there to the end of
> the struct.  Another difference is __compiletime_object_size() returns
> -1 for unknown sizes.
> 
> If you pass a char pointer to __compiletime_size() then it returns zero.
> 
> The strcpy() check ignores buffers with just one byte because people
> often use those for variable length strings at the end of a struct.
> 
> I have tested this patch lightly and created some bugs for it to detect
> but I have not detected any real life overflows.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> index e863dd5..5e0fc2b 100644
> --- a/include/acpi/platform/acenv.h
> +++ b/include/acpi/platform/acenv.h

This is an ACPICA header and changes to it need to be submitted to the ACPICA
maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
upstream project (except for really special situations).

> @@ -320,7 +320,7 @@
>  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
>  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
>  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
>  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
>  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
>  #define ACPI_STRCMP(d,s)        strcmp((d), (s))

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Rafael J. Wysocki]

On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.  The downside is that it makes strcpy
> slower.
> 
> I introduced __compiletime_size() to make this work.  It returns the
> size of the destination buffer or zero if the size isn't known.  The
> __compiletime_object_size() is similar but if you pass it a struct
> member then it returns the size of everything from there to the end of
> the struct.  Another difference is __compiletime_object_size() returns
> -1 for unknown sizes.
> 
> If you pass a char pointer to __compiletime_size() then it returns zero.
> 
> The strcpy() check ignores buffers with just one byte because people
> often use those for variable length strings at the end of a struct.
> 
> I have tested this patch lightly and created some bugs for it to detect
> but I have not detected any real life overflows.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> index e863dd5..5e0fc2b 100644
> --- a/include/acpi/platform/acenv.h
> +++ b/include/acpi/platform/acenv.h

This is an ACPICA header and changes to it need to be submitted to the ACPICA
maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
upstream project (except for really special situations).

> @@ -320,7 +320,7 @@
>  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
>  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
>  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
>  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
>  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
>  #define ACPI_STRCMP(d,s)        strcmp((d), (s))

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 09:49:23PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> > There are sometimes where we know that we are doing an strcpy() into a
> > fixed length buffer.  In those cases, we could verify that the strcpy()
> > doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > if you want to check for that.  The downside is that it makes strcpy
> > slower.
> > 
> > I introduced __compiletime_size() to make this work.  It returns the
> > size of the destination buffer or zero if the size isn't known.  The
> > __compiletime_object_size() is similar but if you pass it a struct
> > member then it returns the size of everything from there to the end of
> > the struct.  Another difference is __compiletime_object_size() returns
> > -1 for unknown sizes.
> > 
> > If you pass a char pointer to __compiletime_size() then it returns zero.
> > 
> > The strcpy() check ignores buffers with just one byte because people
> > often use those for variable length strings at the end of a struct.
> > 
> > I have tested this patch lightly and created some bugs for it to detect
> > but I have not detected any real life overflows.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > index e863dd5..5e0fc2b 100644
> > --- a/include/acpi/platform/acenv.h
> > +++ b/include/acpi/platform/acenv.h
> 
> This is an ACPICA header and changes to it need to be submitted to the ACPICA
> maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> upstream project (except for really special situations).

Ok.  I should have added Robert and Lv to the CC list.  My guess is
that the (void) is needed to avoid compile warnings but it's needed for
us to avoid compile breakage with this change.

Anyway, I'll wait for a couple days and resend that bit broken out.

regards,
dan carpenter

> 
> > @@ -320,7 +320,7 @@
> >  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
> >  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
> >  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> > -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> > +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
> >  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
> >  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
> >  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> 
> Thanks!
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 09:49:23PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> > There are sometimes where we know that we are doing an strcpy() into a
> > fixed length buffer.  In those cases, we could verify that the strcpy()
> > doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > if you want to check for that.  The downside is that it makes strcpy
> > slower.
> > 
> > I introduced __compiletime_size() to make this work.  It returns the
> > size of the destination buffer or zero if the size isn't known.  The
> > __compiletime_object_size() is similar but if you pass it a struct
> > member then it returns the size of everything from there to the end of
> > the struct.  Another difference is __compiletime_object_size() returns
> > -1 for unknown sizes.
> > 
> > If you pass a char pointer to __compiletime_size() then it returns zero.
> > 
> > The strcpy() check ignores buffers with just one byte because people
> > often use those for variable length strings at the end of a struct.
> > 
> > I have tested this patch lightly and created some bugs for it to detect
> > but I have not detected any real life overflows.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > index e863dd5..5e0fc2b 100644
> > --- a/include/acpi/platform/acenv.h
> > +++ b/include/acpi/platform/acenv.h
> 
> This is an ACPICA header and changes to it need to be submitted to the ACPICA
> maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> upstream project (except for really special situations).

Ok.  I should have added Robert and Lv to the CC list.  My guess is
that the (void) is needed to avoid compile warnings but it's needed for
us to avoid compile breakage with this change.

Anyway, I'll wait for a couple days and resend that bit broken out.

regards,
dan carpenter

> 
> > @@ -320,7 +320,7 @@
> >  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
> >  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
> >  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> > -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> > +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
> >  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
> >  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
> >  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> 
> Thanks!
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 11:15:29PM +0300, Dan Carpenter wrote:
> > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > > index e863dd5..5e0fc2b 100644
> > > --- a/include/acpi/platform/acenv.h
> > > +++ b/include/acpi/platform/acenv.h
> > 
> > This is an ACPICA header and changes to it need to be submitted to the ACPICA
> > maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> > upstream project (except for really special situations).
> 
> Ok.  I should have added Robert and Lv to the CC list.  My guess is
> that the (void) is needed to avoid compile warnings but it's needed for
> us to avoid compile breakage with this change.
> 
> Anyway, I'll wait for a couple days and resend that bit broken out.
> 

In the end, I won't need to modify the ACPICA headers if I use an
expression statement ({ ... }) instead of a do { } while (0) statement.

Thanks, though.  :)

regards,
dan carpenter

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

On Wed, Apr 30, 2014 at 11:15:29PM +0300, Dan Carpenter wrote:
> > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > > index e863dd5..5e0fc2b 100644
> > > --- a/include/acpi/platform/acenv.h
> > > +++ b/include/acpi/platform/acenv.h
> > 
> > This is an ACPICA header and changes to it need to be submitted to the ACPICA
> > maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> > upstream project (except for really special situations).
> 
> Ok.  I should have added Robert and Lv to the CC list.  My guess is
> that the (void) is needed to avoid compile warnings but it's needed for
> us to avoid compile breakage with this change.
> 
> Anyway, I'll wait for a couple days and resend that bit broken out.
> 

In the end, I won't need to modify the ACPICA headers if I use an
expression statement ({ ... }) instead of a do { } while (0) statement.

Thanks, though.  :)

regards,
dan carpenter

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Solar Designer]

On Wed, Apr 30, 2014 at 06:08:44PM +0300, Dan Carpenter wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.

FWIW, I had posted similar macros for userland strcpy() and friends to
the security-audit list (now defunct) in 1998.  Someone preserved a copy
here (although the indentation is lost):

http://www.opennet.ru/soft/0432.html

In (weird) use, with proper indentation:

http://www.merit.edu/mail.archives/nanog/2000-02/msg00366.html
https://github.com/tureba/trinoo/blob/master/strfix.h

Personally, I was using this at the time for building known-broken
software like wu-ftpd, where the risk of false positives felt lower than
the risk of buffer overflow bugs being in fact present in the code.

I used gcc's Statement Exprs extension, which is also used in the Linux
kernel a lot:

http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

So maybe you should, too.  (That is, if you want to go ahead with this
approach for code that isn't meant to be as broken as wu-ftpd was.)
This lets us propagate the original return value.

To determine the destination buffer size, I simply used sizeof() and
skipped my added protection in case the size looked like that of a
pointer.  Now you have those nice new gcc features instead. :-)

> The downside is that it makes strcpy slower.

I guess the slowdown is mostly from the added strlen().  I avoided it by
using strncat(), so I had truncation instead of detection.  It is
unclear which is better.

Other functions I did this for are strcat(), sprintf(), vsprintf().

Alexander

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Solar Designer]

On Wed, Apr 30, 2014 at 06:08:44PM +0300, Dan Carpenter wrote:
> There are sometimes where we know that we are doing an strcpy() into a
> fixed length buffer.  In those cases, we could verify that the strcpy()
> doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> if you want to check for that.

FWIW, I had posted similar macros for userland strcpy() and friends to
the security-audit list (now defunct) in 1998.  Someone preserved a copy
here (although the indentation is lost):

http://www.opennet.ru/soft/0432.html

In (weird) use, with proper indentation:

http://www.merit.edu/mail.archives/nanog/2000-02/msg00366.html
https://github.com/tureba/trinoo/blob/master/strfix.h

Personally, I was using this at the time for building known-broken
software like wu-ftpd, where the risk of false positives felt lower than
the risk of buffer overflow bugs being in fact present in the code.

I used gcc's Statement Exprs extension, which is also used in the Linux
kernel a lot:

http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

So maybe you should, too.  (That is, if you want to go ahead with this
approach for code that isn't meant to be as broken as wu-ftpd was.)
This lets us propagate the original return value.

To determine the destination buffer size, I simply used sizeof() and
skipped my added protection in case the size looked like that of a
pointer.  Now you have those nice new gcc features instead. :-)

> The downside is that it makes strcpy slower.

I guess the slowdown is mostly from the added strlen().  I avoided it by
using strncat(), so I had truncation instead of detection.  It is
unclear which is better.

Other functions I did this for are strcat(), sprintf(), vsprintf().

Alexander

[kernel-hardening] Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

Ah.  Fantastic.  That's all great stuff.  I'm on holiday today but I'll
send a new later in the week.

regards,
dan carpenter

Re: [patch] lib: check for strcpy() overflows to fixed length buffers

[Dan Carpenter]

Ah.  Fantastic.  That's all great stuff.  I'm on holiday today but I'll
send a new later in the week.

regards,
dan carpenter

Re: [Devel] [patch] lib: check for strcpy() overflows to fixed length buffers

[Zheng, Lv]

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

Hi,

> From: Dan Carpenter [mailto:dan.carpenter(a)oracle.com]
> Sent: Thursday, May 01, 2014 4:15 AM
> 
> On Wed, Apr 30, 2014 at 09:49:23PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> > > There are sometimes where we know that we are doing an strcpy() into a
> > > fixed length buffer.  In those cases, we could verify that the strcpy()
> > > doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > > if you want to check for that.  The downside is that it makes strcpy
> > > slower.
> > >
> > > I introduced __compiletime_size() to make this work.  It returns the
> > > size of the destination buffer or zero if the size isn't known.  The
> > > __compiletime_object_size() is similar but if you pass it a struct
> > > member then it returns the size of everything from there to the end of
> > > the struct.  Another difference is __compiletime_object_size() returns
> > > -1 for unknown sizes.
> > >
> > > If you pass a char pointer to __compiletime_size() then it returns zero.
> > >
> > > The strcpy() check ignores buffers with just one byte because people
> > > often use those for variable length strings at the end of a struct.
> > >
> > > I have tested this patch lightly and created some bugs for it to detect
> > > but I have not detected any real life overflows.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com>
> > >
> > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > > index e863dd5..5e0fc2b 100644
> > > --- a/include/acpi/platform/acenv.h
> > > +++ b/include/acpi/platform/acenv.h
> >
> > This is an ACPICA header and changes to it need to be submitted to the ACPICA
> > maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> > upstream project (except for really special situations).
> 
> Ok.  I should have added Robert and Lv to the CC list.  My guess is
> that the (void) is needed to avoid compile warnings but it's needed for
> us to avoid compile breakage with this change.

In normal ACPICA build environment, I didn't suffer from new build errors after deleting this "void".
But this might be required by lint users.
You can split ACPICA changes into a separate patch so that it could be easily reverted if someone complained.

Thanks
-Lv

> 
> Anyway, I'll wait for a couple days and resend that bit broken out.
> 
> regards,
> dan carpenter
> 
> >
> > > @@ -320,7 +320,7 @@
> > >  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
> > >  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
> > >  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> > > -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> > > +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
> > >  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> >
> > Thanks!
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [patch] lib: check for strcpy() overflows to fixed length buffers

[Zheng, Lv]

Hi,

> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, May 01, 2014 4:15 AM
> 
> On Wed, Apr 30, 2014 at 09:49:23PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> > > There are sometimes where we know that we are doing an strcpy() into a
> > > fixed length buffer.  In those cases, we could verify that the strcpy()
> > > doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > > if you want to check for that.  The downside is that it makes strcpy
> > > slower.
> > >
> > > I introduced __compiletime_size() to make this work.  It returns the
> > > size of the destination buffer or zero if the size isn't known.  The
> > > __compiletime_object_size() is similar but if you pass it a struct
> > > member then it returns the size of everything from there to the end of
> > > the struct.  Another difference is __compiletime_object_size() returns
> > > -1 for unknown sizes.
> > >
> > > If you pass a char pointer to __compiletime_size() then it returns zero.
> > >
> > > The strcpy() check ignores buffers with just one byte because people
> > > often use those for variable length strings at the end of a struct.
> > >
> > > I have tested this patch lightly and created some bugs for it to detect
> > > but I have not detected any real life overflows.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > > index e863dd5..5e0fc2b 100644
> > > --- a/include/acpi/platform/acenv.h
> > > +++ b/include/acpi/platform/acenv.h
> >
> > This is an ACPICA header and changes to it need to be submitted to the ACPICA
> > maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> > upstream project (except for really special situations).
> 
> Ok.  I should have added Robert and Lv to the CC list.  My guess is
> that the (void) is needed to avoid compile warnings but it's needed for
> us to avoid compile breakage with this change.

In normal ACPICA build environment, I didn't suffer from new build errors after deleting this "void".
But this might be required by lint users.
You can split ACPICA changes into a separate patch so that it could be easily reverted if someone complained.

Thanks
-Lv

> 
> Anyway, I'll wait for a couple days and resend that bit broken out.
> 
> regards,
> dan carpenter
> 
> >
> > > @@ -320,7 +320,7 @@
> > >  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
> > >  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
> > >  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> > > -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> > > +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
> > >  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> >
> > Thanks!
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.

[kernel-hardening] RE: [patch] lib: check for strcpy() overflows to fixed length buffers

[Zheng, Lv]

Hi,

> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, May 01, 2014 4:15 AM
> 
> On Wed, Apr 30, 2014 at 09:49:23PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, April 30, 2014 06:08:44 PM Dan Carpenter wrote:
> > > There are sometimes where we know that we are doing an strcpy() into a
> > > fixed length buffer.  In those cases, we could verify that the strcpy()
> > > doesn't overflow.  This patch introduces DEBUG_STRICT_SLOW_STRCPY_CHECKS
> > > if you want to check for that.  The downside is that it makes strcpy
> > > slower.
> > >
> > > I introduced __compiletime_size() to make this work.  It returns the
> > > size of the destination buffer or zero if the size isn't known.  The
> > > __compiletime_object_size() is similar but if you pass it a struct
> > > member then it returns the size of everything from there to the end of
> > > the struct.  Another difference is __compiletime_object_size() returns
> > > -1 for unknown sizes.
> > >
> > > If you pass a char pointer to __compiletime_size() then it returns zero.
> > >
> > > The strcpy() check ignores buffers with just one byte because people
> > > often use those for variable length strings at the end of a struct.
> > >
> > > I have tested this patch lightly and created some bugs for it to detect
> > > but I have not detected any real life overflows.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/include/acpi/platform/acenv.h b/include/acpi/platform/acenv.h
> > > index e863dd5..5e0fc2b 100644
> > > --- a/include/acpi/platform/acenv.h
> > > +++ b/include/acpi/platform/acenv.h
> >
> > This is an ACPICA header and changes to it need to be submitted to the ACPICA
> > maintainers (as per MAINTAINERS).  We only get ACPICA changes from the
> > upstream project (except for really special situations).
> 
> Ok.  I should have added Robert and Lv to the CC list.  My guess is
> that the (void) is needed to avoid compile warnings but it's needed for
> us to avoid compile breakage with this change.

In normal ACPICA build environment, I didn't suffer from new build errors after deleting this "void".
But this might be required by lint users.
You can split ACPICA changes into a separate patch so that it could be easily reverted if someone complained.

Thanks
-Lv

> 
> Anyway, I'll wait for a couple days and resend that bit broken out.
> 
> regards,
> dan carpenter
> 
> >
> > > @@ -320,7 +320,7 @@
> > >  #define ACPI_STRSTR(s1,s2)      strstr((s1), (s2))
> > >  #define ACPI_STRCHR(s1,c)       strchr((s1), (c))
> > >  #define ACPI_STRLEN(s)          (acpi_size) strlen((s))
> > > -#define ACPI_STRCPY(d,s)        (void) strcpy((d), (s))
> > > +#define ACPI_STRCPY(d,s)        strcpy((d), (s))
> > >  #define ACPI_STRNCPY(d,s,n)     (void) strncpy((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRNCMP(d,s,n)     strncmp((d), (s), (acpi_size)(n))
> > >  #define ACPI_STRCMP(d,s)        strcmp((d), (s))
> >
> > Thanks!
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.