kernel: fortify: Hide run-time copy size from value range tracking
Fix compilation warning treated as an error:
./include/linux/fortify-string.h:114:33: error: '__builtin_memcpy' reading between 65 and 536870904 bytes from a region of size 64 [-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:735:48: note: source object 'pcpumask' of size [0, 64]
735 | cpumask_var_t pcpumask,
| ~~~~~~~~~~~~~~^~~~~~~~
Link: https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=239d87327dcd361b0098038995f8908f3296864f
Signed-off-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
Link: https://github.com/openwrt/openwrt/pull/16547
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
This commit is contained in:
Mieczyslaw Nalewaj
Christian Marangi
parent
aa69e02002
commit
7e5bccd011
1 changed files with 155 additions and 0 deletions
|
|
@ -0,0 +1,155 @@
|
|||
From 239d87327dcd361b0098038995f8908f3296864f Mon Sep 17 00:00:00 2001
|
||||
From: Kees Cook <kees@kernel.org>
|
||||
Date: Thu, 12 Dec 2024 17:28:06 -0800
|
||||
Subject: fortify: Hide run-time copy size from value range tracking
|
||||
MIME-Version: 1.0
|
||||
Content-Type: text/plain; charset=UTF-8
|
||||
Content-Transfer-Encoding: 8bit
|
||||
|
||||
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 GCC's compile-time 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 the range will be updated for the execution path. Above, len is
|
||||
now [0, 16] when reading memcpy(), so depending on other optimizations,
|
||||
we might see a -Wstringop-overflow warning like:
|
||||
|
||||
error: '__builtin_memcpy' writing between 9 and 16 bytes into region of size 8 [-Werror=stringop-overflow]
|
||||
|
||||
When building with CONFIG_FORTIFY_SOURCE, the fortified run-time bounds
|
||||
checking can appear to narrow value ranges of lengths for memcpy(),
|
||||
depending on how the compiler constructs the execution paths during
|
||||
optimization passes, due to the checks against the field sizes. 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 FORTIFY's bounds 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 the cpumask 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 these false positives but keep otherwise 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" <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/
|
||||
Tested-by: Nilay Shroff <nilay@linux.ibm.com>
|
||||
Acked-by: Yury Norov <yury.norov@gmail.com>
|
||||
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
|
||||
Signed-off-by: Kees Cook <kees@kernel.org>
|
||||
---
|
||||
include/linux/fortify-string.h | 14 +++++++++++++-
|
||||
1 file changed, 13 insertions(+), 1 deletion(-)
|
||||
|
||||
--- a/include/linux/fortify-string.h
|
||||
+++ b/include/linux/fortify-string.h
|
||||
@@ -616,6 +616,12 @@ __FORTIFY_INLINE bool fortify_memcpy_chk
|
||||
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
|
||||
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
|
||||
__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(__copy_size)) \
|
||||
+ OPTIMIZER_HIDE_VAR(__copy_size); \
|
||||
+ __underlying_##op(p, q, __copy_size); \
|
||||
})
|
||||
|
||||
/*
|
||||
Loading…
Reference in a new issue