Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1696760 > unrolled thread

[PATCH] fortify: Use WARN instead of BUG for now

Back to article view | Back to linux.kernel

Contents

  [PATCH] fortify: Use WARN instead of BUG for now Kees Cook <keescook@chromium.org> - 2017-07-26 06:00 +0200
    Re: [PATCH] fortify: Use WARN instead of BUG for now Daniel Micay <danielmicay@gmail.com> - 2017-07-26 15:00 +0200
      Re: [PATCH] fortify: Use WARN instead of BUG for now Kees Cook <keescook@chromium.org> - 2017-07-26 19:30 +0200
        Re: [PATCH] fortify: Use WARN instead of BUG for now Daniel Micay <danielmicay@gmail.com> - 2017-07-26 20:00 +0200
    Re: [PATCH] fortify: Use WARN instead of BUG for now Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-26 19:20 +0200
      Re: [PATCH] fortify: Use WARN instead of BUG for now Kees Cook <keescook@chromium.org> - 2017-07-26 19:20 +0200

#1696760 — [PATCH] 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.

Cc: Daniel Micay <danielmicay@gmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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

[toc] | [next] | [standalone]

#1697070

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.

[toc] | [prev] | [next] | [standalone]

#1697417

On Wed, Jul 26, 2017 at 5:52 AM, Daniel Micay <danielmicay@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1697459

> 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.

[toc] | [prev] | [next] | [standalone]

#1697386

On Tue, Jul 25, 2017 at 8:50 PM, Kees Cook <keescook@chromium.org> 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

[toc] | [prev] | [next] | [standalone]

#1697393

On Wed, Jul 26, 2017 at 10:10 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jul 25, 2017 at 8:50 PM, Kees Cook <keescook@chromium.org> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web