[permalink] [raw]

Subject: [PATCH v2 63/63] fortify: Work around Clang inlining bugs

To enable FORTIFY_SOURCE support for Clang, the kernel must work around
a pair of bugs, related to Clang's inlining.

Change all the fortified APIs into macros with different inline names to
bypass Clang's broken inline-of-a-builtin detection:
https://bugs.llvm.org/show_bug.cgi?id=50322

Lift all misbehaving __builtin_object_size() calls into the macros to
bypass Clang's broken __builtin_object_size() arguments-of-an-inline
visibility:
https://github.com/ClangBuiltLinux/linux/issues/1401

Thankfully, due to how the inlining already behaves in GCC, this change
has no effect on GCC builds, but allows Clang to finally gain full
FORTIFY coverage.

However, because of a third bug which had no work-arounds, FORTIFY_SOURCE
will only work with Clang version 13 and later. Update the Kconfig to
reflect the new requirements.

Signed-off-by: Kees Cook <[email protected]>
---
include/linux/fortify-string.h | 55 +++++++++++++++++++++-------------
security/Kconfig | 2 +-
2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 7de4673dfe2c..e62d3633a329 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -48,10 +48,10 @@ extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size)
#define __underlying_strncpy __builtin_strncpy
#endif

-__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size)
+#define strncpy(p, q, s) __fortify_strncpy(p, q, s, __builtin_object_size(p, 1))
+__FORTIFY_INLINE char *__fortify_strncpy(char *p, const char *q,
+ __kernel_size_t size, size_t p_size)
{
- size_t p_size = __builtin_object_size(p, 1);
-
if (__builtin_constant_p(size) && p_size < size)
__write_overflow();
if (p_size < size)
@@ -71,9 +71,10 @@ __FORTIFY_INLINE char *strcat(char *p, const char *q)
}

extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
-__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
+#define strnlen(p, s) __fortify_strnlen(p, s, __builtin_object_size(p, 1))
+__FORTIFY_INLINE __kernel_size_t __fortify_strnlen(const char *p, size_t maxlen,
+ size_t p_size)
{
- size_t p_size = __builtin_object_size(p, 1);
size_t p_len = __compiletime_strlen(p);
size_t ret;

@@ -108,10 +109,14 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)

/* defined after fortified strlen to reuse it */
extern size_t __real_strlcpy(char *, const char *, size_t) __RENAME(strlcpy);
-__FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)
+#define strlcpy(p, q, s) __fortify_strlcpy(p, q, s, \
+ __builtin_object_size(p, 1), \
+ __builtin_object_size(q, 1))
+__FORTIFY_INLINE size_t __fortify_strlcpy(char *p, const char *q,
+ size_t size,
+ const size_t p_size,
+ const size_t q_size)
{
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);
size_t q_len; /* Full count of source string length. */
size_t len; /* Count of characters going into destination. */

@@ -135,12 +140,15 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size)

/* defined after fortified strnlen to reuse it */
extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(strscpy);
-__FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
+#define strscpy(p, q, s) __fortify_strscpy(p, q, s, \
+ __builtin_object_size(p, 1), \
+ __builtin_object_size(q, 1))
+__FORTIFY_INLINE ssize_t __fortify_strscpy(char *p, const char *q,
+ size_t size,
+ const size_t p_size,
+ const size_t q_size)
{
size_t len;
- /* Use string size rather than possible enclosing struct size. */
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);

/* If we cannot get size of p and q default to call strscpy. */
if (p_size == (size_t) -1 && q_size == (size_t) -1)
@@ -181,11 +189,13 @@ __FORTIFY_INLINE ssize_t strscpy(char *p, const char *q, size_t size)
}

/* defined after fortified strlen and strnlen to reuse them */
-__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
+#define strncat(p, q, count) __fortify_strncat(p, q, count, \
+ __builtin_object_size(p, 1), \
+ __builtin_object_size(q, 1))
+__FORTIFY_INLINE char *__fortify_strncat(char *p, const char *q, size_t count,
+ size_t p_size, size_t q_size)
{
size_t p_len, copy_len;
- size_t p_size = __builtin_object_size(p, 1);
- size_t q_size = __builtin_object_size(q, 1);

if (p_size == (size_t)-1 && q_size == (size_t)-1)
return __underlying_strncat(p, q, count);
@@ -352,7 +362,8 @@ __FORTIFY_INLINE void fortify_memcpy_chk(__kernel_size_t size,
memmove)

extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan);
-__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
+#define memscan(p, c, s) __fortify_memscan(p, c, s)
+__FORTIFY_INLINE void *__fortify_memscan(void *p, int c, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);

