* Re: Fortify compilation warning in __padata_set_cpumask()
2024-10-31 20:40 Fortify compilation warning in __padata_set_cpumask() Thomas Weißschuh
@ 2024-11-02 20:43 ` Kees Cook
0 siblings, 0 replies; 2+ messages in thread
From: Kees Cook @ 2024-11-02 20:43 UTC (permalink / raw)
To: Thomas Weißschuh; +Cc: linux-hardening, linux-kernel
On Thu, Oct 31, 2024 at 08:40:33PM +0000, Thomas Weißschuh wrote:
> Hi Kees,
>
> I'm running into compilation warnings/errors due to fortify-string.h.
>
> Environment:
> - Commit 0fc810ae3ae110f9e2fcccce80fc8c8d62f97907 (current mainline master)
> - gcc (GCC) 14.2.1 20240910
> - Relevant config (from an Arch Linux distro config):
> CONFIG_64BIT=y
> CONFIG_X86_64=y
> CONFIG_NR_CPUS=320
> CONFIG_NR_CPUS_RANGE_BEGIN=2
> CONFIG_NR_CPUS_RANGE_END=512
> CONFIG_NR_CPUS_RANGE_DEFAULT=64
> CONFIG_PADATA=y
>
> Warning:
>
> CC kernel/padata.o
> In file included from ./include/linux/string.h:390,
> from ./include/linux/bitmap.h:13,
> from ./include/linux/cpumask.h:12,
> from ./arch/x86/include/asm/paravirt.h:21,
> from ./arch/x86/include/asm/irqflags.h:80,
> from ./include/linux/irqflags.h:18,
> from ./include/linux/spinlock.h:59,
> from ./include/linux/swait.h:7,
> from ./include/linux/completion.h:12,
> from kernel/padata.c:14:
> 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 41 and 536870904 bytes from a region of size 40 [-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, 40]
> 713 | cpumask_var_t pcpumask,
> | ~~~~~~~~~~~~~~^~~~~~~~
> 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 41 and 536870904 bytes from a region of size 40 [-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, 40]
> 713 | cpumask_var_t pcpumask,
> | ~~~~~~~~~~~~~~^~~~~~~~
> cc1: all warnings being treated as errors
>
> Code:
>
> 712 static int __padata_set_cpumasks(struct padata_instance *pinst,
> 713 cpumask_var_t pcpumask,
> 714 cpumask_var_t cbcpumask)
> 715 {
> 716 int valid;
> 717 int err;
> 718
> 719 valid = padata_validate_cpumask(pinst, pcpumask);
> 720 if (!valid) {
> 721 __padata_stop(pinst);
> 722 goto out_replace;
> 723 }
> 724
> 725 valid = padata_validate_cpumask(pinst, cbcpumask);
> 726 if (!valid)
> 727 __padata_stop(pinst);
> 728
> 729 out_replace:
> 730 cpumask_copy(pinst->cpumask.pcpu, pcpumask);
> 731 cpumask_copy(pinst->cpumask.cbcpu, cbcpumask);
> 732
> 733 err = padata_setup_cpumasks(pinst) ?: padata_replace(pinst);
> 734
> 735 if (valid)
> 736 __padata_start(pinst);
> 737
> 738 return err;
> 739 }
>
>
> The weird thing is, that only the cpumask_copy() in line 730 triggers
> this warning. The one in line 731 doesn't. Also this is the only
> instance of the warning I see in the whole build.
>
> The warning goes away with the following change, but that introduces
> runtime overhead and feels wrong. Also it doesn't explain why this
> specific call is different from all others.
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 9278a50d514f..ded9d1bcef03 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -836,7 +838,7 @@ void cpumask_shift_left(struct cpumask *dstp, const struct cpumask *srcp, int n)
> static __always_inline
> void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
> {
> - bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), large_cpumask_bits);
> + bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), MIN(NR_CPUS, large_cpumask_bits));
> }
>
> /**
>
> Any ideas?
My notes on looking at this:
#define DECLARE_BITMAP(name,bits) \
unsigned long name[BITS_TO_LONGS(bits)]
...
typedef struct cpumask { DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
#ifdef CONFIG_CPUMASK_OFFSTACK
typedef struct cpumask *cpumask_var_t;
#else
typedef struct cpumask cpumask_var_t[1];
#endif /* CONFIG_CPUMASK_OFFSTACK */
...
#define cpumask_bits(maskp) ((maskp)->bits)
...
int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
cpumask_var_t cpumask)
...
struct cpumask *serial_mask, *parallel_mask;
...
parallel_mask = cpumask;
...or...
parallel_mask = pinst->cpumask.pcpu;
...
err = __padata_set_cpumasks(pinst, parallel_mask, serial_mask);
...
static int __padata_set_cpumasks(struct padata_instance *pinst,
cpumask_var_t pcpumask,
cpumask_var_t cbcpumask)
...
cpumask_copy(pinst->cpumask.cbcpu, cbcpumask);
...
static __always_inline
void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
{
bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp),
large_cpumask_bits);
}
...
extern unsigned int nr_cpu_ids;
...
#if NR_CPUS <= BITS_PER_LONG // false: 320 <= 64
#define small_cpumask_bits ((unsigned int)NR_CPUS)
#define large_cpumask_bits ((unsigned int)NR_CPUS)
#elif NR_CPUS <= 4*BITS_PER_LONG // false: 320 <= 256
#define small_cpumask_bits nr_cpu_ids
#define large_cpumask_bits ((unsigned int)NR_CPUS)
#else
#define small_cpumask_bits nr_cpu_ids
#define large_cpumask_bits nr_cpu_ids // not a compile-time constant
#endif
...
static __always_inline
void bitmap_copy(unsigned long *dst, const unsigned long *src, unsigned
int nbits)
{
unsigned int len = bitmap_size(nbits);
if (small_const_nbits(nbits))
*dst = *src;
else
memcpy(dst, src, len);
}
So the result is:
memcpy(pcpumask->bits, cbcpumask->bits, nr_cpu_ids)
And the error is that the compiler has determined that nc_cpu_ids got
limited to possibly have a value between 41 and 536870904. This is an
odd limit, though: 0x1FFFFFF8, but does feel constructable -- it's close
to some magic numbers.
There are some new diagnostics being added to GCC that might help track
this down[1]. I'm waiting for a v4 before I take it for a spin.
-Kees
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-October/666870.html
--
Kees Cook
^ permalink raw reply [flat|nested] 2+ messages in thread