lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 11 Feb 2022 17:31:22 -0800 From: Kees Cook <keescook@...omium.org> To: Jakub Kicinski <kuba@...nel.org> Cc: Marc Kleine-Budde <mkl@...gutronix.de>, Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net> Subject: Re: ether_addr_equal_64bits breakage with gcc-12 On Fri, Feb 11, 2022 at 04:35:41PM -0800, Jakub Kicinski wrote: > On Fri, 11 Feb 2022 15:12:13 +0100 Marc Kleine-Budde wrote: > > Hello, > > > > the current arm-linux-gnueabihf-gcc 12 snapshot in Debian breaks (at > > least with CONFIG_WERROR=y): > > > > | CC net/core/dev.o > > | net/core/dev.c: In function ‘bpf_prog_run_generic_xdp’: > > | net/core/dev.c:4618:21: warning: ‘ether_addr_equal_64bits’ reading 8 bytes from a region of size 6 [-Wstringop-overread] > > | 4618 | orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr); > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4618:21: note: referencing argument 1 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | net/core/dev.c:4618:21: note: referencing argument 2 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | In file included from net/core/dev.c:91: > > | include/linux/etherdevice.h:375:20: note: in a call to function ‘ether_addr_equal_64bits’ > > | 375 | static inline bool ether_addr_equal_64bits(const u8 addr1[6+2], > > | | ^~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4619:22: warning: ‘is_multicast_ether_addr_64bits’ reading 8 bytes from a region of size 6 [-Wstringop-overread] > > | 4619 | orig_bcast = is_multicast_ether_addr_64bits(eth->h_dest); > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4619:22: note: referencing argument 1 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | include/linux/etherdevice.h:137:20: note: in a call to function ‘is_multicast_ether_addr_64bits’ > > | 137 | static inline bool is_multicast_ether_addr_64bits(const u8 addr[6+2]) > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4646:27: warning: ‘ether_addr_equal_64bits’ reading 8 bytes from a region of size 6 [-Wstringop-overread] > > | 4646 | (orig_host != ether_addr_equal_64bits(eth->h_dest, > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | 4647 | skb->dev->dev_addr)) || > > | | ~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4646:27: note: referencing argument 1 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | net/core/dev.c:4646:27: note: referencing argument 2 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | include/linux/etherdevice.h:375:20: note: in a call to function ‘ether_addr_equal_64bits’ > > | 375 | static inline bool ether_addr_equal_64bits(const u8 addr1[6+2], > > | | ^~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4648:28: warning: ‘is_multicast_ether_addr_64bits’ reading 8 bytes from a region of size 6 [-Wstringop-overread] > > | 4648 | (orig_bcast != is_multicast_ether_addr_64bits(eth->h_dest))) { > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/core/dev.c:4648:28: note: referencing argument 1 of type ‘const u8[8]’ {aka ‘const unsigned char[8]’} > > | include/linux/etherdevice.h:137:20: note: in a call to function ‘is_multicast_ether_addr_64bits’ > > | 137 | static inline bool is_multicast_ether_addr_64bits(const u8 addr[6+2]) > > | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > | arm-linux-gnueabihf-gcc -v > > | Using built-in specs. > > | COLLECT_GCC=/usr/bin/arm-linux-gnueabihf-gcc > > | COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabihf/12/lto-wrapper > > | Target: arm-linux-gnueabihf > > | Configured with: ../src/configure -v --with-pkgversion='Debian 12-20220126-1' --with-bugurl=file:///usr/share/doc/gcc-12/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-12 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --without-target-system-zlib --enable-multiarch --disable-sjlj-exceptions --with-arch=armv7-a+fp --with-float=hard --with-mode=thumb --disable-werror --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=arm-linux-gnueabihf --program-prefix=arm-linux-gnueabihf- --includedir=/usr/arm-linux-gnueabihf/include > > | Thread model: posix > > | Supported LTO compression algorithms: zlib zstd > > | gcc version 12.0.1 20220126 (experimental) [master r12-6872-gf3e6ef7d873] (Debian 12-20220126-1) > > Maybe Kees will have as suggestion - Kees, are there any best practices > for dealing with such issues? For the reference we do a oversized load > from a structure (read 8B of a 6B array): Wheee. So, the short theoretical "don't do that" scenario would be "what happens if": struct page *page; void *ptr; unsigned char *eth_addr; page = alloc_pages(GFP_KERNEL, 0); ... ptr = page_address(page); ... /* "eth_addr" at end of allocated memory */ eth_addr = ptr + PAGE_SIZE - 6; /* access fault... */ ether_addr_equal_64bits(eth_addr, ...); But, yes, pragmatically, this is likely extremely rare. Regardless, with the other cases like this that got fixed like this, it was a matter of finding a way to represent the "actual" available memory (best), or telling the compiler what real contract is (less good). It looks like alignment isn't a concern, so I'd say adjust the prototype to reflect the reality, and go with: diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h index 2ad71cc90b37..92b10e67d5f8 100644 --- a/include/linux/etherdevice.h +++ b/include/linux/etherdevice.h @@ -134,7 +134,7 @@ static inline bool is_multicast_ether_addr(const u8 *addr) #endif } -static inline bool is_multicast_ether_addr_64bits(const u8 addr[6+2]) +static inline bool is_multicast_ether_addr_64bits(const u8 *addr) { #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64 #ifdef __BIG_ENDIAN @@ -372,8 +372,7 @@ static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2) * Please note that alignment of addr1 & addr2 are only guaranteed to be 16 bits. */ -static inline bool ether_addr_equal_64bits(const u8 addr1[6+2], - const u8 addr2[6+2]) +static inline bool ether_addr_equal_64bits(const u8 *addr1, const u8 *addr2) { #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64 u64 fold = (*(const u64 *)addr1) ^ (*(const u64 *)addr2); -- Kees Cook
Powered by blists - more mailing lists