| Message ID | 20241213020929.work.498-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fortify: Hide run-time copy size from value range tracking | expand |
On 12/13/24 07:39, Kees Cook wrote: > GCC performs value range tracking for variables as a way to provide better > diagnostics. One place this is regularly seen is with warnings associated > with bounds-checking, e.g. -Wstringop-overflow, -Wstringop-overread, > -Warray-bounds, etc. In order to keep the signal-to-noise ratio high, > warnings aren't emitted when a value range spans the entire value range > representable by a given variable. For example: > > unsigned int len; > char dst[8]; > ... > memcpy(dst, src, len); > > If len's value is unknown, it has the full "unsigned int" range of [0, > UINT_MAX], and bounds checks against memcpy() will be ignored. However, > when a code path has been able to narrow the range: > > if (len > 16) > return; > memcpy(dst, src, len); > > Then a range will be updated for the execution path. Above, len is now > [0, 16], so we might see a -Wstringop-overflow warning like: > > error: '__builtin_memcpy' writing between 9 and 16 bytes from to region of size 8 [-Werror=stringop-overflow] > > When building with CONFIG_FORTIFY_SOURCE, the run-time bounds checking > can appear to narrow value ranges for lengths for memcpy(), depending on > how the compile constructs the execution paths during optimization > passes, due to the checks on the size. For example: > > if (p_size_field != SIZE_MAX && > p_size != p_size_field && p_size_field < size) > > As intentionally designed, these checks only affect the kernel warnings > emitted at run-time and do not block the potentially overflowing memcpy(), > so GCC thinks it needs to produce a warning about the resulting value > range that might be reaching the memcpy(). > > We have seen this manifest a few times now, with the most recent being > with cpumasks: > > In function ‘bitmap_copy’, > inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2, > inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2: > ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread] > 114 | #define __underlying_memcpy __builtin_memcpy > | ^ > ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’ > 633 | __underlying_##op(p, q, __fortify_size); \ > | ^~~~~~~~~~~~~ > ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’ > 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \ > | ^~~~~~~~~~~~~~~~~~~~ > ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’ > 259 | memcpy(dst, src, len); > | ^~~~~~ > kernel/padata.c: In function ‘__padata_set_cpumasks’: > kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256] > 713 | cpumask_var_t pcpumask, > | ~~~~~~~~~~~~~~^~~~~~~~ > > This warning is _not_ emitted when CONFIG_FORTIFY_SOURCE is disabled, > and with the recent -fdiagnostics-details we can confirm the origin of > the warning is due to the FORTIFY range checking: > > ../include/linux/bitmap.h:259:17: note: in expansion of macro 'memcpy' > 259 | memcpy(dst, src, len); > | ^~~~~~ > '__padata_set_cpumasks': events 1-2 > ../include/linux/fortify-string.h:613:36: > 612 | if (p_size_field != SIZE_MAX && > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 613 | p_size != p_size_field && p_size_field < size) > | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ > | | > | (1) when the condition is evaluated to false > | (2) when the condition is evaluated to true > '__padata_set_cpumasks': event 3 > 114 | #define __underlying_memcpy __builtin_memcpy > | ^ > | | > | (3) out of array bounds here > > (Note that this warning started appearing since bitmap functions were > recently marked __always_inline, which allowed GCC to gain visibility > into the variables as they passed through the FORTIFY implementation.) > > In order to silence this false positive but keep deterministic > compile-time warnings intact, hide the length variable from GCC with > OPTIMIZE_HIDE_VAR() before calling the builtin memcpy. > > Reported-by: "Thomas Weißschuh" <linux@weissschuh.net> > Closes: https://lore.kernel.org/all/db7190c8-d17f-4a0d-bc2f-5903c79f36c2@t-8ch.de/ > Reported-by: Nilay Shroff <nilay@linux.ibm.com> > Closes: https://lore.kernel.org/all/20241112124127.1666300-1-nilay@linux.ibm.com/ > Acked-by: Yury Norov <yury.norov@gmail.com> > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Nathan Chancellor <nathan@kernel.org> > Cc: "Qing Zhao" <qing.zhao@oracle.com> > Cc: linux-hardening@vger.kernel.org > --- > include/linux/fortify-string.h | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h > index 0d99bf11d260..cab419f3a05f 100644 > --- a/include/linux/fortify-string.h > +++ b/include/linux/fortify-string.h > @@ -630,7 +630,16 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, > __fortify_size, \ > "field \"" #p "\" at " FILE_LINE, \ > __p_size_field); \ > - __underlying_##op(p, q, __fortify_size); \ > + if (__builtin_constant_p(__fortify_size)) { \ > + /* Pass through compile-time value for real warnings. */\ > + __underlying_##op(p, q, __fortify_size); \ > + } else { \ > + /* Hide the run-time size from compile-time value */ \ > + /* range tracking to silence false positives. */ \ > + size_t ___fortify_size = __fortify_size; \ > + OPTIMIZER_HIDE_VAR(___fortify_size); \ > + __underlying_##op(p, q, ___fortify_size); \ > + } \ > }) > > /* Thank you for the patch and explanation! I just applied your patch now I see following error while building latest upstream kernel using GCC 13.x (I tested it on x86-64 and PowerPC): In file included from ./include/linux/string.h:389, from ./arch/powerpc/include/asm/paca.h:16, from ./arch/powerpc/include/asm/current.h:13, from ./include/linux/thread_info.h:23, from ./include/asm-generic/preempt.h:5, from ./arch/powerpc/include/generated/asm/preempt.h:1, from ./include/linux/preempt.h:79, from ./include/linux/spinlock.h:56, from ./include/linux/wait.h:9, from ./include/linux/wait_bit.h:8, from ./include/linux/fs.h:6, from ./include/uapi/linux/aio_abi.h:31, from ./include/linux/syscalls.h:83, from fs/d_path.c:2: fs/d_path.c: In function ‘dynamic_dname’: ./include/linux/fortify-string.h:620:63: error: void value not ignored as it ought to be 620 | p_size_field, q_size_field, op) ({ \ | ~^~~~~~~~~~~ 621 | const size_t __fortify_size = (size_t)(size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 622 | const size_t __p_size = (p_size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 623 | const size_t __q_size = (q_size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 624 | const size_t __p_size_field = (p_size_field); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 625 | const size_t __q_size_field = (q_size_field); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 626 | fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 627 | __q_size, __p_size_field, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 628 | __q_size_field, FORTIFY_FUNC_ ##op), \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 629 | #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 630 | __fortify_size, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 631 | "field \"" #p "\" at " FILE_LINE, \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 632 | __p_size_field); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 633 | if (__builtin_constant_p(__fortify_size)) { \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 634 | /* Pass through compile-time value for real warnings. */\ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 635 | __underlying_##op(p, q, __fortify_size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 636 | } else { \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 637 | /* Hide the run-time size from compile-time value */ \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 638 | /* range tracking to silence false positives. */ \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 639 | size_t ___fortify_size = __fortify_size; \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 640 | OPTIMIZER_HIDE_VAR(___fortify_size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 641 | __underlying_##op(p, q, ___fortify_size); \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 642 | } \ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 643 | }) | ~~ ./include/linux/fortify-string.h:687:26: note: in expansion of macro ‘__fortify_memcpy_chk’ 687 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \ | ^~~~~~~~~~~~~~~~~~~~ fs/d_path.c:315:16: note: in expansion of macro ‘memcpy’ 315 | return memcpy(buffer, temp, sz); | ^~~~~~ fs/d_path.c:316:1: error: control reaches end of non-void function [-Werror=return-type] 316 | } | ^ cc1: all warnings being treated as errors make[3]: *** [scripts/Makefile.build:194: fs/d_path.o] Error 1 make[3]: *** Waiting for unfinished jobs.... THERE ARE many such more such instances of the error (as I am building in parallel and before build stops, I also see few more errors): In file included from ./include/linux/build_bug.h:5, from ./include/linux/bits.h:22, from ./include/linux/gfp_types.h:5, from ./include/linux/gfp.h:5, from ./include/linux/slab.h:16, from kernel/fork.c:16: kernel/fork.c: In function ‘vm_area_dup’: ./include/linux/compiler.h:212:21: error: variable or field ‘__v’ declared void 212 | __auto_type __v = (expr); \ | ^~~ kernel/fork.c:498:9: note: in expansion of macro ‘data_race’ 498 | data_race(memcpy(new, orig, sizeof(*new))); | ^~~~~~~~~ ./include/linux/compiler.h:212:27: error: void value not ignored as it ought to be 212 | __auto_type __v = (expr); \ | ^ kernel/fork.c:498:9: note: in expansion of macro ‘data_race’ 498 | data_race(memcpy(new, orig, sizeof(*new))); | ^~~~~~~~~ CC security/keys/key.o certs/blacklist.c: In function ‘get_raw_hash’: certs/blacklist.c:169:11: error: void value not ignored as it ought to be 169 | p = memcpy(buffer, type_prefix, type_len); | ^ make[3]: *** [scripts/Makefile.build:194: certs/blacklist.o] Error 1 make[2]: *** [scripts/Makefile.build:440: certs] Error 2 make[2]: *** Waiting for unfinished jobs.... make[3]: *** [scripts/Makefile.build:194: kernel/fork.o] Error 1 make[2]: *** [scripts/Makefile.build:440: kernel] Error 2 Please let me know if you need more information. Thanks, --Nilay
On Fri, Dec 13, 2024 at 02:49:20PM +0530, Nilay Shroff wrote:
> ./include/linux/fortify-string.h:620:63: error: void value not ignored as it ought to be
Whoops! Thanks, I had only spot-checked padata.o :( I will fix this up
and send a v2.
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h index 0d99bf11d260..cab419f3a05f 100644 --- a/include/linux/fortify-string.h +++ b/include/linux/fortify-string.h @@ -630,7 +630,16 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, __fortify_size, \ "field \"" #p "\" at " FILE_LINE, \ __p_size_field); \ - __underlying_##op(p, q, __fortify_size); \ + if (__builtin_constant_p(__fortify_size)) { \ + /* Pass through compile-time value for real warnings. */\ + __underlying_##op(p, q, __fortify_size); \ + } else { \ + /* Hide the run-time size from compile-time value */ \ + /* range tracking to silence false positives. */ \ + size_t ___fortify_size = __fortify_size; \ + OPTIMIZER_HIDE_VAR(___fortify_size); \ + __underlying_##op(p, q, ___fortify_size); \ + } \ }) /*
GCC performs value range tracking for variables as a way to provide better diagnostics. One place this is regularly seen is with warnings associated with bounds-checking, e.g. -Wstringop-overflow, -Wstringop-overread, -Warray-bounds, etc. In order to keep the signal-to-noise ratio high, warnings aren't emitted when a value range spans the entire value range representable by a given variable. For example: unsigned int len; char dst[8]; ... memcpy(dst, src, len); If len's value is unknown, it has the full "unsigned int" range of [0, UINT_MAX], and bounds checks against memcpy() will be ignored. However, when a code path has been able to narrow the range: if (len > 16) return; memcpy(dst, src, len); Then a range will be updated for the execution path. Above, len is now [0, 16], so we might see a -Wstringop-overflow warning like: error: '__builtin_memcpy' writing between 9 and 16 bytes from to region of size 8 [-Werror=stringop-overflow] When building with CONFIG_FORTIFY_SOURCE, the run-time bounds checking can appear to narrow value ranges for lengths for memcpy(), depending on how the compile constructs the execution paths during optimization passes, due to the checks on the size. For example: if (p_size_field != SIZE_MAX && p_size != p_size_field && p_size_field < size) As intentionally designed, these checks only affect the kernel warnings emitted at run-time and do not block the potentially overflowing memcpy(), so GCC thinks it needs to produce a warning about the resulting value range that might be reaching the memcpy(). We have seen this manifest a few times now, with the most recent being with cpumasks: In function ‘bitmap_copy’, inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2, inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2: ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread] 114 | #define __underlying_memcpy __builtin_memcpy | ^ ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’ 633 | __underlying_##op(p, q, __fortify_size); \ | ^~~~~~~~~~~~~ ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’ 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \ | ^~~~~~~~~~~~~~~~~~~~ ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’ 259 | memcpy(dst, src, len); | ^~~~~~ kernel/padata.c: In function ‘__padata_set_cpumasks’: kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256] 713 | cpumask_var_t pcpumask, | ~~~~~~~~~~~~~~^~~~~~~~ This warning is _not_ emitted when CONFIG_FORTIFY_SOURCE is disabled, and with the recent -fdiagnostics-details we can confirm the origin of the warning is due to the FORTIFY range checking: ../include/linux/bitmap.h:259:17: note: in expansion of macro 'memcpy' 259 | memcpy(dst, src, len); | ^~~~~~ '__padata_set_cpumasks': events 1-2 ../include/linux/fortify-string.h:613:36: 612 | if (p_size_field != SIZE_MAX && | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ 613 | p_size != p_size_field && p_size_field < size) | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~ | | | (1) when the condition is evaluated to false | (2) when the condition is evaluated to true '__padata_set_cpumasks': event 3 114 | #define __underlying_memcpy __builtin_memcpy | ^ | | | (3) out of array bounds here (Note that this warning started appearing since bitmap functions were recently marked __always_inline, which allowed GCC to gain visibility into the variables as they passed through the FORTIFY implementation.) In order to silence this false positive but keep deterministic compile-time warnings intact, hide the length variable from GCC with OPTIMIZE_HIDE_VAR() before calling the builtin memcpy. Reported-by: "Thomas Weißschuh" <linux@weissschuh.net> Closes: https://lore.kernel.org/all/db7190c8-d17f-4a0d-bc2f-5903c79f36c2@t-8ch.de/ Reported-by: Nilay Shroff <nilay@linux.ibm.com> Closes: https://lore.kernel.org/all/20241112124127.1666300-1-nilay@linux.ibm.com/ Acked-by: Yury Norov <yury.norov@gmail.com> Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Nathan Chancellor <nathan@kernel.org> Cc: "Qing Zhao" <qing.zhao@oracle.com> Cc: linux-hardening@vger.kernel.org --- include/linux/fortify-string.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)