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.
Cc: Daniel Micay <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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(-)
diff --git a/include/linux/string.h b/include/linux/string.h
index a467e617eeb0..97468047b965 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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);
}
diff --git a/lib/string.c b/lib/string.c
index ebbb99c775bd..0fb68ec9a455 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -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);
--
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.
On Tue, Jul 25, 2017 at 8:50 PM, Kees Cook <[email protected]> wrote:
> +
> +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
On Wed, Jul 26, 2017 at 10:10 AM, Linus Torvalds
<[email protected]> wrote:
> On Tue, Jul 25, 2017 at 8:50 PM, Kees Cook <[email protected]> wrote:
>> +
>> +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.
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
--
Kees Cook
Pixel Security
On Wed, Jul 26, 2017 at 5:52 AM, Daniel Micay <[email protected]> wrote:
> 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.
Somehow I missed these. I'll send a v2. I wonder why those didn't trip
in my build...
> 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.
> 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.
> 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.
Maybe we could do two phases? One to s/BUG/WARN/ and the second to
improve the message?
-Kees
--
Kees Cook
Pixel Security
> 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.
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.