fortify: Use WARN instead of BUG for now
Subsystems (2)
PATCH 1/1 fortify: Use WARN instead of BUG for now
While CONFIG_FORTIFY_SOURCE continues to shake out, don't unconditionally use BUG(), opting instead for WARN(). At the same time, expand the runtime detection to provide a better hint about what went wrong.
---
Sending to akpm, since fortify went through -mm originally.
--- include/linux/string.h | 48 ++++++++++++++++++++++++++++++------------------ lib/string.c | 19 +++++++++++++++---- 2 files changed, 45 insertions(+), 22 deletions(-)
include/linux/string.h +30 -18
@@ -197,7 +197,10 @@ static inline const char *kbasename(const char *path)#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))#define __RENAME (x) __asm__(#x) -void fortify_panic (const char *name)__noreturn __cold ; +void fortify_read_overflow (const char *func)__cold ; +void fortify_read_overflow2 (const char *func)__cold ; +void fortify_write_overflow (const char *func)__cold ; + void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter"); void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter"); void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter");
@@ -209,7 +212,7 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)if (__builtin_constant_p (size) && p_size < size)__write_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_write_overflow (__func__);return __builtin_strncpy (p, q, size); }
@@ -219,7 +222,7 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)if (p_size == (size_t )-1 )return __builtin_strcat (p, q);if (strlcat (p, q, p_size) >= p_size) -fortify_panic (__func__); +fortify_write_overflow (__func__);return p; }
@@ -231,7 +234,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)return __builtin_strlen (p); ret =strnlen (p, p_size);if (p_size <= ret) -fortify_panic (__func__); +fortify_read_overflow (__func__);return ret; }
@@ -241,7 +244,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)size_t p_size =__builtin_object_size (p,0 );__kernel_size_t ret =__real_strnlen (p, maxlen < p_size ? maxlen : p_size);if (p_size <= ret && maxlen != ret) -fortify_panic (__func__); +fortify_read_overflow (__func__);return ret; }
@@ -260,7 +263,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)if (__builtin_constant_p (len) && len >= p_size)__write_overflow ();if (len >= p_size) -fortify_panic (__func__); +fortify_write_overflow (__func__);__builtin_memcpy (p, q, len); p[len] ='\0' ; }
@@ -278,7 +281,7 @@ __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count) p_len =strlen (p); copy_len =strnlen (q, count);if (p_size < p_len + copy_len +1 ) -fortify_panic (__func__); +fortify_write_overflow (__func__);__builtin_memcpy (p + p_len, q, copy_len); p[p_len + copy_len] ='\0' ;return p;
@@ -290,7 +293,7 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)if (__builtin_constant_p (size) && p_size < size)__write_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_write_overflow (__func__);return __builtin_memset (p, c, size); }
@@ -303,9 +306,12 @@ __FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)__write_overflow ();if (q_size < size)__read_overflow2 (); + }else { +if (p_size < size) +fortify_write_overflow (__func__); +if (q_size < size) +fortify_read_overflow2 (__func__); } -if (p_size < size || q_size < size) -fortify_panic (__func__);return __builtin_memcpy (p, q, size); }
@@ -318,9 +324,12 @@ __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size)__write_overflow ();if (q_size < size)__read_overflow2 (); + }else { +if (p_size < size) +fortify_write_overflow (__func__); +if (q_size < size) +fortify_read_overflow2 (__func__); } -if (p_size < size || q_size < size) -fortify_panic (__func__);return __builtin_memmove (p, q, size); }
@@ -331,7 +340,7 @@ __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)if (__builtin_constant_p (size) && p_size < size)__read_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_read_overflow (__func__);return __real_memscan (p, c, size); }
@@ -344,9 +353,12 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)__read_overflow ();if (q_size < size)__read_overflow2 (); + }else { +if (p_size < size) +fortify_read_overflow (__func__); +if (q_size < size) +fortify_read_overflow2 (__func__); } -if (p_size < size || q_size < size) -fortify_panic (__func__);return __builtin_memcmp (p, q, size); }
@@ -356,7 +368,7 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)if (__builtin_constant_p (size) && p_size < size)__read_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_read_overflow (__func__);return __builtin_memchr (p, c, size); }
@@ -367,7 +379,7 @@ __FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)if (__builtin_constant_p (size) && p_size < size)__read_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_read_overflow (__func__);return __real_memchr_inv (p, c, size); }
@@ -378,7 +390,7 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)if (__builtin_constant_p (size) && p_size < size)__read_overflow ();if (p_size < size) -fortify_panic (__func__); +fortify_read_overflow (__func__);return __real_kmemdup (p, size, gfp); }
lib/string.c +15 -4
@@ -979,9 +979,20 @@ char *strreplace(char *s, char old, char new) }EXPORT_SYMBOL (strreplace); -void fortify_panic (const char *name) +void fortify_read_overflow (const char *func) { -pr_emerg ("detected buffer overflow in %s\n" , name); -BUG (); +WARN (1 ,"detected read beyond size of object passed as 1st parameter in %s\n" , func); } -EXPORT_SYMBOL (fortify_panic); +EXPORT_SYMBOL (fortify_read_overflow); + +void fortify_read_overflow2 (const char *func) +{ +WARN (1 ,"detected read beyond size of object passed as 2nd parameter in %s\n" , func); +} +EXPORT_SYMBOL (fortify_read_overflow2); + +void fortify_write_overflow (const char *func) +{ +WARN (1 ,"detected write beyond size of object passed as 1st parameter in %s\n" , func); +} +EXPORT_SYMBOL (fortify_write_overflow);
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
It should just be renamed from fortify_panic -> fortify_error, including in arch/x86/boot/compressed/misc.c and arch/x86/boot/compressed/misc.c. It can use WARN instead of BUG by with a 'default n', !COMPILE_TEST option to use BUG again. Otherwise it needs to be patched downstream when that's wanted.
I don't think splitting it is the right approach to improving the runtime error handling. That only makes sense for the compile-time errors due to the limitations of __attribute__((error)). Can we think about that before changing it? Just make it use WARN for now.
The best debugging experience would be passing along the sizes and having the fortify_error function convert that into nice error messages. For memcpy(p, q, n), n can be larger than both the detected sizes of p and q, not just either one. The error should just be saying the function name and printing the copy size and maximum sizes of p and q. That's going to increase the code size too but I think splitting it will be worse and it goes in the wrong direction in terms of complexity. It's going to make future extensions / optimization harder if it's split.
Quoted 2 lines · Daniel Micay
It should just be renamed from
fortify_panic->fortify_error, including inarch/x86/boot/compressed/misc.candarch/x86/boot/compressed/misc.c.
Somehow I missed these. I'll send a v2. I wonder why those didn't trip in my build...
Quoted 3 lines
It can use WARN instead of BUG by with a 'default n', !COMPILE_TEST option to use BUG again. Otherwise it needs to be patched downstream when that's wanted.
I figure that'll be a separate conversation. For now, we'll do WARN.
Quoted 4 lines
I don't think splitting it is the right approach to improving the runtime error handling. That only makes sense for the compile-time errors due to the limitations of __attribute__((error)). Can we think about that before changing it? Just make it use WARN for now.
Part of Linus's objection was the vague error report. Since the split didn't bloat things very much, it seemed okay to me.
Quoted 8 lines
The best debugging experience would be passing along the sizes and having the
fortify_errorfunction convert that into nice error messages. Formemcpy(p, q, n), n can be larger than both the detected sizes of p and q, not just either one. The error should just be saying the function name and printing the copy size and maximum sizes of p and q. That's going to increase the code size too but I think splitting it will be worse and it goes in the wrong direction in terms of complexity. It's going to make future extensions / optimization harder if it's split.
Maybe we could do two phases? One to s/BUG/WARN/ and the second to improve the message?
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 2 lines
Maybe we could do two phases? One to s/BUG/WARN/ and the second to improve the message?
s/fortify_panic/fortify_overflow/ + use WARN + remove __noreturn makes sense as one commit. Still think the *option* of __noreturn + BUG should be kept there even just for measuring the size overhead. !COMPILE_TIME && EXPERT if it needs to be for now. If you're fully removing __noreturn then the entry in tools/objtool/check.c for __noreturn functions also won't make sense (either way it needs to use the new name).
I think improving error messages should be done a bit differently though and it'll be easier to not tie these things together.
lib/string.c char *strreplace(char *s, char old, char new) · line 985
Quoted 7 lines · Kees Cook
lib/string.c +3
-2+ +void fortify_read_overflow (const char *func) { -pr_emerg ("detected buffer overflow in %s\n" , name); -BUG (); +WARN (1 ,"detected read beyond size of object passed as 1st parameter in %s\n" , func); }
Side note: have you actually checked the code generation of this all?
In particular, do you have any reason to use the out-of-line functions? Our WARN() code isn't horrible, and isn't likely to be noticeably worse than your own explicit out-of-lining. And you'd get the "unlikely()" for free, so you'll possibly get smaller code that runs better too.
And it would even *look* better. This:
if (p_size < size)
fortify_read_overflow(__func__);
would become
WARN(p_size < size, "kmemdup size overflow");
or something.
Linus
Quoted 19 lines · Linus Torvalds
lib/string.c char *strreplace(char *s, char old, char new) · line 985
Quoted 7 lines · Kees Cook
lib/string.c +3
-2+ +void fortify_read_overflow (const char *func) { -pr_emerg ("detected buffer overflow in %s\n" , name); -BUG (); +WARN (1 ,"detected read beyond size of object passed as 1st parameter in %s\n" , func); }Side note: have you actually checked the code generation of this all?
In particular, do you have any reason to use the out-of-line functions? Our
WARN()code isn't horrible, and isn't likely to be noticeably worse than your own explicit out-of-lining. And you'd get the "unlikely()" for free, so you'll possibly get smaller code that runs better too.And it would even *look* better. This:
Quoted 8 lines
if (
p_size< size)
fortify_read_overflow(__func__);would become
WARN(p_size < size, "kmemdup size overflow");or something.
I did, yeah. It's actually slightly smaller code size to out-of-line these:
$ size vmlinux.fortify* text data bss dec hex filename 10903767 5605009 13930496 30439272 1d07768 vmlinux.fortify-off 10944795 5617801 13930496 30493092 1d149a4 vmlinux.fortify 10950117 5626725 13930496 30507338 1d1814a vmlinux.fortify-inline
If the readability improvement is preferred over the growth in size, I can certainly respin it.
-Kees
Signature · 2 lines
Kees Cook Pixel Security
kbuild test robot
build boturl: https://github.com/0day-ci/linux/commits/Kees-Cook/fortify-Use-WARN-instead-of-BUG-for-now/20170727-101839 config: i386-allmodconfig (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0…
Hi Kees,
[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc2 next-20170726]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kees-Cook/fortify-Use-WARN-instead-of-BUG-for-now/20170727-101839
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 All errors (new ones prefixed by >>):
In file included from arch/x86/include/asm/page_32.h:34:0,
from arch/x86/include/asm/page.h:13,
from arch/x86/include/asm/thread_info.h:11,
from include/linux/thread_info.h:37,
from arch/x86/include/asm/preempt.h:6,
from include/linux/preempt.h:80,
from include/linux/spinlock.h:50,
from include/linux/wait.h:8,
from include/linux/wait_bit.h:7,
from include/linux/fs.h:5,
from include/linux/buffer_head.h:11,
from fs/adfs/dir_f.c:12:
In function 'memcpy',
inlined from '__adfs_dir_put' at fs/adfs/dir_f.c:318:2,
inlined from 'adfs_f_update' at fs/adfs/dir_f.c:403:2:
>> include/linux/string.h:308:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
__read_overflow2();
^~~~~~~~~~~~~~~~~~vim +/__read_overflow2 +308 include/linux/string.h
6974f0c4 Daniel Micay 2017-07-12 299
6974f0c4 Daniel Micay 2017-07-12 300 __FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size)
6974f0c4 Daniel Micay 2017-07-12 301 {
6974f0c4 Daniel Micay 2017-07-12 302 size_t p_size = __builtin_object_size(p, 0);
6974f0c4 Daniel Micay 2017-07-12 303 size_t q_size = __builtin_object_size(q, 0);
6974f0c4 Daniel Micay 2017-07-12 304 if (__builtin_constant_p(size)) {
6974f0c4 Daniel Micay 2017-07-12 305 if (p_size < size)
6974f0c4 Daniel Micay 2017-07-12 306 __write_overflow();
6974f0c4 Daniel Micay 2017-07-12 307 if (q_size < size)
6974f0c4 Daniel Micay 2017-07-12 @308 __read_overflow2();
acf23d09 Kees Cook 2017-07-25 309 } else {
acf23d09 Kees Cook 2017-07-25 310 if (p_size < size)
acf23d09 Kees Cook 2017-07-25 311 fortify_write_overflow(__func__);
acf23d09 Kees Cook 2017-07-25 312 if (q_size < size)
acf23d09 Kees Cook 2017-07-25 313 fortify_read_overflow2(__func__);
6974f0c4 Daniel Micay 2017-07-12 314 }
6974f0c4 Daniel Micay 2017-07-12 315 return __builtin_memcpy(p, q, size);
6974f0c4 Daniel Micay 2017-07-12 316 }6974f0c4 Daniel Micay 2017-07-12 317
:::::: The code at line 308 was first introduced by commit
:::::: 6974f0c4555e285ab217cee58b6e874f776ff409 include/linux/string.h: add the option of fortified string.h functions
:::::: TO: Daniel Micay <[email protected]> :::::: CC: Linus Torvalds <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
I think the 'else' added in the proposed patch makes it too complicated for GCC to optimize out the __attribute__((error)) checks before they're considered to be errors. It's not needed so it's probably best to just avoid doing something like that. The runtime checks can't get false positives from overly complex code but the compile-time ones depend on GCC being able to reliably optimize them out.
This might be easier for GCC:
if (__builtin_constant_p(size) && condition_a) {
compiletimeerror();
}if (__builtin_constant_p(size) && condition_b) {
compiletimeerror();
}than the current:
if (__builtin_constant_p(size)) {
if (condition_a) {
compiletimeerror();
}
if (condition_b) {
compiletimeerror();
}
}but it hasn't had a false positive like that with the current code.
Removing __noreturn is making the inline code more complex from GCC's perspective too, but hopefully it's neither reducing coverage (i.e. not making it less able to resolve __builtin_object_size - intuitively it shouldn't impact it much but you never know) or making GCC unable to deal with the compile-time checks.