@@ -363,7 +374,8 @@ __FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size)
return __real_memscan(p, c, size);
}

-__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
+#define memcmp(p, q, s) __fortify_memcmp(p, q, s)
+__FORTIFY_INLINE int __fortify_memcmp(const void *p, const void *q, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);
size_t q_size = __builtin_object_size(q, 0);
@@ -379,7 +391,8 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size)
return __underlying_memcmp(p, q, size);
}

-__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
+#define memchr(p, c, s) __fortify_memchr(p, c, s)
+__FORTIFY_INLINE void *__fortify_memchr(const void *p, int c, __kernel_size_t size)
{
size_t p_size = __builtin_object_size(p, 0);

@@ -391,7 +404,8 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size)
}

void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv);
-__FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size)
+#define memchr_inv(p, c, s) __fortify_memchr_inv(p, c, s)
+__FORTIFY_INLINE void *__fortify_memchr_inv(const void *p, int c, size_t size)
{
size_t p_size = __builtin_object_size(p, 0);

@@ -415,7 +429,8 @@ __FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp)
}

/* Defined after fortified strlen to reuse it. */
-__FORTIFY_INLINE char *strcpy(char *p, const char *q)
+#define strcpy(p, q) __fortify_strcpy(p, q)
+__FORTIFY_INLINE char *__fortify_strcpy(char *p, const char *q)
{
size_t p_size = __builtin_object_size(p, 1);
size_t q_size = __builtin_object_size(q, 1);
diff --git a/security/Kconfig b/security/Kconfig
index fe6c0395fa02..530a15566b1d 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -193,7 +193,7 @@ config FORTIFY_SOURCE
depends on ARCH_HAS_FORTIFY_SOURCE
# https://bugs.llvm.org/show_bug.cgi?id=50322
# https://bugs.llvm.org/show_bug.cgi?id=41459
- depends on !CC_IS_CLANG
+ depends on !CC_IS_CLANG || CLANG_VERSION >= 130000
help
Detect overflows of buffers in common string and memory functions
where the compiler can determine and validate the buffer sizes.
--
2.30.2

2021-08-18 06:13:44

by Kees Cook

[permalink] [raw]

Subject: [PATCH v2 47/63] intel_th: msu: Use memset_startat() for clearing hw header

2021-08-18 06:13:57

by Kees Cook

[permalink] [raw]

Subject: [PATCH v2 62/63] fortify: Detect struct member overflows in memset() at compile-time

As done for memcpy(), also update memset() to use the same tightened
compile-time bounds checking under CONFIG_FORTIFY_SOURCE.

Signed-off-by: Kees Cook <[email protected]>
---
include/linux/fortify-string.h | 54 ++++++++++++++++---
.../write_overflow_field-memset.c | 5 ++
2 files changed, 51 insertions(+), 8 deletions(-)
create mode 100644 lib/test_fortify/write_overflow_field-memset.c

diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 0120d463ba33..7de4673dfe2c 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -198,17 +198,56 @@ __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count)
return p;
}

-__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size)
+__FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
+ const size_t p_size,
+ const size_t p_size_field)
{
- size_t p_size = __builtin_object_size(p, 0);
+ if (__builtin_constant_p(size)) {
+ /*
+ * Length argument is a constant expression, so we
+ * can perform compile-time bounds checking where
+ * buffer sizes are known.
+ */

- if (__builtin_constant_p(size) && p_size < size)
- __write_overflow();
- if (p_size < size)
- fortify_panic(__func__);
- return __underlying_memset(p, c, size);
+ /* Error when size is larger than enclosing struct. */
+ if (p_size > p_size_field && p_size < size)
+ __write_overflow();
+
+ /* Warn when write size is larger than dest field. */
+ if (p_size_field < size)
+ __write_overflow_field(p_size_field, size);
+ }
+ /*
+ * At this point, length argument may not be a constant expression,
+ * so run-time bounds checking can be done where buffer sizes are
+ * known. (This is not an "else" because the above checks may only
+ * be compile-time warnings, and we want to still warn for run-time
+ * overflows.)
+ */
+
+ /*
+ * Always stop accesses beyond the struct that contains the
+ * field, when the buffer's remaining size is known.
+ * (The -1 test is to optimize away checks where the buffer
+ * lengths are unknown.)
+ */
+ if (p_size != (size_t)(-1) && p_size < size)
+ fortify_panic("memset");
}

