New issue
Advanced search Search tips

Issue 901062 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

v4.4, and possibly older kernels: Enable CONFIG_FORTIFY_SOURCE

Project Member Reported by zsm@chromium.org, Nov 1

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.
 
Cc: groeck@chromium.org keescook@chromium.org
Correction in #1: CONFIG_FORTIFY is not set in v4.14, however the commit that introduces the CONFIG is present.
Do you mean FORTIFY_SOURCE, by any change ? I don't see CONFIG_FORTIFY in chromeos-4.14.


Summary: v4.4, and possibly older kernels: Enable CONFIG_FORTIFY_SOURCE (was: v4.4, and possibly older kernels: Enable CONFIG_FORTIFY)
Yes, thanks, I meant FORTIFY_SOURCE.
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 7

Labels: merge-merged-chromeos-4.4
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

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by bugdroid1@chromium.org, 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

Project Member

Comment 16 by bugdroid1@chromium.org, 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