Re: [PATCH v2] fortify: Hide run-time copy size from value range tracking

[Nilay Shroff]

On 12/14/24 07:06, 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" <linux@xxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/db7190c8-d17f-4a0d-bc2f-5903c79f36c2@xxxxxxxx/
> Reported-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20241112124127.1666300-1-nilay@xxxxxxxxxxxxx/
> Acked-by: Yury Norov <yury.norov@xxxxxxxxx>
> Signed-off-by: Kees Cook <kees@xxxxxxxxxx>
> ---
> Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> Cc: "Qing Zhao" <qing.zhao@xxxxxxxxxx>
> Cc: linux-hardening@xxxxxxxxxxxxxxx
> 
>  v2: Make sure the expression statement ends with a single statement
>  v1: https://lore.kernel.org/all/20241213020929.work.498-kees@xxxxxxxxxx/
> ---
>  include/linux/fortify-string.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 0d99bf11d260..1eef0119671c 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -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);				\
>  })
>  
>  /*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,

Tested-By: nilay@xxxxxxxxxxxxx