+#define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({ \
+ size_t __fortify_size = (size_t)(size); \
+ fortify_memset_chk(__fortify_size, p_size, p_size_field), \
+ __underlying_memset(p, c, __fortify_size); \
+})
+
+/*
+ * __builtin_object_size() must be captured here to avoid evaluating argument
+ * side-effects further into the macro layers.
+ */
+#define memset(p, c, s) __fortify_memset_chk(p, c, s, \
+ __builtin_object_size(p, 0), __builtin_object_size(p, 1))
+
/*
* To make sure the compiler can enforce protection against buffer overflows,
* memcpy(), memmove(), and memset() must not be used beyond individual
@@ -399,7 +438,6 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q)
/* Don't use these outside the FORITFY_SOURCE implementation */
#undef __underlying_memchr
#undef __underlying_memcmp
-#undef __underlying_memset
#undef __underlying_strcat
#undef __underlying_strcpy
#undef __underlying_strlen
diff --git a/lib/test_fortify/write_overflow_field-memset.c b/lib/test_fortify/write_overflow_field-memset.c
new file mode 100644
index 000000000000..2331da26909e
--- /dev/null
+++ b/lib/test_fortify/write_overflow_field-memset.c
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define TEST \
+ memset(instance.buf, 0x42, sizeof(instance.buf) + 1)
+
+#include "test_fortify.h"
--
2.30.2

2021-08-18 06:14:59

by Kees Cook

[permalink] [raw]

by Kees Cook

[permalink] [raw]

Subject: [PATCH v2 43/63] net: qede: Use memset_startat() for counters

2021-08-18 06:24:54

by Kees Cook

[permalink] [raw]

[permalink] [raw]

Subject: Re: [PATCH v2 12/63] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region

On Tue, 2021-11-23 at 14:19 +0100, Rafael J. Wysocki wrote:
> On Wed, Aug 18, 2021 at 8:08 AM Kees Cook <[email protected]>
> wrote:
> >
> > In preparation for FORTIFY_SOURCE performing compile-time and run-
> > time
> > field bounds checking for memcpy(), avoid intentionally writing
> > across
> > neighboring fields.
> >
> > Use struct_group() in struct art around members weight, and ac[0-
> > 9]_max,
> > so they can be referenced together. This will allow memcpy() and
> > sizeof()
> > to more easily reason about sizes, improve readability, and avoid
> > future
> > warnings about writing beyond the end of weight.
> >
> > "pahole" shows no size nor member offset changes to struct art.
> > "objdump -d" shows no meaningful object code changes (i.e. only
> > source
> > line number induced differences).
> >
> > Cc: Zhang Rui <[email protected]>
> > Cc: Daniel Lezcano <[email protected]>
> > Cc: Amit Kucheria <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
>
> Rui, Srinivas, any comments here?
Looks good.
Reviewed-by: Srinivas Pandruvada <[email protected]>

Thanks,
Srinivas

