* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) [not found] <20250516202417.31b13d13@canb.auug.org.au> @ 2025-05-17 2:54 ` Randy Dunlap 2025-05-19 15:29 ` Mickaël Salaün 0 siblings, 1 reply; 12+ messages in thread From: Randy Dunlap @ 2025-05-17 2:54 UTC (permalink / raw) To: Stephen Rothwell, Linux Next Mailing List Cc: Linux Kernel Mailing List, Mickaël Salaün, linux-security-module, Kees Cook [-- Attachment #1: Type: text/plain, Size: 1023 bytes --] On 5/16/25 3:24 AM, Stephen Rothwell wrote: > Hi all, > > Changes since 20250515: > on i386: In file included from ../arch/x86/include/asm/string.h:3, from ../include/linux/string.h:65, from ../include/linux/bitmap.h:13, from ../include/linux/cpumask.h:12, from ../include/linux/smp.h:13, from ../include/linux/lockdep.h:14, from ../security/landlock/ruleset.c:16: ../security/landlock/ruleset.c: In function 'create_rule': ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~ ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' 137 | memcpy(new_rule->layers, layers, | ^~~~~~ Full randconfig file is attached. -- ~Randy [-- Attachment #2: config-r9208.gz --] [-- Type: application/gzip, Size: 28788 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-17 2:54 ` linux-next: Tree for May 16 (security/landlock/ruleset.c) Randy Dunlap @ 2025-05-19 15:29 ` Mickaël Salaün 2025-05-19 18:19 ` Kees Cook 0 siblings, 1 reply; 12+ messages in thread From: Mickaël Salaün @ 2025-05-19 15:29 UTC (permalink / raw) To: Randy Dunlap Cc: Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Kees Cook, Günther Noack On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > Hi all, > > > > Changes since 20250515: Thanks for the report. It is the same warning as reported here: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ I don't know what the actual issue is though. > > on i386: > > In file included from ../arch/x86/include/asm/string.h:3, > from ../include/linux/string.h:65, > from ../include/linux/bitmap.h:13, > from ../include/linux/cpumask.h:12, > from ../include/linux/smp.h:13, > from ../include/linux/lockdep.h:14, > from ../security/landlock/ruleset.c:16: > ../security/landlock/ruleset.c: In function 'create_rule': > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > 137 | memcpy(new_rule->layers, layers, > | ^~~~~~ > > > Full randconfig file is attached. > > -- > ~Randy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 15:29 ` Mickaël Salaün @ 2025-05-19 18:19 ` Kees Cook 2025-05-19 18:41 ` Mickaël Salaün 0 siblings, 1 reply; 12+ messages in thread From: Kees Cook @ 2025-05-19 18:19 UTC (permalink / raw) To: Mickaël Salaün Cc: Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: > On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > > Hi all, > > > > > > Changes since 20250515: > > Thanks for the report. > > It is the same warning as reported here: > https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > I don't know what the actual issue is though. > > > > > on i386: > > > > In file included from ../arch/x86/include/asm/string.h:3, > > from ../include/linux/string.h:65, > > from ../include/linux/bitmap.h:13, > > from ../include/linux/cpumask.h:12, > > from ../include/linux/smp.h:13, > > from ../include/linux/lockdep.h:14, > > from ../security/landlock/ruleset.c:16: > > ../security/landlock/ruleset.c: In function 'create_rule': > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > 137 | memcpy(new_rule->layers, layers, > > | ^~~~~~ > > > > > > Full randconfig file is attached. The trigger appears to be CONFIG_PROFILE_ALL_BRANCHES, and GCC getting tricked into thinking check_mul_overflow() returns true: In file included from ../arch/x86/include/asm/string.h:3, from ../include/linux/string.h:65, from ../include/linux/bitmap.h:13, from ../include/linux/cpumask.h:12, from ../include/linux/smp.h:13, from ../include/linux/lockdep.h:14, from ../security/landlock/ruleset.c:16: ../security/landlock/ruleset.c: In function 'create_rule': ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 0 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~ ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' 137 | memcpy(new_rule->layers, layers, | ^~~~~~ 'create_rule': event 1 ../include/linux/compiler.h:69:46: 68 | (cond) ? \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 69 | (__if_trace.miss_hit[1]++,1) : \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ | | | (1) when the condition is evaluated to true 70 | (__if_trace.miss_hit[0]++,0); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../include/linux/compiler.h:57:69: note: in expansion of macro '__trace_if_value' 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~~~~~~~~~~~~~ ../include/linux/compiler.h:55:28: note: in expansion of macro '__trace_if_var' 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~~~~~~~~~~~ ../include/linux/overflow.h:270:9: note: in expansion of macro 'if' 270 | if (check_mul_overflow(factor1, factor2, &bytes)) | ^~ 'create_rule': event 2 ../arch/x86/include/asm/string_32.h:150:25: 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~ | | | (2) out of array bounds here ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' 137 | memcpy(new_rule->layers, layers, | ^~~~~~ make[1]: Leaving directory '/srv/code/gcc-bug' I'll take a look at ways to make either the overflow macros or memcpy robust against this kind of weirdness... -- Kees Cook ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 18:19 ` Kees Cook @ 2025-05-19 18:41 ` Mickaël Salaün 2025-05-19 19:15 ` Kees Cook 0 siblings, 1 reply; 12+ messages in thread From: Mickaël Salaün @ 2025-05-19 18:41 UTC (permalink / raw) To: Kees Cook Cc: Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 11:19:53AM -0700, Kees Cook wrote: > On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: > > On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > > > > > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > > > Hi all, > > > > > > > > Changes since 20250515: > > > > Thanks for the report. > > > > It is the same warning as reported here: > > https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > > > I don't know what the actual issue is though. > > > > > > > > on i386: > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > from ../include/linux/string.h:65, > > > from ../include/linux/bitmap.h:13, > > > from ../include/linux/cpumask.h:12, > > > from ../include/linux/smp.h:13, > > > from ../include/linux/lockdep.h:14, > > > from ../security/landlock/ruleset.c:16: > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > 137 | memcpy(new_rule->layers, layers, > > > | ^~~~~~ > > > > > > > > > Full randconfig file is attached. > > The trigger appears to be CONFIG_PROFILE_ALL_BRANCHES, and GCC getting > tricked into thinking check_mul_overflow() returns true: > > In file included from ../arch/x86/include/asm/string.h:3, > from ../include/linux/string.h:65, > from ../include/linux/bitmap.h:13, > from ../include/linux/cpumask.h:12, > from ../include/linux/smp.h:13, > from ../include/linux/lockdep.h:14, > from ../security/landlock/ruleset.c:16: > ../security/landlock/ruleset.c: In function 'create_rule': > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 0 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > 137 | memcpy(new_rule->layers, layers, > | ^~~~~~ > 'create_rule': event 1 > ../include/linux/compiler.h:69:46: > 68 | (cond) ? \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 69 | (__if_trace.miss_hit[1]++,1) : \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > | | > | (1) when the condition is evaluated to true > 70 | (__if_trace.miss_hit[0]++,0); \ > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../include/linux/compiler.h:57:69: note: in expansion of macro '__trace_if_value' > 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > | ^~~~~~~~~~~~~~~~ > ../include/linux/compiler.h:55:28: note: in expansion of macro '__trace_if_var' > 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > | ^~~~~~~~~~~~~~ > ../include/linux/overflow.h:270:9: note: in expansion of macro 'if' > 270 | if (check_mul_overflow(factor1, factor2, &bytes)) > | ^~ > 'create_rule': event 2 > ../arch/x86/include/asm/string_32.h:150:25: > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > | | > | (2) out of array bounds here > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > 137 | memcpy(new_rule->layers, layers, > | ^~~~~~ > make[1]: Leaving directory '/srv/code/gcc-bug' That's interesting... > > > I'll take a look at ways to make either the overflow macros or memcpy > robust against this kind of weirdness... Thanks! > > -- > Kees Cook > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 18:41 ` Mickaël Salaün @ 2025-05-19 19:15 ` Kees Cook 2025-05-19 20:26 ` Randy Dunlap ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Kees Cook @ 2025-05-19 19:15 UTC (permalink / raw) To: Mickaël Salaün Cc: Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: > On Mon, May 19, 2025 at 11:19:53AM -0700, Kees Cook wrote: > > On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: > > > On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > > > > > > > > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > > > > Hi all, > > > > > > > > > > Changes since 20250515: > > > > > > Thanks for the report. > > > > > > It is the same warning as reported here: > > > https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > > > > > I don't know what the actual issue is though. > > > > > > > > > > > on i386: > > > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > > from ../include/linux/string.h:65, > > > > from ../include/linux/bitmap.h:13, > > > > from ../include/linux/cpumask.h:12, > > > > from ../include/linux/smp.h:13, > > > > from ../include/linux/lockdep.h:14, > > > > from ../security/landlock/ruleset.c:16: > > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > > 137 | memcpy(new_rule->layers, layers, > > > > | ^~~~~~ > > > > > > > > > > > > Full randconfig file is attached. > > > > The trigger appears to be CONFIG_PROFILE_ALL_BRANCHES, and GCC getting > > tricked into thinking check_mul_overflow() returns true: > > > > In file included from ../arch/x86/include/asm/string.h:3, > > from ../include/linux/string.h:65, > > from ../include/linux/bitmap.h:13, > > from ../include/linux/cpumask.h:12, > > from ../include/linux/smp.h:13, > > from ../include/linux/lockdep.h:14, > > from ../security/landlock/ruleset.c:16: > > ../security/landlock/ruleset.c: In function 'create_rule': > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 0 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > 137 | memcpy(new_rule->layers, layers, > > | ^~~~~~ > > 'create_rule': event 1 > > ../include/linux/compiler.h:69:46: > > 68 | (cond) ? \ > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 69 | (__if_trace.miss_hit[1]++,1) : \ > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > > | | > > | (1) when the condition is evaluated to true > > 70 | (__if_trace.miss_hit[0]++,0); \ > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../include/linux/compiler.h:57:69: note: in expansion of macro '__trace_if_value' > > 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > | ^~~~~~~~~~~~~~~~ > > ../include/linux/compiler.h:55:28: note: in expansion of macro '__trace_if_var' > > 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > | ^~~~~~~~~~~~~~ > > ../include/linux/overflow.h:270:9: note: in expansion of macro 'if' > > 270 | if (check_mul_overflow(factor1, factor2, &bytes)) > > | ^~ > > 'create_rule': event 2 > > ../arch/x86/include/asm/string_32.h:150:25: > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > | | > > | (2) out of array bounds here > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > 137 | memcpy(new_rule->layers, layers, > > | ^~~~~~ > > make[1]: Leaving directory '/srv/code/gcc-bug' > > That's interesting... > > > > > > > I'll take a look at ways to make either the overflow macros or memcpy > > robust against this kind of weirdness... > > Thanks! I'm doing some build testing, but the below patch makes GCC happy. Alternatively we could make CONFIG_PROFILE_ALL_BRANCHES=y depend on CONFIG_FORTIFY_SOURCE=y ... From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 From: Kees Cook <kees@kernel.org> Date: Mon, 19 May 2025 11:52:06 -0700 Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of defining memcpy() in terms of __builtin_memcpy() deep in arch/x86/include/asm/string_32.h, notice that it is needed up in the general string.h, as done with other common C String APIs. This allows us to add basic sanity checking for pathological "size" arguments to memcpy(). Besides the run-time checking benefit, this avoids GCC trying to be very smart about value range tracking[1] when CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ Reported-by: Randy Dunlap <rdunlap@infradead.org> Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: "Mickaël Salaün" <mic@digikod.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: <x86@kernel.org> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Andy Shevchenko <andy@kernel.org> Cc: Uros Bizjak <ubizjak@gmail.com> Cc: <linux-hardening@vger.kernel.org> --- arch/x86/include/asm/string_32.h | 6 ------ include/linux/string.h | 13 +++++++++++++ 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h index e9cce169bb4c..74397c95fa37 100644 --- a/arch/x86/include/asm/string_32.h +++ b/arch/x86/include/asm/string_32.h @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, #define __HAVE_ARCH_MEMCPY extern void *memcpy(void *, const void *, size_t); -#ifndef CONFIG_FORTIFY_SOURCE - -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) - -#endif /* !CONFIG_FORTIFY_SOURCE */ - #define __HAVE_ARCH_MEMMOVE void *memmove(void *dest, const void *src, size_t n); diff --git a/include/linux/string.h b/include/linux/string.h index 01621ad0f598..ffcee31a14f9 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -3,6 +3,7 @@ #define _LINUX_STRING_H_ #include <linux/args.h> +#include <linux/bug.h> #include <linux/array_size.h> #include <linux/cleanup.h> /* for DEFINE_FREE() */ #include <linux/compiler.h> /* for inline */ @@ -390,7 +391,19 @@ static inline const char *kbasename(const char *path) #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) #include <linux/fortify-string.h> +#else +/* Basic sanity checking even without FORTIFY_SOURCE */ +# ifndef __HAVE_ARCH_MEMCPY +# define memcpy(t, f, n) \ + do { \ + typeof(n) __n = (n); \ + /* Skip impossible sizes. */ \ + if (!WARN_ON(__n < 0 || __n == SIZE_MAX)) \ + __builtin_memcpy(t, f, __n); \ + } while (0) +# endif #endif + #ifndef unsafe_memcpy #define unsafe_memcpy(dst, src, bytes, justification) \ memcpy(dst, src, bytes) -- 2.34.1 -- Kees Cook ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 19:15 ` Kees Cook @ 2025-05-19 20:26 ` Randy Dunlap 2025-05-20 16:44 ` Kees Cook 2025-05-20 14:01 ` Andy Shevchenko 2025-05-20 14:45 ` Mickaël Salaün 2 siblings, 1 reply; 12+ messages in thread From: Randy Dunlap @ 2025-05-19 20:26 UTC (permalink / raw) To: Kees Cook, Mickaël Salaün Cc: Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack > From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > From: Kees Cook <kees@kernel.org> > Date: Mon, 19 May 2025 11:52:06 -0700 > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > in arch/x86/include/asm/string_32.h, notice that it is needed up in > the general string.h, as done with other common C String APIs. This > allows us to add basic sanity checking for pathological "size" > arguments to memcpy(). Besides the run-time checking benefit, this > avoids GCC trying to be very smart about value range tracking[1] when > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ > Signed-off-by: Kees Cook <kees@kernel.org> Tested-by: Randy Dunlap <rdunlap@infradead.org> Thanks. > --- > Cc: "Mickaël Salaün" <mic@digikod.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: <x86@kernel.org> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Andy Shevchenko <andy@kernel.org> > Cc: Uros Bizjak <ubizjak@gmail.com> > Cc: <linux-hardening@vger.kernel.org> > --- > arch/x86/include/asm/string_32.h | 6 ------ > include/linux/string.h | 13 +++++++++++++ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h > index e9cce169bb4c..74397c95fa37 100644 > --- a/arch/x86/include/asm/string_32.h > +++ b/arch/x86/include/asm/string_32.h > @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, > #define __HAVE_ARCH_MEMCPY > extern void *memcpy(void *, const void *, size_t); > > -#ifndef CONFIG_FORTIFY_SOURCE > - > -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) > - > -#endif /* !CONFIG_FORTIFY_SOURCE */ > - > #define __HAVE_ARCH_MEMMOVE > void *memmove(void *dest, const void *src, size_t n); > > diff --git a/include/linux/string.h b/include/linux/string.h > index 01621ad0f598..ffcee31a14f9 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -3,6 +3,7 @@ > #define _LINUX_STRING_H_ > > #include <linux/args.h> > +#include <linux/bug.h> > #include <linux/array_size.h> > #include <linux/cleanup.h> /* for DEFINE_FREE() */ > #include <linux/compiler.h> /* for inline */ > @@ -390,7 +391,19 @@ static inline const char *kbasename(const char *path) > > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) > #include <linux/fortify-string.h> > +#else > +/* Basic sanity checking even without FORTIFY_SOURCE */ > +# ifndef __HAVE_ARCH_MEMCPY > +# define memcpy(t, f, n) \ > + do { \ > + typeof(n) __n = (n); \ > + /* Skip impossible sizes. */ \ > + if (!WARN_ON(__n < 0 || __n == SIZE_MAX)) \ > + __builtin_memcpy(t, f, __n); \ > + } while (0) > +# endif > #endif > + > #ifndef unsafe_memcpy > #define unsafe_memcpy(dst, src, bytes, justification) \ > memcpy(dst, src, bytes) -- ~Randy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 20:26 ` Randy Dunlap @ 2025-05-20 16:44 ` Kees Cook 0 siblings, 0 replies; 12+ messages in thread From: Kees Cook @ 2025-05-20 16:44 UTC (permalink / raw) To: Randy Dunlap Cc: Mickaël Salaün, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 01:26:52PM -0700, Randy Dunlap wrote: > > > > From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > > From: Kees Cook <kees@kernel.org> > > Date: Mon, 19 May 2025 11:52:06 -0700 > > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > > in arch/x86/include/asm/string_32.h, notice that it is needed up in > > the general string.h, as done with other common C String APIs. This > > allows us to add basic sanity checking for pathological "size" > > arguments to memcpy(). Besides the run-time checking benefit, this > > avoids GCC trying to be very smart about value range tracking[1] when > > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > > > Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ > > Signed-off-by: Kees Cook <kees@kernel.org> > > Tested-by: Randy Dunlap <rdunlap@infradead.org> I missed this when I sent out the proper patch. I'll add it locally. Thanks! -Kees > > Thanks. > > > --- > > Cc: "Mickaël Salaün" <mic@digikod.net> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > Cc: <x86@kernel.org> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: Andy Shevchenko <andy@kernel.org> > > Cc: Uros Bizjak <ubizjak@gmail.com> > > Cc: <linux-hardening@vger.kernel.org> > > --- > > arch/x86/include/asm/string_32.h | 6 ------ > > include/linux/string.h | 13 +++++++++++++ > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h > > index e9cce169bb4c..74397c95fa37 100644 > > --- a/arch/x86/include/asm/string_32.h > > +++ b/arch/x86/include/asm/string_32.h > > @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, > > #define __HAVE_ARCH_MEMCPY > > extern void *memcpy(void *, const void *, size_t); > > > > -#ifndef CONFIG_FORTIFY_SOURCE > > - > > -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > - > > -#endif /* !CONFIG_FORTIFY_SOURCE */ > > - > > #define __HAVE_ARCH_MEMMOVE > > void *memmove(void *dest, const void *src, size_t n); > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 01621ad0f598..ffcee31a14f9 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -3,6 +3,7 @@ > > #define _LINUX_STRING_H_ > > > > #include <linux/args.h> > > +#include <linux/bug.h> > > #include <linux/array_size.h> > > #include <linux/cleanup.h> /* for DEFINE_FREE() */ > > #include <linux/compiler.h> /* for inline */ > > @@ -390,7 +391,19 @@ static inline const char *kbasename(const char *path) > > > > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) > > #include <linux/fortify-string.h> > > +#else > > +/* Basic sanity checking even without FORTIFY_SOURCE */ > > +# ifndef __HAVE_ARCH_MEMCPY > > +# define memcpy(t, f, n) \ > > + do { \ > > + typeof(n) __n = (n); \ > > + /* Skip impossible sizes. */ \ > > + if (!WARN_ON(__n < 0 || __n == SIZE_MAX)) \ > > + __builtin_memcpy(t, f, __n); \ > > + } while (0) > > +# endif > > #endif > > + > > #ifndef unsafe_memcpy > > #define unsafe_memcpy(dst, src, bytes, justification) \ > > memcpy(dst, src, bytes) > > -- > ~Randy -- Kees Cook ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 19:15 ` Kees Cook 2025-05-19 20:26 ` Randy Dunlap @ 2025-05-20 14:01 ` Andy Shevchenko 2025-05-20 16:47 ` Kees Cook 2025-05-20 14:45 ` Mickaël Salaün 2 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2025-05-20 14:01 UTC (permalink / raw) To: Kees Cook Cc: Mickaël Salaün, Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 12:15:30PM -0700, Kees Cook wrote: > On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: ... > >From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > From: Kees Cook <kees@kernel.org> > Date: Mon, 19 May 2025 11:52:06 -0700 > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > in arch/x86/include/asm/string_32.h, notice that it is needed up in > the general string.h, as done with other common C String APIs. This > allows us to add basic sanity checking for pathological "size" > arguments to memcpy(). Besides the run-time checking benefit, this > avoids GCC trying to be very smart about value range tracking[1] when > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: "Mickaël Salaün" <mic@digikod.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: <x86@kernel.org> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Andy Shevchenko <andy@kernel.org> > Cc: Uros Bizjak <ubizjak@gmail.com> > Cc: <linux-hardening@vger.kernel.org> > --- > arch/x86/include/asm/string_32.h | 6 ------ > include/linux/string.h | 13 +++++++++++++ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h > index e9cce169bb4c..74397c95fa37 100644 > --- a/arch/x86/include/asm/string_32.h > +++ b/arch/x86/include/asm/string_32.h > @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, > #define __HAVE_ARCH_MEMCPY > extern void *memcpy(void *, const void *, size_t); > > -#ifndef CONFIG_FORTIFY_SOURCE > - > -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) > - > -#endif /* !CONFIG_FORTIFY_SOURCE */ > - > #define __HAVE_ARCH_MEMMOVE > void *memmove(void *dest, const void *src, size_t n); > > diff --git a/include/linux/string.h b/include/linux/string.h > index 01621ad0f598..ffcee31a14f9 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -3,6 +3,7 @@ > #define _LINUX_STRING_H_ > > #include <linux/args.h> > +#include <linux/bug.h> In case you are go with this change, please keep the headers in order. > #include <linux/array_size.h> (should be located here) > #include <linux/cleanup.h> /* for DEFINE_FREE() */ > #include <linux/compiler.h> /* for inline */ > @@ -390,7 +391,19 @@ static inline const char *kbasename(const char *path) > > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) > #include <linux/fortify-string.h> > +#else > +/* Basic sanity checking even without FORTIFY_SOURCE */ > +# ifndef __HAVE_ARCH_MEMCPY > +# define memcpy(t, f, n) \ > + do { \ > + typeof(n) __n = (n); \ > + /* Skip impossible sizes. */ \ > + if (!WARN_ON(__n < 0 || __n == SIZE_MAX)) \ > + __builtin_memcpy(t, f, __n); \ > + } while (0) > +# endif > #endif > + > #ifndef unsafe_memcpy > #define unsafe_memcpy(dst, src, bytes, justification) \ > memcpy(dst, src, bytes) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-20 14:01 ` Andy Shevchenko @ 2025-05-20 16:47 ` Kees Cook 0 siblings, 0 replies; 12+ messages in thread From: Kees Cook @ 2025-05-20 16:47 UTC (permalink / raw) To: Andy Shevchenko Cc: Mickaël Salaün, Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Tue, May 20, 2025 at 05:01:51PM +0300, Andy Shevchenko wrote: > On Mon, May 19, 2025 at 12:15:30PM -0700, Kees Cook wrote: > > On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: > > ... > > > >From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > > From: Kees Cook <kees@kernel.org> > > Date: Mon, 19 May 2025 11:52:06 -0700 > > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > > in arch/x86/include/asm/string_32.h, notice that it is needed up in > > the general string.h, as done with other common C String APIs. This > > allows us to add basic sanity checking for pathological "size" > > arguments to memcpy(). Besides the run-time checking benefit, this > > avoids GCC trying to be very smart about value range tracking[1] when > > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > > > Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] > > Reported-by: kernel test robot <lkp@intel.com> > > Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ > > Signed-off-by: Kees Cook <kees@kernel.org> > > --- > > Cc: "Mickaël Salaün" <mic@digikod.net> > > Cc: Thomas Gleixner <tglx@linutronix.de> > > Cc: Ingo Molnar <mingo@redhat.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Dave Hansen <dave.hansen@linux.intel.com> > > Cc: <x86@kernel.org> > > Cc: "H. Peter Anvin" <hpa@zytor.com> > > Cc: Andy Shevchenko <andy@kernel.org> > > Cc: Uros Bizjak <ubizjak@gmail.com> > > Cc: <linux-hardening@vger.kernel.org> > > --- > > arch/x86/include/asm/string_32.h | 6 ------ > > include/linux/string.h | 13 +++++++++++++ > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h > > index e9cce169bb4c..74397c95fa37 100644 > > --- a/arch/x86/include/asm/string_32.h > > +++ b/arch/x86/include/asm/string_32.h > > @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, > > #define __HAVE_ARCH_MEMCPY > > extern void *memcpy(void *, const void *, size_t); > > > > -#ifndef CONFIG_FORTIFY_SOURCE > > - > > -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > - > > -#endif /* !CONFIG_FORTIFY_SOURCE */ > > - > > #define __HAVE_ARCH_MEMMOVE > > void *memmove(void *dest, const void *src, size_t n); > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 01621ad0f598..ffcee31a14f9 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -3,6 +3,7 @@ > > #define _LINUX_STRING_H_ > > > > #include <linux/args.h> > > +#include <linux/bug.h> > > In case you are go with this change, please keep the headers in order. > > > #include <linux/array_size.h> > > (should be located here) Oops, yes, that was my intent but I typoed my insert, it seems. Fixed now; thanks! -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-19 19:15 ` Kees Cook 2025-05-19 20:26 ` Randy Dunlap 2025-05-20 14:01 ` Andy Shevchenko @ 2025-05-20 14:45 ` Mickaël Salaün 2025-05-20 15:48 ` Randy Dunlap 2025-05-20 16:15 ` Kees Cook 2 siblings, 2 replies; 12+ messages in thread From: Mickaël Salaün @ 2025-05-20 14:45 UTC (permalink / raw) To: Kees Cook Cc: Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Mon, May 19, 2025 at 12:15:30PM -0700, Kees Cook wrote: > On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: > > On Mon, May 19, 2025 at 11:19:53AM -0700, Kees Cook wrote: > > > On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: > > > > On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > > > > > > > > > > > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > > > > > Hi all, > > > > > > > > > > > > Changes since 20250515: > > > > > > > > Thanks for the report. > > > > > > > > It is the same warning as reported here: > > > > https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > > > > > > > I don't know what the actual issue is though. > > > > > > > > > > > > > > on i386: > > > > > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > > > from ../include/linux/string.h:65, > > > > > from ../include/linux/bitmap.h:13, > > > > > from ../include/linux/cpumask.h:12, > > > > > from ../include/linux/smp.h:13, > > > > > from ../include/linux/lockdep.h:14, > > > > > from ../security/landlock/ruleset.c:16: > > > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > > > 137 | memcpy(new_rule->layers, layers, > > > > > | ^~~~~~ > > > > > > > > > > > > > > > Full randconfig file is attached. > > > > > > The trigger appears to be CONFIG_PROFILE_ALL_BRANCHES, and GCC getting > > > tricked into thinking check_mul_overflow() returns true: > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > from ../include/linux/string.h:65, > > > from ../include/linux/bitmap.h:13, > > > from ../include/linux/cpumask.h:12, > > > from ../include/linux/smp.h:13, > > > from ../include/linux/lockdep.h:14, > > > from ../security/landlock/ruleset.c:16: > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 0 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > 137 | memcpy(new_rule->layers, layers, > > > | ^~~~~~ > > > 'create_rule': event 1 > > > ../include/linux/compiler.h:69:46: > > > 68 | (cond) ? \ > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > 69 | (__if_trace.miss_hit[1]++,1) : \ > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > > > | | > > > | (1) when the condition is evaluated to true > > > 70 | (__if_trace.miss_hit[0]++,0); \ > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > ../include/linux/compiler.h:57:69: note: in expansion of macro '__trace_if_value' > > > 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > | ^~~~~~~~~~~~~~~~ > > > ../include/linux/compiler.h:55:28: note: in expansion of macro '__trace_if_var' > > > 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > | ^~~~~~~~~~~~~~ > > > ../include/linux/overflow.h:270:9: note: in expansion of macro 'if' > > > 270 | if (check_mul_overflow(factor1, factor2, &bytes)) > > > | ^~ > > > 'create_rule': event 2 > > > ../arch/x86/include/asm/string_32.h:150:25: > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > | | > > > | (2) out of array bounds here > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > 137 | memcpy(new_rule->layers, layers, > > > | ^~~~~~ > > > make[1]: Leaving directory '/srv/code/gcc-bug' > > > > That's interesting... > > > > > > > > > > > I'll take a look at ways to make either the overflow macros or memcpy > > > robust against this kind of weirdness... > > > > Thanks! > > I'm doing some build testing, but the below patch makes GCC happy. > Alternatively we could make CONFIG_PROFILE_ALL_BRANCHES=y depend on > CONFIG_FORTIFY_SOURCE=y ... > > > From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > From: Kees Cook <kees@kernel.org> > Date: Mon, 19 May 2025 11:52:06 -0700 > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > in arch/x86/include/asm/string_32.h, notice that it is needed up in > the general string.h, as done with other common C String APIs. This > allows us to add basic sanity checking for pathological "size" > arguments to memcpy(). Besides the run-time checking benefit, this > avoids GCC trying to be very smart about value range tracking[1] when > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. It works for me but I couldn't reproduce the issue. I tried with CONFIG_PROFILE_ALL_BRANCHES=y and CONFIG_FORTIFY_SOURCE=n but it always works without a warning. I'm using GCC 15. Is it specific to a version of GCC? > > Link: https://lore.kernel.org/all/202505191117.C094A90F88@keescook/ [1] > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Closes: https://lore.kernel.org/all/e3754f69-1dea-4542-8de0-a567a14fb95b@infradead.org/ > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: "Mickaël Salaün" <mic@digikod.net> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: <x86@kernel.org> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Andy Shevchenko <andy@kernel.org> > Cc: Uros Bizjak <ubizjak@gmail.com> > Cc: <linux-hardening@vger.kernel.org> > --- > arch/x86/include/asm/string_32.h | 6 ------ > include/linux/string.h | 13 +++++++++++++ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/include/asm/string_32.h b/arch/x86/include/asm/string_32.h > index e9cce169bb4c..74397c95fa37 100644 > --- a/arch/x86/include/asm/string_32.h > +++ b/arch/x86/include/asm/string_32.h > @@ -145,12 +145,6 @@ static __always_inline void *__constant_memcpy(void *to, const void *from, > #define __HAVE_ARCH_MEMCPY > extern void *memcpy(void *, const void *, size_t); > > -#ifndef CONFIG_FORTIFY_SOURCE > - > -#define memcpy(t, f, n) __builtin_memcpy(t, f, n) > - > -#endif /* !CONFIG_FORTIFY_SOURCE */ > - > #define __HAVE_ARCH_MEMMOVE > void *memmove(void *dest, const void *src, size_t n); > > diff --git a/include/linux/string.h b/include/linux/string.h > index 01621ad0f598..ffcee31a14f9 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -3,6 +3,7 @@ > #define _LINUX_STRING_H_ > > #include <linux/args.h> > +#include <linux/bug.h> > #include <linux/array_size.h> > #include <linux/cleanup.h> /* for DEFINE_FREE() */ > #include <linux/compiler.h> /* for inline */ > @@ -390,7 +391,19 @@ static inline const char *kbasename(const char *path) > > #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) > #include <linux/fortify-string.h> > +#else > +/* Basic sanity checking even without FORTIFY_SOURCE */ > +# ifndef __HAVE_ARCH_MEMCPY > +# define memcpy(t, f, n) \ > + do { \ > + typeof(n) __n = (n); \ > + /* Skip impossible sizes. */ \ > + if (!WARN_ON(__n < 0 || __n == SIZE_MAX)) \ > + __builtin_memcpy(t, f, __n); \ > + } while (0) > +# endif > #endif > + > #ifndef unsafe_memcpy > #define unsafe_memcpy(dst, src, bytes, justification) \ > memcpy(dst, src, bytes) > -- > 2.34.1 > > > > -- > Kees Cook > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-20 14:45 ` Mickaël Salaün @ 2025-05-20 15:48 ` Randy Dunlap 2025-05-20 16:15 ` Kees Cook 1 sibling, 0 replies; 12+ messages in thread From: Randy Dunlap @ 2025-05-20 15:48 UTC (permalink / raw) To: Mickaël Salaün, Kees Cook Cc: Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On 5/20/25 7:45 AM, Mickaël Salaün wrote: > On Mon, May 19, 2025 at 12:15:30PM -0700, Kees Cook wrote: >> On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: >>> On Mon, May 19, 2025 at 11:19:53AM -0700, Kees Cook wrote: >>>> On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: >>>>> On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: >>>>>> >>>>>> >>>>>> On 5/16/25 3:24 AM, Stephen Rothwell wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> Changes since 20250515: >>>>> >>>>> Thanks for the report. >>>>> >>>>> It is the same warning as reported here: >>>>> https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ >>>>> [snip] >>> >>>> >>>> >>>> I'll take a look at ways to make either the overflow macros or memcpy >>>> robust against this kind of weirdness... >>> >>> Thanks! >> >> I'm doing some build testing, but the below patch makes GCC happy. >> Alternatively we could make CONFIG_PROFILE_ALL_BRANCHES=y depend on >> CONFIG_FORTIFY_SOURCE=y ... >> >> >> From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 >> From: Kees Cook <kees@kernel.org> >> Date: Mon, 19 May 2025 11:52:06 -0700 >> Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> Instead of defining memcpy() in terms of __builtin_memcpy() deep >> in arch/x86/include/asm/string_32.h, notice that it is needed up in >> the general string.h, as done with other common C String APIs. This >> allows us to add basic sanity checking for pathological "size" >> arguments to memcpy(). Besides the run-time checking benefit, this >> avoids GCC trying to be very smart about value range tracking[1] when >> CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > It works for me but I couldn't reproduce the issue. I tried with > CONFIG_PROFILE_ALL_BRANCHES=y and CONFIG_FORTIFY_SOURCE=n but it always > works without a warning. I'm using GCC 15. Is it specific to a version > of GCC? I dunno. I'm using GCC 14.2.1. -- ~Randy ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: linux-next: Tree for May 16 (security/landlock/ruleset.c) 2025-05-20 14:45 ` Mickaël Salaün 2025-05-20 15:48 ` Randy Dunlap @ 2025-05-20 16:15 ` Kees Cook 1 sibling, 0 replies; 12+ messages in thread From: Kees Cook @ 2025-05-20 16:15 UTC (permalink / raw) To: Mickaël Salaün Cc: Randy Dunlap, Steven Rostedt, Stephen Rothwell, Linux Next Mailing List, Linux Kernel Mailing List, linux-security-module, Günther Noack On Tue, May 20, 2025 at 04:45:19PM +0200, Mickaël Salaün wrote: > On Mon, May 19, 2025 at 12:15:30PM -0700, Kees Cook wrote: > > On Mon, May 19, 2025 at 08:41:17PM +0200, Mickaël Salaün wrote: > > > On Mon, May 19, 2025 at 11:19:53AM -0700, Kees Cook wrote: > > > > On Mon, May 19, 2025 at 05:29:30PM +0200, Mickaël Salaün wrote: > > > > > On Fri, May 16, 2025 at 07:54:14PM -0700, Randy Dunlap wrote: > > > > > > > > > > > > > > > > > > On 5/16/25 3:24 AM, Stephen Rothwell wrote: > > > > > > > Hi all, > > > > > > > > > > > > > > Changes since 20250515: > > > > > > > > > > Thanks for the report. > > > > > > > > > > It is the same warning as reported here: > > > > > https://lore.kernel.org/all/202501040747.S3LYfvYq-lkp@intel.com/ > > > > > > > > > > I don't know what the actual issue is though. > > > > > > > > > > > > > > > > > on i386: > > > > > > > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > > > > from ../include/linux/string.h:65, > > > > > > from ../include/linux/bitmap.h:13, > > > > > > from ../include/linux/cpumask.h:12, > > > > > > from ../include/linux/smp.h:13, > > > > > > from ../include/linux/lockdep.h:14, > > > > > > from ../security/landlock/ruleset.c:16: > > > > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 20 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > > > > 137 | memcpy(new_rule->layers, layers, > > > > > > | ^~~~~~ > > > > > > > > > > > > > > > > > > Full randconfig file is attached. > > > > > > > > The trigger appears to be CONFIG_PROFILE_ALL_BRANCHES, and GCC getting > > > > tricked into thinking check_mul_overflow() returns true: > > > > > > > > In file included from ../arch/x86/include/asm/string.h:3, > > > > from ../include/linux/string.h:65, > > > > from ../include/linux/bitmap.h:13, > > > > from ../include/linux/cpumask.h:12, > > > > from ../include/linux/smp.h:13, > > > > from ../include/linux/lockdep.h:14, > > > > from ../security/landlock/ruleset.c:16: > > > > ../security/landlock/ruleset.c: In function 'create_rule': > > > > ../arch/x86/include/asm/string_32.h:150:25: warning: '__builtin_memcpy' accessing 4294967295 bytes at offsets 0 and 0 overlaps 6442450943 bytes at offset -2147483648 [-Wrestrict] > > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > > 137 | memcpy(new_rule->layers, layers, > > > > | ^~~~~~ > > > > 'create_rule': event 1 > > > > ../include/linux/compiler.h:69:46: > > > > 68 | (cond) ? \ > > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > 69 | (__if_trace.miss_hit[1]++,1) : \ > > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~ > > > > | | > > > > | (1) when the condition is evaluated to true > > > > 70 | (__if_trace.miss_hit[0]++,0); \ > > > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > ../include/linux/compiler.h:57:69: note: in expansion of macro '__trace_if_value' > > > > 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > > > > | ^~~~~~~~~~~~~~~~ > > > > ../include/linux/compiler.h:55:28: note: in expansion of macro '__trace_if_var' > > > > 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) > > > > | ^~~~~~~~~~~~~~ > > > > ../include/linux/overflow.h:270:9: note: in expansion of macro 'if' > > > > 270 | if (check_mul_overflow(factor1, factor2, &bytes)) > > > > | ^~ > > > > 'create_rule': event 2 > > > > ../arch/x86/include/asm/string_32.h:150:25: > > > > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > > > | | > > > > | (2) out of array bounds here > > > > ../security/landlock/ruleset.c:137:9: note: in expansion of macro 'memcpy' > > > > 137 | memcpy(new_rule->layers, layers, > > > > | ^~~~~~ > > > > make[1]: Leaving directory '/srv/code/gcc-bug' > > > > > > That's interesting... > > > > > > > > > > > > > > > I'll take a look at ways to make either the overflow macros or memcpy > > > > robust against this kind of weirdness... > > > > > > Thanks! > > > > I'm doing some build testing, but the below patch makes GCC happy. > > Alternatively we could make CONFIG_PROFILE_ALL_BRANCHES=y depend on > > CONFIG_FORTIFY_SOURCE=y ... > > > > > > From 6fbf66fdfd0a7dac809b77faafdd72c60112bb8d Mon Sep 17 00:00:00 2001 > > From: Kees Cook <kees@kernel.org> > > Date: Mon, 19 May 2025 11:52:06 -0700 > > Subject: [PATCH] string.h: Provide basic sanity checks for fallback memcpy() > > MIME-Version: 1.0 > > Content-Type: text/plain; charset=UTF-8 > > Content-Transfer-Encoding: 8bit > > > > Instead of defining memcpy() in terms of __builtin_memcpy() deep > > in arch/x86/include/asm/string_32.h, notice that it is needed up in > > the general string.h, as done with other common C String APIs. This > > allows us to add basic sanity checking for pathological "size" > > arguments to memcpy(). Besides the run-time checking benefit, this > > avoids GCC trying to be very smart about value range tracking[1] when > > CONFIG_PROFILE_ALL_BRANCHES=y but FORTIFY_SOURCE=n. > > It works for me but I couldn't reproduce the issue. I tried with > CONFIG_PROFILE_ALL_BRANCHES=y and CONFIG_FORTIFY_SOURCE=n but it always > works without a warning. I'm using GCC 15. Is it specific to a version > of GCC? It must be more than just those options -- I reproduced it with Randy's randconfig under GCC 15. -- Kees Cook ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-05-20 16:47 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250516202417.31b13d13@canb.auug.org.au>
2025-05-17 2:54 ` linux-next: Tree for May 16 (security/landlock/ruleset.c) Randy Dunlap
2025-05-19 15:29 ` Mickaël Salaün
2025-05-19 18:19 ` Kees Cook
2025-05-19 18:41 ` Mickaël Salaün
2025-05-19 19:15 ` Kees Cook
2025-05-19 20:26 ` Randy Dunlap
2025-05-20 16:44 ` Kees Cook
2025-05-20 14:01 ` Andy Shevchenko
2025-05-20 16:47 ` Kees Cook
2025-05-20 14:45 ` Mickaël Salaün
2025-05-20 15:48 ` Randy Dunlap
2025-05-20 16:15 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).