fortify: Hide run-time copy size from value range tracking
Kees Cook
maintainerPATCH v2 1/1 fortify: Hide run-time copy size from value range tracking
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 hereNote that this warning started appearing since bitmap functions were recently marked __always_inline in commit ed8cd2b3bd9f ("bitmap: Switch from inline to __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.
Additionally add a comment about why all the macro args have copies with const storage.
---
Cc: Nathan Chancellor <[email protected]>
Cc: "Qing Zhao" <[email protected]>
Cc: [email protected]
v2: Make sure the expression statement ends with a single statement
v1: https://lore.kernel.org/all/[email protected]/
--- include/linux/fortify-string.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
include/linux/fortify-string.h +13 -1
@@ -616,6 +616,12 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,return false; } +/* +* To work around what seems to be an optimizer bug, the macro arguments +* need to have const copies or the values end up changed by the time they +* reach fortify_warn_once(). See commit 6f7630b1b5bc ("fortify: Capture +* __bos() results in const temp vars") for more details. +*/ #define __fortify_memcpy_chk (p, q, size, p_size, q_size, \ p_size_field, q_size_field, op) ({ \ const size_t __fortify_size = (size_t)(size); \
@@ -623,6 +629,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,const size_t __q_size = (q_size); \const size_t __p_size_field = (p_size_field); \const size_t __q_size_field = (q_size_field); \ +/* Keep a mutable version of the size for the final copy. */ \ +size_t __copy_size = __fortify_size; \ fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size, \ __q_size, __p_size_field, \ __q_size_field, FORTIFY_FUNC_ ##op), \
@@ -630,7 +638,11 @@ __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); \ +/* Hide only the run-time size from value range tracking to */ \ +/* silence compile-time false positive bounds warnings. */ \ +if (!__builtin_constant_p (__fortify_size)) \ +OPTIMIZER_HIDE_VAR (__copy_size); \ + __underlying_##op(p, q, __copy_size); \ })/*
Signature · 1 line
2.34.1
Quoted 95 lines · Kees Cook
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 againstmemcpy()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 formemcpy(), 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 thememcpy().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’ atkernel/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_SOURCEis 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 hereNote that this warning started appearing since bitmap functions were recently marked
__always_inlinein commited8cd2b3bd9f("bitmap: Switch from inline to__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.Additionally add a comment about why all the macro args have copies with const storage.
Quoted 7 lines
Reported-by "Thomas Weißschuh" <[email protected]>Reported-by Nilay Shroff <[email protected]>Acked-by Yury Norov <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Fixed the build issues I have here, so:
Quoted 141 lines · Kees Cook
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 againstmemcpy()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 formemcpy(), 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 thememcpy().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’ atkernel/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_SOURCEis 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 hereNote that this warning started appearing since bitmap functions were recently marked
__always_inlinein commited8cd2b3bd9f("bitmap: Switch from inline to__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.Additionally add a comment about why all the macro args have copies with const storage.
Reported-by "Thomas Weißschuh" <[email protected]>Reported-by Nilay Shroff <[email protected]>Acked-by Yury Norov <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Cc: Nathan Chancellor <[email protected]>
Cc: "Qing Zhao" <[email protected]>
Cc: [email protected]v2: Make sure the expression statement ends with a single statement
v1: https://lore.kernel.org/all/[email protected]/--- include/linux/fortify-string.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)include/linux/fortify-string.h +8
-0@@ -616,6 +616,12 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,return false; } +/* +* To work around what seems to be an optimizer bug, the macro arguments +* need to have const copies or the values end up changed by the time they +* reach fortify_warn_once(). See commit 6f7630b1b5bc ("fortify: Capture +* __bos() results in const temp vars") for more details. +*/ #define __fortify_memcpy_chk (p, q, size, p_size, q_size, \ p_size_field, q_size_field, op) ({ \ const size_t __fortify_size = (size_t)(size); \@@ -623,6 +629,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,const size_t __q_size = (q_size); \const size_t __p_size_field = (p_size_field); \const size_t __q_size_field = (q_size_field); \ +/* Keep a mutable version of the size for the final copy. */ \ +size_t __copy_size = __fortify_size; \ fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size, \ __q_size, __p_size_field, \ __q_size_field, FORTIFY_FUNC_ ##op), \@@ -630,7 +638,11 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, __fortify_size, \ "field \"" #p "\" at " FILE_LINE, \
include/linux/fortify-string.h __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, · line 648
Quoted 10 lines
include/linux/fortify-string.h +5
-1__p_size_field); \ - __underlying_##op(p, q, __fortify_size); \ +/* Hide only the run-time size from value range tracking to */ \ +/* silence compile-time false positive bounds warnings. */ \ +if (!__builtin_constant_p (__fortify_size)) \ +OPTIMIZER_HIDE_VAR (__copy_size); \ + __underlying_##op(p, q, __copy_size); \ })/*This patch works for me. I tested it on PowerPC and x86-64 using GCC 13.X,
CONFIG_FORTIFY_SOURCE=Y and CONFIG_NR_CPUS=2048. So,
From: Kees Cook
Quoted 1 line
Sent: 14 December 2024 01:36
...
Quoted 3 lines
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.
...
Quoted 27 lines
include/linux/fortify-string.h +8
-0@@ -616,6 +616,12 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,return false; } +/* +* To work around what seems to be an optimizer bug, the macro arguments +* need to have const copies or the values end up changed by the time they +* reach fortify_warn_once(). See commit 6f7630b1b5bc ("fortify: Capture +* __bos() results in const temp vars") for more details. +*/ #define __fortify_memcpy_chk (p, q, size, p_size, q_size, \ p_size_field, q_size_field, op) ({ \ const size_t __fortify_size = (size_t)(size); \@@ -623,6 +629,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,const size_t __q_size = (q_size); \const size_t __p_size_field = (p_size_field); \const size_t __q_size_field = (q_size_field); \ +/* Keep a mutable version of the size for the final copy. */ \ +size_t __copy_size = __fortify_size; \ fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size, \ __q_size, __p_size_field, \ __q_size_field, FORTIFY_FUNC_ ##op), \
include/linux/fortify-string.h __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, · line 644
Quoted 8 lines
include/linux/fortify-string.h +4
-1@@ -630,7 +638,11 @@ __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); \ +/* Hide only the run-time size from value range tracking to */ \ +/* silence compile-time false positive bounds warnings. */ \ +if (!__builtin_constant_p (__fortify_size)) \ +OPTIMIZER_HIDE_VAR (__copy_size); \
I think you can make that:
if (!__builtin_constant_p(__copy_size)) \ OPTIMISER_HIDE_VAR(__copy_size) \
which is probably more readable.
David
Quoted 7 lines
include/linux/fortify-string.h +1
-1+ __underlying_##op(p, q, __copy_size); \ })/* -- 2.34.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Kees Cook
maintainerQuoted 44 lines · David Laight
From: Kees Cook
Quoted 1 line
Sent: 14 December 2024 01:36
...
Quoted 3 lines
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....
Quoted 27 lines
include/linux/fortify-string.h +8
-0@@ -616,6 +616,12 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,return false; } +/* +* To work around what seems to be an optimizer bug, the macro arguments +* need to have const copies or the values end up changed by the time they +* reach fortify_warn_once(). See commit 6f7630b1b5bc ("fortify: Capture +* __bos() results in const temp vars") for more details. +*/ #define __fortify_memcpy_chk (p, q, size, p_size, q_size, \ p_size_field, q_size_field, op) ({ \ const size_t __fortify_size = (size_t)(size); \@@ -623,6 +629,8 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,const size_t __q_size = (q_size); \const size_t __p_size_field = (p_size_field); \const size_t __q_size_field = (q_size_field); \ +/* Keep a mutable version of the size for the final copy. */ \ +size_t __copy_size = __fortify_size; \ fortify_warn_once(fortify_memcpy_chk(__fortify_size, __p_size, \ __q_size, __p_size_field, \ __q_size_field, FORTIFY_FUNC_ ##op), \include/linux/fortify-string.h __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size, · line 644
Quoted 8 lines
include/linux/fortify-string.h +4
-1@@ -630,7 +638,11 @@ __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); \ +/* Hide only the run-time size from value range tracking to */ \ +/* silence compile-time false positive bounds warnings. */ \ +if (!__builtin_constant_p (__fortify_size)) \ +OPTIMIZER_HIDE_VAR (__copy_size); \I think you can make that:
Quoted 3 lines
if (!__builtin_constant_p(__copy_size)) \ OPTIMISER_HIDE_VAR(__copy_size) \which is probably more readable.
Yeah, that tests out fine. I've updated it locally. Thanks!
-Kees
Signature · 1 line
Kees Cook