>
> > ---
> > .../intel/int340x_thermal/acpi_thermal_rel.c | 5 +-
> > .../intel/int340x_thermal/acpi_thermal_rel.h | 48 ++++++++++-------
> > --
> > 2 files changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> > b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> > index a478cff8162a..e90690a234c4 100644
> > --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> > +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.c
> > @@ -250,8 +250,9 @@ static int fill_art(char __user *ubuf)
> >                 get_single_name(arts[i].source,
> > art_user[i].source_device);
> >                 get_single_name(arts[i].target,
> > art_user[i].target_device);
> >                 /* copy the rest int data in addition to source and
> > target */
> > -               memcpy(&art_user[i].weight, &arts[i].weight,
> > -                       sizeof(u64) * (ACPI_NR_ART_ELEMENTS - 2));
> > +               BUILD_BUG_ON(sizeof(art_user[i].data) !=
> > +                            sizeof(u64) * (ACPI_NR_ART_ELEMENTS -
> > 2));
> > +               memcpy(&art_user[i].data, &arts[i].data,
> > sizeof(art_user[i].data));
> >         }
> >
> >         if (copy_to_user(ubuf, art_user, art_len))
> > diff --git a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> > b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> > index 58822575fd54..78d942477035 100644
> > --- a/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> > +++ b/drivers/thermal/intel/int340x_thermal/acpi_thermal_rel.h
> > @@ -17,17 +17,19 @@
> > struct art {
> >         acpi_handle source;
> >         acpi_handle target;
> > -       u64 weight;
> > -       u64 ac0_max;
> > -       u64 ac1_max;
> > -       u64 ac2_max;
> > -       u64 ac3_max;
> > -       u64 ac4_max;
> > -       u64 ac5_max;
> > -       u64 ac6_max;
> > -       u64 ac7_max;
> > -       u64 ac8_max;
> > -       u64 ac9_max;
> > +       struct_group(data,
> > +               u64 weight;
> > +               u64 ac0_max;
> > +               u64 ac1_max;
> > +               u64 ac2_max;
> > +               u64 ac3_max;
> > +               u64 ac4_max;
> > +               u64 ac5_max;
> > +               u64 ac6_max;
> > +               u64 ac7_max;
> > +               u64 ac8_max;
> > +               u64 ac9_max;
> > +       );
> > } __packed;
> >
> > struct trt {
> > @@ -47,17 +49,19 @@ union art_object {
> >         struct {
> >                 char source_device[8]; /* ACPI single name */
> >                 char target_device[8]; /* ACPI single name */
> > -               u64 weight;
> > -               u64 ac0_max_level;
> > -               u64 ac1_max_level;
> > -               u64 ac2_max_level;
> > -               u64 ac3_max_level;
> > -               u64 ac4_max_level;
> > -               u64 ac5_max_level;
> > -               u64 ac6_max_level;
> > -               u64 ac7_max_level;
> > -               u64 ac8_max_level;
> > -               u64 ac9_max_level;
> > +               struct_group(data,
> > +                       u64 weight;
> > +                       u64 ac0_max_level;
> > +                       u64 ac1_max_level;
> > +                       u64 ac2_max_level;
> > +                       u64 ac3_max_level;
> > +                       u64 ac4_max_level;
> > +                       u64 ac5_max_level;
> > +                       u64 ac6_max_level;
> > +                       u64 ac7_max_level;
> > +                       u64 ac8_max_level;
> > +                       u64 ac9_max_level;
> > +               );
> >         };
> >         u64 __data[ACPI_NR_ART_ELEMENTS];
> > };
> > --
> > 2.30.2
> >

2021-11-24 14:35:15

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH v2 12/63] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region

On Wed, Nov 24, 2021 at 12:53 AM Srinivas Pandruvada
<[email protected]> wrote:
>
> On Tue, 2021-11-23 at 14:19 +0100, Rafael J. Wysocki wrote:
> > On Wed, Aug 18, 2021 at 8:08 AM Kees Cook <[email protected]>
> > wrote:
> > >
> > > In preparation for FORTIFY_SOURCE performing compile-time and run-
> > > time
> > > field bounds checking for memcpy(), avoid intentionally writing
> > > across
> > > neighboring fields.
> > >
> > > Use struct_group() in struct art around members weight, and ac[0-
> > > 9]_max,
> > > so they can be referenced together. This will allow memcpy() and
> > > sizeof()
> > > to more easily reason about sizes, improve readability, and avoid
> > > future
> > > warnings about writing beyond the end of weight.
> > >
> > > "pahole" shows no size nor member offset changes to struct art.
> > > "objdump -d" shows no meaningful object code changes (i.e. only
> > > source
> > > line number induced differences).
> > >
> > > Cc: Zhang Rui <[email protected]>
> > > Cc: Daniel Lezcano <[email protected]>
> > > Cc: Amit Kucheria <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Kees Cook <[email protected]>
> >
> > Rui, Srinivas, any comments here?
> Looks good.
> Reviewed-by: Srinivas Pandruvada <[email protected]>

Applied as 5.17 material, thank you!