v4.4, and possibly older kernels: Enable CONFIG_FORTIFY_SOURCE |
||||
Issue description
Upstream kernel has a335de3e2e9("include/linux/string.h: add the option of fortified string.h functions") which adds support for a rough equivalent of _FORTIFY_SOURCE=1.
v4.14 and newer kernels have CONFIG_FORTIFY set.
Older kernels that might have missed out on security fixes would be able to benefit from these runtime copy-size checks.
,
Nov 2
coral-pre-cq https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8931029493884396528 bob-pre-cq https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8931029490835751056 kevin-pre-cq https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8931029487712511376 bob-paladin-tryjob https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8931029481996358160 coral-paladin and kevin-paladin have been having repeated tryjob testlabfailures.
,
Nov 2
,
Nov 2
Correction in #1: CONFIG_FORTIFY is not set in v4.14, however the commit that introduces the CONFIG is present.
,
Nov 2
Do you mean FORTIFY_SOURCE, by any change ? I don't see CONFIG_FORTIFY in chromeos-4.14.
,
Nov 2
Yes, thanks, I meant FORTIFY_SOURCE.
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/9cf575403809ec6608d20c624244c9ed96ac1b88 commit 9cf575403809ec6608d20c624244c9ed96ac1b88 Author: Philipp Rudo <prudo@linux.vnet.ibm.com> Date: Wed Nov 07 14:33:50 2018 UPSTREAM: kernel/kexec_file.c: remove checks in kexec_purgatory_load Before the purgatory is loaded several checks are done whether the ELF file in kexec_purgatory is valid or not. These checks are incomplete. For example they don't check for the total size of the sections defined in the section header table or if the entry point actually points into the purgatory. On the other hand the purgatory, although an ELF file on its own, is part of the kernel. Thus not trusting the purgatory means not trusting the kernel build itself. So remove all validity checks on the purgatory and just trust the kernel build. BUG=chromium:901062 TEST=Build and boot Change-Id: Ic2692dc75b7ed7a8af96c0bf2cdc58f9ba323857 Link: http://lkml.kernel.org/r/20180321112751.22196-3-prudo@linux.vnet.ibm.com Signed-off-by: Philipp Rudo <prudo@linux.vnet.ibm.com> Acked-by: Dave Young <dyoung@redhat.com> Cc: AKASHI Takahiro <takahiro.akashi@linaro.org> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Heiko Carstens <heiko.carstens@de.ibm.com> Cc: Ingo Molnar <mingo@kernel.org> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> Cc: Vivek Goyal <vgoyal@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> (cherry picked from commit d2b8178ca7324a21495cb71049b4e4a041ab5942) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313418 [modify] https://crrev.com/9cf575403809ec6608d20c624244c9ed96ac1b88/kernel/kexec_file.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/b6b4a5b563ca2133c5927e486d0bcf052dd82ff9 commit b6b4a5b563ca2133c5927e486d0bcf052dd82ff9 Author: Kees Cook <keescook@chromium.org> Date: Wed Nov 07 14:33:51 2018 USPTREAM: bna: ethtool: Avoid reading past end of buffer Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, use strncpy() which will fill the trailing bytes with zeros. This was found with the future CONFIG_FORTIFY_SOURCE feature. BUG=chromium:901062 TEST=Build and boot Change-Id: I8c3261987a95af8fcd85ac04b1109d7bb5b0c4bb Cc: Daniel Micay <danielmicay@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: David S. Miller <davem@davemloft.net> (cherry picked from commit 4dc69c1c1fff2f587f8e737e70b4a4e7565a5c94) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313419 [modify] https://crrev.com/b6b4a5b563ca2133c5927e486d0bcf052dd82ff9/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/bf0698a172a8af2f609a14f5a714aff014085163 commit bf0698a172a8af2f609a14f5a714aff014085163 Author: Kees Cook <keescook@chromium.org> Date: Wed Nov 07 14:33:53 2018 UPSTREAM: libertas: Avoid reading past end of buffer Using memcpy() from a string that is shorter than the length copied means the destination buffer is being filled with arbitrary data from the kernel rodata segment. Instead, redefine the stat strings to be ETH_GSTRING_LEN sizes, like other drivers. This lets us use a single memcpy that does not leak rodata contents. Additionally adjust indentation to keep checkpatch.pl happy. This was found with the future CONFIG_FORTIFY_SOURCE feature. BUG=chromium:901062 TEST=Build and boot Change-Id: I627a1ab501c9e0508b849cf1273cca2553a07716 Cc: Daniel Micay <danielmicay@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> (cherry picked from commit 12e3c0433e8a3b817fbb978a1be973a04cd5d6f3) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313420 [modify] https://crrev.com/bf0698a172a8af2f609a14f5a714aff014085163/drivers/net/wireless/marvell/libertas/mesh.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/3c4823f9ac595b43f495583d3bf59f3a93f3974e commit 3c4823f9ac595b43f495583d3bf59f3a93f3974e Author: Kees Cook <keescook@chromium.org> Date: Wed Nov 07 14:33:54 2018 UPSTREAM: scsi: csiostor: Avoid content leaks and casts When copying attributes, the len argument was padded out and the resulting memcpy() would copy beyond the end of the source buffer. Avoid this, and use size_t for val_len to avoid all the casts. Similarly, avoid source buffer casts and use void *. Additionally enforces val_len can be represented by u16 and that the DMA buffer was not overflowed. Fixes the size of mfa, which is not FC_FDMI_PORT_ATTR_MAXFRAMESIZE_LEN (but it will be padded up to 4). This was noticed by the future CONFIG_FORTIFY_SOURCE checks. BUG=chromium:901062 TEST=Build and boot Change-Id: I76a42216704f534de57a46dd9cd7268413d95d06 Cc: Daniel Micay <danielmicay@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Varun Prakash <varun@chelsio.com> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> (cherry picked from commit 42c335f7e67029d2e01711f2f2bc6252277c8993) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313421 [modify] https://crrev.com/3c4823f9ac595b43f495583d3bf59f3a93f3974e/drivers/scsi/csiostor/csio_lnode.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/183f492e8d0a2c3ea0320255fdb7d120099ef763 commit 183f492e8d0a2c3ea0320255fdb7d120099ef763 Author: Sudip Mukherjee <sudipm.mukherjee@gmail.com> Date: Wed Nov 07 14:33:55 2018 UPSTREAM: matroxfb: fix size of memcpy hw->DACreg has a size of 80 bytes and MGADACbpp32 has 21. So when memcpy copies MGADACbpp32 to hw->DACreg it copies 80 bytes but only 21 bytes are valid. BUG=chromium:901062 TEST=Build and boot Change-Id: I1991231efb91a2e931b5d5761bd6c6bc2177447c Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> (cherry picked from commit 59921b239056fb6389a865083284e00ce0518db6) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313422 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/183f492e8d0a2c3ea0320255fdb7d120099ef763/drivers/video/fbdev/matrox/matroxfb_Ti3026.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e7d32651511483ac340eab98a044ff0a2a18d181 commit e7d32651511483ac340eab98a044ff0a2a18d181 Author: Kees Cook <keescook@chromium.org> Date: Wed Nov 07 14:33:57 2018 BACKPORT: arm64, vdso: Define vdso_{start,end} as array Adjust vdso_{start|end} to be char arrays to avoid compile-time analysis that flags "too large" memcmp() calls with CONFIG_FORTIFY_SOURCE. BUG=chromium:901062 TEST=Build and boot Backport Note: - Upstream patch sets vdso_pagelist[i] to pfn_to_page(...). In v4.4, we use vdso_start, so fix the type there as well. Change-Id: I7dac49291f07d6ef189f10e9e638fd9f5e02b4b7 Cc: Jisheng Zhang <jszhang@marvell.com> Acked-by: Catalin Marinas <catalin.marinas@arm.com> Suggested-by: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Will Deacon <will.deacon@arm.com> (cherry picked from commit dbbb08f500d6146398b794fdc68a8e811366b451) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313423 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/e7d32651511483ac340eab98a044ff0a2a18d181/arch/arm64/kernel/vdso.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/a08657f74026c14de2f20e7fab2804407b6ad98a commit a08657f74026c14de2f20e7fab2804407b6ad98a Author: Zubin Mithra <zsm@chromium.org> Date: Wed Nov 07 14:33:58 2018 CHROMIUM: lkdtm: Change memset usage to use size of destination When CONFIG_FORTIFY_SOURCE is enabled and the kernel is built with allmodconfig there is the following compile time error. In function memset, inlined from corrupt_stack at drivers/misc/lkdtm.c:338:2: include/linux/string.h:278:3: error: call to __write_overflow declared with attribute error: detected write beyond size of object passed as 1st parameter __write_overflow(); ^~~~~~~~~~~~~~~~~~ Upstream commit 93e78c6b14c("lkdtm: Add -fstack-protector-strong test") deals with this by moving this memset into another function. Make a similar change to avoid the compile time error. BUG=chromium:901062 TEST=Build and boot Change-Id: I5cc92fb263da556c46ef1ea9ac63b25e853317dc Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313424 Reviewed-by: Kees Cook <keescook@chromium.org> [modify] https://crrev.com/a08657f74026c14de2f20e7fab2804407b6ad98a/drivers/misc/lkdtm.c
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/d65257b2c9e68a5b44590195f0bacd323a567786 commit d65257b2c9e68a5b44590195f0bacd323a567786 Author: Daniel Micay <danielmicay@gmail.com> Date: Wed Nov 07 14:34:00 2018 UPSTREAM: include/linux/string.h: add the option of fortified string.h functions This adds support for compiling with a rough equivalent to the glibc _FORTIFY_SOURCE=1 feature, providing compile-time and runtime buffer overflow checks for string.h functions when the compiler determines the size of the source or destination buffer at compile-time. Unlike glibc, it covers buffer reads in addition to writes. GNU C __builtin_*_chk intrinsics are avoided because they would force a much more complex implementation. They aren't designed to detect read overflows and offer no real benefit when using an implementation based on inline checks. Inline checks don't add up to much code size and allow full use of the regular string intrinsics while avoiding the need for a bunch of _chk functions and per-arch assembly to avoid wrapper overhead. This detects various overflows at compile-time in various drivers and some non-x86 core kernel code. There will likely be issues caught in regular use at runtime too. Future improvements left out of initial implementation for simplicity, as it's all quite optional and can be done incrementally: * Some of the fortified string functions (strncpy, strcat), don't yet place a limit on reads from the source based on __builtin_object_size of the source buffer. * Extending coverage to more string functions like strlcat. * It should be possible to optionally use __builtin_object_size(x, 1) for some functions (C strings) to detect intra-object overflows (like glibc's _FORTIFY_SOURCE=2), but for now this takes the conservative approach to avoid likely compatibility issues. * The compile-time checks should be made available via a separate config option which can be enabled by default (or always enabled) once enough time has passed to get the issues it catches fixed. Kees said: "This is great to have. While it was out-of-tree code, it would have blocked at least CVE-2016-3858 from being exploitable (improper size argument to strlcpy()). I've sent a number of fixes for out-of-bounds-reads that this detected upstream already" BUG=chromium:901062 TEST=Build and boot Change-Id: If67c761aa5941cd733f04af621d91677c363e3e1 [arnd@arndb.de: x86: fix fortified memcpy] Link: http://lkml.kernel.org/r/20170627150047.660360-1-arnd@arndb.de [keescook@chromium.org: avoid panic() in favor of BUG()] Link: http://lkml.kernel.org/r/20170626235122.GA25261@beast [keescook@chromium.org: move from -mm, add ARCH_HAS_FORTIFY_SOURCE, tweak Kconfig help] Link: http://lkml.kernel.org/r/20170526095404.20439-1-danielmicay@gmail.com Link: http://lkml.kernel.org/r/1497903987-21002-8-git-send-email-keescook@chromium.org Signed-off-by: Daniel Micay <danielmicay@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Arnd Bergmann <arnd@arndb.de> Acked-by: Kees Cook <keescook@chromium.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Daniel Axtens <dja@axtens.net> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Chris Metcalf <cmetcalf@ezchip.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Ingo Molnar <mingo@elte.hu> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> (cherry picked from commit 6974f0c4555e285ab217cee58b6e874f776ff409) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313425 [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/include/linux/string.h [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/x86/Kconfig [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/powerpc/Kconfig [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/x86/include/asm/string_64.h [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/security/Kconfig [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/x86/boot/compressed/misc.c [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/Kconfig [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/arm64/include/asm/string.h [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/x86/include/asm/string_32.h [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/x86/lib/memcpy_32.c [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/lib/string.c [modify] https://crrev.com/d65257b2c9e68a5b44590195f0bacd323a567786/arch/arm64/Kconfig
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e31696ae4e3e4b6467003f53ff069814031eeb09 commit e31696ae4e3e4b6467003f53ff069814031eeb09 Author: Arnd Bergmann <arnd@arndb.de> Date: Fri Nov 16 23:10:55 2018 UPSTREAM: string.h: workaround for increased stack usage The hardened strlen() function causes rather large stack usage in at least one file in the kernel, in particular when CONFIG_KASAN is enabled: drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init': drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=] Analyzing this problem led to the discovery that gcc fails to merge the stack slots for the i2c_board_info[] structures after we strlcpy() into them, due to the 'noreturn' attribute on the source string length check. I reported this as a gcc bug, but it is unlikely to get fixed for gcc-8, since it is relatively easy to work around, and it gets triggered rarely. An earlier workaround I did added an empty inline assembly statement before the call to fortify_panic(), which works surprisingly well, but is really ugly and unintuitive. This is a new approach to the same problem, this time addressing it by not calling the 'extern __real_strnlen()' function for string constants where __builtin_strlen() is a compile-time constant and therefore known to be safe. We do this by checking if the last character in the string is a compile-time constant '\0'. If it is, we can assume that strlen() of the string is also constant. As a side-effect, this should also improve the object code output for any other call of strlen() on a string constant. BUG=chromium:901062 TEST=Build and boot Change-Id: I7c511e25820ca98b29e4d8658c57df87abadf411 [akpm@linux-foundation.org: add comment] Link: http://lkml.kernel.org/r/20171205215143.3085755-1-arnd@arndb.de Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365 Link: https://patchwork.kernel.org/patch/9980413/ Link: https://patchwork.kernel.org/patch/9974047/ Fixes: 6974f0c4555 ("include/linux/string.h: add the option of fortified string.h functions") Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Kees Cook <keescook@chromium.org> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Alexander Potapenko <glider@google.com> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com> Cc: Daniel Micay <danielmicay@gmail.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Martin Wilck <mwilck@suse.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> (cherry picked from commit 146734b091430c80d80bb96b1139a96fb4bc830e) Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1313426 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/e31696ae4e3e4b6467003f53ff069814031eeb09/include/linux/string.h
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71 commit c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71 Author: Zubin Mithra <zsm@chromium.org> Date: Fri Dec 14 03:28:01 2018 CHROMIUM: Re-normalize configs In order to enable CONFIG_FORTIFY_SOURCE, renormalize configs. Run :- $ chromeos/scripts/kernelconfig olddefconfig Enables CONFIG_FORTIFY_SOURCE in :- - arm64/common.config - i386/common.config - x86_64/common.config - "# CONFIG_BT_HCIUART_3WIRE is not set" is added in arm64/common.config, armel/common.config - "# CONFIG_DRM_POWERVR_ROGUE_1_10 is not set" in removed in armel/common.config, i386/common.config and x86_64/common.config BUG=chromium:901062 TEST=None Change-Id: I7bb9ef839116639fc4997729db314199fcb9448c Signed-off-by: Zubin Mithra <zsm@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/1361603 Reviewed-by: Guenter Roeck <groeck@chromium.org> [modify] https://crrev.com/c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71/chromeos/config/x86_64/common.config [modify] https://crrev.com/c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71/chromeos/config/i386/common.config [modify] https://crrev.com/c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71/chromeos/config/armel/common.config [modify] https://crrev.com/c9ddbd0ed87dfb32c2a1f504ebcd0b5a73b6dd71/chromeos/config/arm64/common.config |
||||
►
Sign in to add a comment |
||||
Comment 1 by zsm@chromium.org
, Nov 1