Improve the reporting of buffer overflows under CONFIG_FORTIFY_SOURCE to
help accelerate debugging efforts. The calculations are all just sitting
in registers anyway, so pass them along to the function to be reported.
For example, before:
detected buffer overflow in memcpy
and after:
memcpy: detected buffer overflow: 4096 byte read from buffer of size 1
Cc: Jakub Kicinski <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Nicolas Schier <[email protected]>
Cc: Tom Rix <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Miroslav Benes <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Cezary Rojewski <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Puyou Lu <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/fortify-string.h | 51 +++++++++++++++++++---------------
lib/string_helpers.c | 5 ++--
2 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index c9de1f59ee80..981e2838f99a 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -9,7 +9,7 @@
#define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
#define __RENAME(x) __asm__(#x)
-void fortify_panic(const char *name) __noreturn __cold;
+void fortify_panic(const char *name, bool dest, size_t avail, size_t len) __noreturn __cold;
void __read_overflow(void) __compiletime_error("detected read beyond size of object (1st parameter)");
void __read_overflow2(void) __compiletime_error("detected read beyond size of object (2nd parameter)");
void __read_overflow2_field(size_t avail, size_t wanted) __compiletime_warning("detected read beyond size of field (2nd parameter); maybe use struct_group()?");
@@ -147,7 +147,7 @@ char *strncpy(char * const POS p, const char *q, __kernel_size_t size)
if (__compiletime_lessthan(p_size, size))
__write_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
return __underlying_strncpy(p, q, size);
}
@@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
char *strcat(char * const POS p, const char *q)
{
size_t p_size = __member_size(p);
+ size_t size;
if (p_size == SIZE_MAX)
return __underlying_strcat(p, q);
- if (strlcat(p, q, p_size) >= p_size)
- fortify_panic(__func__);
+ size = strlcat(p, q, p_size);
+ if (p_size < size)
+ fortify_panic(__func__, 1, p_size, size);
return p;
}
@@ -205,7 +207,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
/* Do not check characters beyond the end of p. */
ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
if (p_size <= ret && maxlen != ret)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, ret);
return ret;
}
@@ -241,7 +243,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
return __underlying_strlen(p);
ret = strnlen(p, p_size);
if (p_size <= ret)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, ret);
return ret;
}
@@ -282,8 +284,8 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
__write_overflow();
}
if (size) {
- if (len >= p_size)
- fortify_panic(__func__);
+ if (p_size < len)
+ fortify_panic(__func__, 1, p_size, len);
__underlying_memcpy(p, q, len);
p[len] = '\0';
}
@@ -361,7 +363,7 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
* p_size.
*/
if (len > p_size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, len);
/*
* We can now safely call vanilla strscpy because we are protected from:
@@ -397,13 +399,15 @@ char *strncat(char * const POS p, const char * const POS q, __kernel_size_t coun
size_t p_len, copy_len;
size_t p_size = __member_size(p);
size_t q_size = __member_size(q);
+ size_t size;
if (p_size == SIZE_MAX && q_size == SIZE_MAX)
return __underlying_strncat(p, q, count);
p_len = strlen(p);
copy_len = strnlen(q, count);
- if (p_size < p_len + copy_len + 1)
- fortify_panic(__func__);
+ size = p_len + copy_len + 1;
+ if (p_size < size)
+ fortify_panic(__func__, 1, p_size, size);
__underlying_memcpy(p + p_len, q, copy_len);
p[p_len + copy_len] = '\0';
return p;
@@ -444,7 +448,7 @@ __FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
* lengths are unknown.)
*/
if (p_size != SIZE_MAX && p_size < size)
- fortify_panic("memset");
+ fortify_panic("memset", 1, p_size, size);
}
#define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({ \
@@ -542,9 +546,10 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
* (The SIZE_MAX test is to optimize away checks where the buffer
* lengths are unknown.)
*/
- if ((p_size != SIZE_MAX && p_size < size) ||
- (q_size != SIZE_MAX && q_size < size))
- fortify_panic(func);
+ if (p_size != SIZE_MAX && p_size < size)
+ fortify_panic(func, 1, p_size, size);
+ if (q_size != SIZE_MAX && q_size < size)
+ fortify_panic(func, 0, q_size, size);
/*
* Warn when writing beyond destination field size.
@@ -644,7 +649,7 @@ __FORTIFY_INLINE void *memscan(void * const POS0 p, int c, __kernel_size_t size)
if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
return __real_memscan(p, c, size);
}
@@ -660,8 +665,10 @@ int memcmp(const void * const POS0 p, const void * const POS0 q, __kernel_size_t
if (__compiletime_lessthan(q_size, size))
__read_overflow2();
}
- if (p_size < size || q_size < size)
- fortify_panic(__func__);
+ if (p_size < size)
+ fortify_panic(__func__, 1, p_size, size);
+ if (q_size < size)
+ fortify_panic(__func__, 0, q_size, size);
return __underlying_memcmp(p, q, size);
}
@@ -673,7 +680,7 @@ void *memchr(const void * const POS0 p, int c, __kernel_size_t size)
if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
return __underlying_memchr(p, c, size);
}
@@ -685,7 +692,7 @@ __FORTIFY_INLINE void *memchr_inv(const void * const POS0 p, int c, size_t size)
if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
return __real_memchr_inv(p, c, size);
}
@@ -698,7 +705,7 @@ __FORTIFY_INLINE void *kmemdup(const void * const POS0 p, size_t size, gfp_t gfp
if (__compiletime_lessthan(p_size, size))
__read_overflow();
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
return __real_kmemdup(p, size, gfp);
}
@@ -735,7 +742,7 @@ char *strcpy(char * const POS p, const char * const POS q)
__write_overflow();
/* Run-time check for dynamic size overflow. */
if (p_size < size)
- fortify_panic(__func__);
+ fortify_panic(__func__, 1, p_size, size);
__underlying_memcpy(p, q, size);
return p;
}
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 230020a2e076..b2d3e1d3931e 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -1021,9 +1021,10 @@ EXPORT_SYMBOL(__read_overflow2_field);
void __write_overflow_field(size_t avail, size_t wanted) { }
EXPORT_SYMBOL(__write_overflow_field);
-void fortify_panic(const char *name)
+void fortify_panic(const char *name, bool dest, size_t avail, size_t len)
{
- pr_emerg("detected buffer overflow in %s\n", name);
+ pr_emerg("%s: detected buffer overflow: %zu byte %s buffer of size %zu\n",
+ name, len, dest ? "write to" : "read from", avail);
BUG();
}
EXPORT_SYMBOL(fortify_panic);
--
2.34.1
On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <[email protected]> wrote:
>
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index c9de1f59ee80..981e2838f99a 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
> char *strcat(char * const POS p, const char *q)
> {
> size_t p_size = __member_size(p);
> + size_t size;
>
> if (p_size == SIZE_MAX)
> return __underlying_strcat(p, q);
> - if (strlcat(p, q, p_size) >= p_size)
> - fortify_panic(__func__);
> + size = strlcat(p, q, p_size);
> + if (p_size < size)
What happens when they're equal? I think this patch changes
behavior...? Intentional?
Did flipping this conditional drop what should be `<=`?
Was there an off by one, or is this version of this patch potentially
introducing one? Or am I misremembering my boolean algebra?
> + fortify_panic(__func__, 1, p_size, size);
> return p;
> }
>
> @@ -205,7 +207,7 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size
> /* Do not check characters beyond the end of p. */
> ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> if (p_size <= ret && maxlen != ret)
> - fortify_panic(__func__);
> + fortify_panic(__func__, 1, p_size, ret);
> return ret;
> }
>
> @@ -241,7 +243,7 @@ __kernel_size_t __fortify_strlen(const char * const POS p)
> return __underlying_strlen(p);
> ret = strnlen(p, p_size);
> if (p_size <= ret)
> - fortify_panic(__func__);
> + fortify_panic(__func__, 1, p_size, ret);
> return ret;
> }
>
> @@ -282,8 +284,8 @@ __FORTIFY_INLINE size_t strlcpy(char * const POS p, const char * const POS q, si
> __write_overflow();
> }
> if (size) {
> - if (len >= p_size)
> - fortify_panic(__func__);
> + if (p_size < len)
`<=` ? (This used to panic when they were equal)
> + fortify_panic(__func__, 1, p_size, len);
> __underlying_memcpy(p, q, len);
> p[len] = '\0';
> }
--
Thanks,
~Nick Desaulniers
On March 2, 2023 3:21:11 PM PST, Nick Desaulniers <[email protected]> wrote:
>On Thu, Mar 2, 2023 at 2:58 PM Kees Cook <[email protected]> wrote:
>>
>> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
>> index c9de1f59ee80..981e2838f99a 100644
>> --- a/include/linux/fortify-string.h
>> +++ b/include/linux/fortify-string.h
>> @@ -170,11 +170,13 @@ __FORTIFY_INLINE __diagnose_as(__builtin_strcat, 1, 2)
>> char *strcat(char * const POS p, const char *q)
>> {
>> size_t p_size = __member_size(p);
>> + size_t size;
>>
>> if (p_size == SIZE_MAX)
>> return __underlying_strcat(p, q);
>> - if (strlcat(p, q, p_size) >= p_size)
>> - fortify_panic(__func__);
>> + size = strlcat(p, q, p_size);
>> + if (p_size < size)
>
>What happens when they're equal? I think this patch changes
>behavior...? Intentional?
>
>Did flipping this conditional drop what should be `<=`?
>
>Was there an off by one, or is this version of this patch potentially
>introducing one? Or am I misremembering my boolean algebra?
Whoops! Thanks for catching that. I was going too fast. And I'm bothered that my regression tests missed it. :|
I will send a v2...
-Kees
--
Kees Cook