On Fri, Dec 13, 2024 at 05:36:10PM -0800, 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 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.
>
> Reported-by: "Thomas Weißschuh" <li...@weissschuh.net>
> Closes:
> https://lore.kernel.org/all/db7190c8-d17f-4a0d-bc2f-5903c79f3...@t-8ch.de/
> Reported-by: Nilay Shroff <ni...@linux.ibm.com>
> Closes:
> https://lore.kernel.org/all/20241112124127.1666300-1-ni...@linux.ibm.com/
> Acked-by: Yury Norov <yury.no...@gmail.com>
> Signed-off-by: Kees Cook <k...@kernel.org>
> ---
Fixed the build issues I have here, so:
Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
