| Message ID | 20140430150844.GA10621@mwanda (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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!
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"
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> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html