Re: Fortify compilation warning in __padata_set_cpumask()

[Kees Cook <kees@kernel.org>]

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