* Re: [PATCH v3 bpf-next 00/21] bpf: Sysctl hook
From: Alexei Starovoitov @ 2019-04-06 17:02 UTC (permalink / raw)
To: Kees Cook
Cc: Andrey Ignatov, Network Development, Alexei Starovoitov,
Daniel Borkmann, Roman Gushchin, kernel-team, Luis Chamberlain,
Alexey Dobriyan, LKML, linux-fsdevel@vger.kernel.org,
linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jJGC1kB-0e3DimBSprcgEBnWRB=i287g-9T5a-RidFqBg@mail.gmail.com>
On Sat, Apr 06, 2019 at 09:43:50AM -0700, Kees Cook wrote:
> On Fri, Apr 5, 2019 at 12:36 PM Andrey Ignatov <rdna@fb.com> wrote:
> >
> > v2->v3:
> > - simplify C based selftests by relying on variable offset stack access.
> >
> > v1->v2:
> > - add fs/proc/proc_sysctl.c mainteners to Cc:.
> >
> > The patch set introduces new BPF hook for sysctl.
> >
> > It adds new program type BPF_PROG_TYPE_CGROUP_SYSCTL and attach type
> > BPF_CGROUP_SYSCTL.
> >
> > BPF_CGROUP_SYSCTL hook is placed before calling to sysctl's proc_handler so
> > that accesses (read/write) to sysctl can be controlled for specific cgroup
> > and either allowed or denied, or traced.
> >
> > The hook has access to sysctl name, current sysctl value and (on write
> > only) to new sysctl value via corresponding helpers. New sysctl value can
> > be overridden by program. Both name and values (current/new) are
> > represented as strings same way they're visible in /proc/sys/. It is up to
> > program to parse these strings.
> >
> > To help with parsing the most common kind of sysctl value, vector of
> > integers, two new helpers are provided: bpf_strtol and bpf_strtoul with
> > semantic similar to user space strtol(3) and strtoul(3).
> >
> > The hook also provides bpf_sysctl context with two fields:
> > * @write indicates whether sysctl is being read (= 0) or written (= 1);
> > * @file_pos is sysctl file position to read from or write to, can be
> > overridden.
> >
> > The hook allows to make better isolation for containerized applications
> > that are run as root so that one container can't change a sysctl and affect
> > all other containers on a host, make changes to allowed sysctl in a safer
> > way and simplify sysctl tracing for cgroups.
>
> This sounds more like an LSM than BPF.
not at all. the key difference is being cgroup scoped.
essentially for different containers.
> So sysctls can get blocked when
> new BPF is added to a cgroup?
bpf prog is attached to this hook in a particular cgroup
and executed for sysctls for tasks that belong to that cgroup.
> Can the BPF be removed (or rather,
> what's the lifetime of such BPF?)
same as all other cgroup-bpf hooks.
Do you have a specific concern or just asking how life time of programs
is managed?
High level description of lifetime is here:
https://facebookmicrosites.github.io/bpf/blog/2018/08/31/object-lifetime.html
^ permalink raw reply
* [REGRESSION] AppArmor module parameter layout changed with c5459b829b716
From: David Rheinsberg @ 2019-04-08 7:21 UTC (permalink / raw)
To: linux-kernel
Cc: John Johansen, James Morris, Serge E. Hallyn,
linux-security-module, Kees Cook, Casey Schaufler
Hi
A recent commit changed how `/sys/module/apparmor/parameters/enabled`
looks. It was "Y"/"N" before, now it is an integer. I *think* the
commit that changed this was:
commit c5459b829b716dafd226ad270f25c9a3050f7586
Author: Kees Cook <keescook@chromium.org>
Date: Thu Sep 13 22:28:48 2018 -0700
LSM: Plumb visibility into optional "enabled" state
I haven't recompiled with a revert, but changing the module-parameter
type looks like the obvious culprit. I don't see how this change can
be safe?
This breaks the AppArmor detection of `dbus-broker`. Can the commit be reverted?
Thanks
David
^ permalink raw reply
* Re: [REGRESSION] AppArmor module parameter layout changed with c5459b829b716
From: Kees Cook @ 2019-04-08 15:02 UTC (permalink / raw)
To: David Rheinsberg
Cc: LKML, John Johansen, James Morris, Serge E. Hallyn,
linux-security-module, Casey Schaufler
In-Reply-To: <CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com>
On Mon, Apr 8, 2019 at 12:21 AM David Rheinsberg
<david.rheinsberg@gmail.com> wrote:
>
> Hi
>
> A recent commit changed how `/sys/module/apparmor/parameters/enabled`
> looks. It was "Y"/"N" before, now it is an integer. I *think* the
> commit that changed this was:
Oooh... the _output_ appears differently based on the type. Argh. Let
me work something up...
> commit c5459b829b716dafd226ad270f25c9a3050f7586
> Author: Kees Cook <keescook@chromium.org>
> Date: Thu Sep 13 22:28:48 2018 -0700
>
> LSM: Plumb visibility into optional "enabled" state
>
> I haven't recompiled with a revert, but changing the module-parameter
> type looks like the obvious culprit. I don't see how this change can
> be safe?
>
> This breaks the AppArmor detection of `dbus-broker`. Can the commit be reverted?
Thanks for catching this!
--
Kees Cook
^ permalink raw reply
* [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
From: Kees Cook @ 2019-04-08 16:07 UTC (permalink / raw)
To: James Morris, John Johansen
Cc: David Rheinsberg, Serge E. Hallyn, linux-security-module,
linux-kernel
Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
since it was using the "bool" handler. After being changed to "int",
this switched to "1" or "0", breaking the userspace AppArmor detection
of dbus-broker. This restores the Y/N output while keeping the LSM
infrastructure happy.
Before:
$ cat /sys/module/apparmor/parameters/enabled
1
After:
$ cat /sys/module/apparmor/parameters/enabled
Y
Reported-by: David Rheinsberg <david.rheinsberg@gmail.com>
Link: https://lkml.kernel.org/r/CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com
Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This fix, if John is okay with it, is needed in v5.1 to correct the
userspace regression reported by David.
---
security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 49d664ddff44..87500bde5a92 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
bool aa_g_paranoid_load = true;
module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
+static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
+static int param_set_aaintbool(const char *val, const struct kernel_param *kp);
+#define param_check_aaintbool param_check_int
+static const struct kernel_param_ops param_ops_aaintbool = {
+ .set = param_set_aaintbool,
+ .get = param_get_aaintbool
+};
/* Boot time disable flag */
static int apparmor_enabled __lsm_ro_after_init = 1;
-module_param_named(enabled, apparmor_enabled, int, 0444);
+module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
static int __init apparmor_enabled_setup(char *str)
{
@@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct kernel_param *kp)
return param_get_uint(buffer, kp);
}
+/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
+static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
+{
+ struct kernel_param kp_local;
+ bool value;
+ int error;
+
+ if (apparmor_initialized)
+ return -EPERM;
+
+ /* Create local copy, with arg pointing to bool type. */
+ value = !!*((int *)kp->arg);
+ memcpy(&kp_local, kp, sizeof(kp_local));
+ kp_local.arg = &value;
+
+ error = param_set_bool(val, &kp_local);
+ if (!error)
+ *((int *)kp->arg) = *((bool *)kp_local.arg);
+ return error;
+}
+
+/*
+ * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
+ * 1/0, this converts the "int that is actually bool" back to bool for
+ * display in the /sys filesystem, while keeping it "int" for the LSM
+ * infrastructure.
+ */
+static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
+{
+ struct kernel_param kp_local;
+ bool value;
+
+ /* Create local copy, with arg pointing to bool type. */
+ value = !!*((int *)kp->arg);
+ memcpy(&kp_local, kp, sizeof(kp_local));
+ kp_local.arg = &value;
+
+ return param_get_bool(buffer, &kp_local);
+}
+
static int param_get_audit(char *buffer, const struct kernel_param *kp)
{
if (!apparmor_enabled)
--
2.17.1
--
Kees Cook
^ permalink raw reply related
* Re: [GIT PULL] tpmdd fixes for Linux v5.1
From: Dan Williams @ 2019-04-08 16:21 UTC (permalink / raw)
To: James Morris
Cc: Jarkko Sakkinen, Linux Kernel Mailing List, linux-security-module,
linux-integrity, linux-nvdimm
In-Reply-To: <alpine.LRH.2.21.1903300540530.2240@namei.org>
On Fri, Mar 29, 2019 at 11:42 AM James Morris <jmorris@namei.org> wrote:
>
> On Fri, 29 Mar 2019, Jarkko Sakkinen wrote:
>
> > Hi James,
> >
> > These are critical fixes for v5.1. Contains also couple of new selftests for
> > v5.1 features (partial reads in /dev/tpm0). I hope these could still reach
> > the release. Thanks.
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-tpm
Hi James,
Friendly ping on when might this go to Linus?
I was hoping that 3d0b1a381f6e ("KEYS: trusted: allow trusted.ko to
initialize w/o a TPM") would have hit -rc4. The NVDIMM subsystem has
been broken since -rc1.
^ permalink raw reply
* Re: [PATCH v2 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Laura Abbott @ 2019-04-08 16:43 UTC (permalink / raw)
To: Alexander Potapenko, yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
In-Reply-To: <20190308132701.133598-3-glider@google.com>
On 3/8/19 5:27 AM, Alexander Potapenko wrote:
> This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> without the need to pass any boot parameters.
>
> No performance optimizations are done at the moment to reduce double
> initialization of memory regions.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Sandeep Patil <sspatil@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> ---
> mm/page_poison.c | 5 +++++
> mm/slub.c | 2 ++
> security/Kconfig.initmem | 11 +++++++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 21d4f97cb49b..a1985f33f635 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
>
> static int __init early_page_poison_param(char *buf)
> {
> +#ifdef CONFIG_INIT_ALL_HEAP
> + want_page_poisoning = true;
> + return 0;
> +#else
> if (!buf)
> return -EINVAL;
> return strtobool(buf, &want_page_poisoning);
> +#endif
> }
> early_param("page_poison", early_page_poison_param);
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1b08fbcb7e61..00e0197d3f35 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
> if (*str == ',')
> slub_debug_slabs = str + 1;
> out:
> + if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
> + slub_debug |= SLAB_POISON;
> return 1;
> }
>
I've looked at doing something similar in the past (failing to find
the thread this morning...) and while this will work, it has pretty
serious performance issues. It's not actually the poisoning which
is expensive but that turning on debugging removes the cpu slab
which has significant performance penalties.
I'd rather go back to the proposal of just poisoning the slab
at alloc/free without using SLAB_POISON.
Thanks,
Laura
> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> index 27aec394365e..5ce49663777a 100644
> --- a/security/Kconfig.initmem
> +++ b/security/Kconfig.initmem
> @@ -13,6 +13,17 @@ config INIT_ALL_MEMORY
>
> if INIT_ALL_MEMORY
>
> +config INIT_ALL_HEAP
> + bool "Initialize all heap"
> + depends on INIT_ALL_MEMORY
> + select CONFIG_PAGE_POISONING
> + select CONFIG_PAGE_POISONING_NO_SANITY
> + select CONFIG_PAGE_POISONING_ZERO
> + select CONFIG_SLUB_DEBUG
> + default y
> + help
> + Enable page poisoning and slub poisoning by default.
> +
> config INIT_ALL_STACK
> bool "Initialize all stack"
> depends on INIT_ALL_MEMORY
>
^ permalink raw reply
* Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
From: John Johansen @ 2019-04-08 16:58 UTC (permalink / raw)
To: Kees Cook, James Morris
Cc: David Rheinsberg, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <20190408160706.GA18786@beast>
On 4/8/19 9:07 AM, Kees Cook wrote:
> Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
> state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
> since it was using the "bool" handler. After being changed to "int",
> this switched to "1" or "0", breaking the userspace AppArmor detection
> of dbus-broker. This restores the Y/N output while keeping the LSM
> infrastructure happy.
>
> Before:
> $ cat /sys/module/apparmor/parameters/enabled
> 1
>
> After:
> $ cat /sys/module/apparmor/parameters/enabled
> Y
>
> Reported-by: David Rheinsberg <david.rheinsberg@gmail.com>
> Link: https://lkml.kernel.org/r/CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This fix, if John is okay with it, is needed in v5.1 to correct the
> userspace regression reported by David.
> ---
> security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..87500bde5a92 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> bool aa_g_paranoid_load = true;
> module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
>
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp);
> +#define param_check_aaintbool param_check_int
> +static const struct kernel_param_ops param_ops_aaintbool = {
> + .set = param_set_aaintbool,
> + .get = param_get_aaintbool
> +};
> /* Boot time disable flag */
> static int apparmor_enabled __lsm_ro_after_init = 1;
> -module_param_named(enabled, apparmor_enabled, int, 0444);
> +module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>
> static int __init apparmor_enabled_setup(char *str)
> {
> @@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct kernel_param *kp)
> return param_get_uint(buffer, kp);
> }
>
> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> + int error;
> +
> + if (apparmor_initialized)
> + return -EPERM;
> +
This isn't sufficient/correct. apparmor_initialized is only set after
apparmor has gone through and completed initialization. However if
apparmor is not selected as one of the LSMs to enable, then this check
won't stop apparmor_enabled from being set post boot.
However with the apparmor_enabled param being 0444 and the
apparmor_enabled_setup() fn handling boot cmdline do with even need
the set parameter fn?
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(&kp_local, kp, sizeof(kp_local));
> + kp_local.arg = &value;
> +
> + error = param_set_bool(val, &kp_local);
> + if (!error)
> + *((int *)kp->arg) = *((bool *)kp_local.arg);
> + return error;
> +}
> +
> +/*
> + * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
> + * 1/0, this converts the "int that is actually bool" back to bool for
> + * display in the /sys filesystem, while keeping it "int" for the LSM
> + * infrastructure.
> + */
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> +
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(&kp_local, kp, sizeof(kp_local));
> + kp_local.arg = &value;
> +
> + return param_get_bool(buffer, &kp_local);
> +}
> +
> static int param_get_audit(char *buffer, const struct kernel_param *kp)
> {
> if (!apparmor_enabled)
>
^ permalink raw reply
* [PATCH v3 0/2] RFC: introduce CONFIG_INIT_ALL_MEMORY
From: Alexander Potapenko @ 2019-04-08 17:04 UTC (permalink / raw)
To: yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
This patch is a part of a bigger initiative to allow initializing
heap/stack memory in the Linux kernels by default.
The rationale behind doing so is to reduce the severity of bugs caused
by using uninitialized memory.
Over the last two years KMSAN (https://github.com/google/kmsan/) has
found more than a hundred bugs running in a really moderate setup (orders
of magnitude less CPU/months than KASAN). Some of those bugs led to
information leaks if uninitialized memory was copied to the userspace,
other could cause DoS because of subverted control flow.
A lot more bugs remain uncovered, so we want to provide the distros and OS
vendors with a last resort measure to mitigate such bugs.
Our plan is to introduce configuration flags to force initialization of
stack and heap variables with a fixed pattern.
This is going to render information leaks inefficient (as we'll only leak
pattern data) and make uses of uninitialized values in conditions more
deterministic and discoverable.
The stack instrumentation part is based on Clang's -ftrivial-auto-var-init
(see https://reviews.llvm.org/D54604 ; there's also a GCC feature request
for a similar flag: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87210)
or GCC's -fplugin-arg-structleak_plugin-byref-all
The heap initialization part is compiler-agnostic and is based on the
existing CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING.
Alexander Potapenko (2):
initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
initmem: introduce CONFIG_INIT_ALL_HEAP
Makefile | 3 ++-
mm/page_poison.c | 5 +++++
mm/slub.c | 2 ++
scripts/Makefile.initmem | 10 ++++++++++
security/Kconfig | 1 +
security/Kconfig.initmem | 40 ++++++++++++++++++++++++++++++++++++++++
6 files changed, 60 insertions(+), 1 deletion(-)
create mode 100644 scripts/Makefile.initmem
create mode 100644 security/Kconfig.initmem
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply
* [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-04-08 17:04 UTC (permalink / raw)
To: yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
In-Reply-To: <20190408170418.148554-1-glider@google.com>
CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
that force heap and stack initialization.
The rationale behind doing so is to reduce the severity of bugs caused
by using uninitialized memory.
CONFIG_INIT_ALL_STACK turns on stack initialization based on
-ftrivial-auto-var-init in Clang builds and on
-fplugin-arg-structleak_plugin-byref-all in GCC builds.
-ftrivial-auto-var-init is a Clang flag that provides trivial
initializers for uninitialized local variables, variable fields and
padding.
It has three possible values:
pattern - uninitialized locals are filled with a fixed pattern
(mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
for more details) likely to cause crashes when uninitialized value is
used;
zero (it's still debated whether this flag makes it to the official
Clang release) - uninitialized locals are filled with zeroes;
uninitialized (default) - uninitialized locals are left intact.
The proposed config builds the kernel with
-ftrivial-auto-var-init=pattern.
Developers have the possibility to opt-out of this feature on a
per-variable basis by using __attribute__((uninitialized)).
For GCC builds, CONFIG_INIT_ALL_STACK is simply wired up to
CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. No opt-out is possible at the
moment.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v2:
- addressed Kees Cook's comments: added GCC support
v3: addressed Masahiro Yamada's comments:
- dropped per-file opt-out mechanism
- fixed GCC_PLUGINS dependencies
---
Makefile | 3 ++-
scripts/Makefile.initmem | 10 ++++++++++
security/Kconfig | 1 +
security/Kconfig.initmem | 29 +++++++++++++++++++++++++++++
4 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 scripts/Makefile.initmem
create mode 100644 security/Kconfig.initmem
diff --git a/Makefile b/Makefile
index f070e0d65186..028ca37878fd 100644
--- a/Makefile
+++ b/Makefile
@@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
-export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
+export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
@@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
include scripts/Makefile.kasan
include scripts/Makefile.extrawarn
include scripts/Makefile.ubsan
+include scripts/Makefile.initmem
# Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
# last assignments
diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
new file mode 100644
index 000000000000..a6253d78fe35
--- /dev/null
+++ b/scripts/Makefile.initmem
@@ -0,0 +1,10 @@
+ifdef CONFIG_INIT_ALL_STACK
+
+# Clang's -ftrivial-auto-var-init=pattern flag initializes the
+# uninitialized parts of local variables (including fields and padding)
+# with a fixed pattern (0xAA in most cases).
+ifdef CONFIG_CC_HAS_AUTO_VAR_INIT
+ CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
+endif
+
+endif
diff --git a/security/Kconfig b/security/Kconfig
index e4fe2f3c2c65..cc12a39424dd 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
If you wish for all usermode helper programs to be disabled,
specify an empty string here (i.e. "").
+source "security/Kconfig.initmem"
source "security/selinux/Kconfig"
source "security/smack/Kconfig"
source "security/tomoyo/Kconfig"
diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
new file mode 100644
index 000000000000..5e49a55382ad
--- /dev/null
+++ b/security/Kconfig.initmem
@@ -0,0 +1,29 @@
+menu "Initialize all memory"
+
+config CC_HAS_AUTO_VAR_INIT
+ def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
+
+config INIT_ALL_MEMORY
+ bool "Initialize all memory"
+ default n
+ help
+ Enforce memory initialization to mitigate infoleaks and make
+ the control-flow bugs depending on uninitialized values more
+ deterministic.
+
+if INIT_ALL_MEMORY
+
+config INIT_ALL_STACK
+ bool "Initialize all stack"
+ depends on INIT_ALL_MEMORY
+ depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
+ select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
+ select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
+ select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
+ default y
+ help
+ Initialize uninitialized stack data with a fixed pattern
+ (0x00 in GCC, 0xAA in Clang).
+
+endif # INIT_ALL_MEMORY
+endmenu
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* [PATCH v3 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-04-08 17:04 UTC (permalink / raw)
To: yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
In-Reply-To: <20190408170418.148554-1-glider@google.com>
This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
without the need to pass any boot parameters.
No performance optimizations are done at the moment to reduce double
initialization of memory regions.
Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Sandeep Patil <sspatil@android.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: kernel-hardening@lists.openwall.com
---
v3:
- addressed comments by Masahiro Yamada (Kconfig fixes)
---
mm/page_poison.c | 5 +++++
mm/slub.c | 2 ++
security/Kconfig.initmem | 11 +++++++++++
3 files changed, 18 insertions(+)
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 21d4f97cb49b..a1985f33f635 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
static int __init early_page_poison_param(char *buf)
{
+#ifdef CONFIG_INIT_ALL_HEAP
+ want_page_poisoning = true;
+ return 0;
+#else
if (!buf)
return -EINVAL;
return strtobool(buf, &want_page_poisoning);
+#endif
}
early_param("page_poison", early_page_poison_param);
diff --git a/mm/slub.c b/mm/slub.c
index 1b08fbcb7e61..00e0197d3f35 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
if (*str == ',')
slub_debug_slabs = str + 1;
out:
+ if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
+ slub_debug |= SLAB_POISON;
return 1;
}
diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
index 5e49a55382ad..37cc10a2eeb5 100644
--- a/security/Kconfig.initmem
+++ b/security/Kconfig.initmem
@@ -13,6 +13,17 @@ config INIT_ALL_MEMORY
if INIT_ALL_MEMORY
+config INIT_ALL_HEAP
+ bool "Initialize all heap"
+ depends on INIT_ALL_MEMORY
+ select PAGE_POISONING
+ select PAGE_POISONING_NO_SANITY
+ select PAGE_POISONING_ZERO
+ select SLUB_DEBUG if SLUB
+ default y
+ help
+ Enable page poisoning and SLUB poisoning by default.
+
config INIT_ALL_STACK
bool "Initialize all stack"
depends on INIT_ALL_MEMORY
--
2.21.0.392.gf8f6787159e-goog
^ permalink raw reply related
* Re: [PATCH v2 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Kees Cook @ 2019-04-08 17:14 UTC (permalink / raw)
To: Laura Abbott
Cc: Alexander Potapenko, Masahiro Yamada, James Morris,
Serge E. Hallyn, linux-security-module, linux-kbuild,
Nick Desaulniers, Kostya Serebryany, Dmitry Vyukov, Sandeep Patil,
Kernel Hardening
In-Reply-To: <497b1201-b2ae-5e0c-d191-ff1830d92fc1@redhat.com>
On Mon, Apr 8, 2019 at 9:43 AM Laura Abbott <labbott@redhat.com> wrote:
> I've looked at doing something similar in the past (failing to find
> the thread this morning...) and while this will work, it has pretty
> serious performance issues. It's not actually the poisoning which
> is expensive but that turning on debugging removes the cpu slab
> which has significant performance penalties.
>
> I'd rather go back to the proposal of just poisoning the slab
> at alloc/free without using SLAB_POISON.
I still agree this would make the most sense. Fundamentally it's not a
debugging feature.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
From: Kees Cook @ 2019-04-08 17:25 UTC (permalink / raw)
To: John Johansen
Cc: James Morris, David Rheinsberg, Serge E. Hallyn,
linux-security-module, LKML
In-Reply-To: <73c80352-ded1-626b-0eb0-a9481165f25d@canonical.com>
On Mon, Apr 8, 2019 at 9:58 AM John Johansen
<john.johansen@canonical.com> wrote:
> > +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> > +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> > +{
> > + struct kernel_param kp_local;
> > + bool value;
> > + int error;
> > +
> > + if (apparmor_initialized)
> > + return -EPERM;
> > +
> This isn't sufficient/correct. apparmor_initialized is only set after
> apparmor has gone through and completed initialization. However if
> apparmor is not selected as one of the LSMs to enable, then this check
> won't stop apparmor_enabled from being set post boot.
>
> However with the apparmor_enabled param being 0444 and the
> apparmor_enabled_setup() fn handling boot cmdline do with even need
> the set parameter fn?
Yup, that's true. I've gone and tested this, and yes, the 0444 is
sufficient to protect the logic here (even if root chmods the inode).
So the test here is redundant. However, very early in the threads
about LSM boot cmdline enabling it was made clear that
"apparmor.enabled=..." needed to stay working, which means the "set"
op is still needed. (But I'm happy to do whatever you want here -- I
was just trying to keep the functionality as it was.)
Should I send a v2 without the "initialized" check or is this okay to
leave as-is with the redundant check?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v3 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Jann Horn @ 2019-04-08 17:39 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Kees Cook, sspatil,
Kernel Hardening, Laura Abbott
In-Reply-To: <20190408170418.148554-3-glider@google.com>
On Mon, Apr 8, 2019 at 7:20 PM Alexander Potapenko <glider@google.com> wrote:
> This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> without the need to pass any boot parameters.
>
> No performance optimizations are done at the moment to reduce double
> initialization of memory regions.
[...]
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 21d4f97cb49b..a1985f33f635 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
>
> static int __init early_page_poison_param(char *buf)
> {
> +#ifdef CONFIG_INIT_ALL_HEAP
> + want_page_poisoning = true;
> + return 0;
> +#else
> if (!buf)
> return -EINVAL;
> return strtobool(buf, &want_page_poisoning);
> +#endif
> }
> early_param("page_poison", early_page_poison_param);
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 1b08fbcb7e61..00e0197d3f35 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1287,6 +1287,8 @@ static int __init setup_slub_debug(char *str)
> if (*str == ',')
> slub_debug_slabs = str + 1;
> out:
> + if (IS_ENABLED(CONFIG_INIT_ALL_HEAP))
> + slub_debug |= SLAB_POISON;
> return 1;
> }
I don't understand how this is supposed to work. As far as I can tell,
the "slub_debug |= SLAB_POISON;" only happens if you actually pass in
a "slub_debug" boot parameter? Same thing for "want_page_poisoning =
true;".
Also, didn't Laura suggest in
https://www.openwall.com/lists/kernel-hardening/2019/04/08/4 that a
different approach might be more sensible to reduce the performance
hit?
^ permalink raw reply
* Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
From: John Johansen @ 2019-04-08 17:47 UTC (permalink / raw)
To: Kees Cook
Cc: James Morris, David Rheinsberg, Serge E. Hallyn,
linux-security-module, LKML
In-Reply-To: <CAGXu5j+k86VtegoRxV1s++YZdMxXEhQhqx1Yh7eu+YKHn0wNZA@mail.gmail.com>
On 4/8/19 10:25 AM, Kees Cook wrote:
> On Mon, Apr 8, 2019 at 9:58 AM John Johansen
> <john.johansen@canonical.com> wrote:
>>> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
>>> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
>>> +{
>>> + struct kernel_param kp_local;
>>> + bool value;
>>> + int error;
>>> +
>>> + if (apparmor_initialized)
>>> + return -EPERM;
>>> +
>> This isn't sufficient/correct. apparmor_initialized is only set after
>> apparmor has gone through and completed initialization. However if
>> apparmor is not selected as one of the LSMs to enable, then this check
>> won't stop apparmor_enabled from being set post boot.
>>
>> However with the apparmor_enabled param being 0444 and the
>> apparmor_enabled_setup() fn handling boot cmdline do with even need
>> the set parameter fn?
>
> Yup, that's true. I've gone and tested this, and yes, the 0444 is
> sufficient to protect the logic here (even if root chmods the inode).
> So the test here is redundant. However, very early in the threads
> about LSM boot cmdline enabling it was made clear that
> "apparmor.enabled=..." needed to stay working, which means the "set"
> op is still needed. (But I'm happy to do whatever you want here -- I
> was just trying to keep the functionality as it was.)
>
Right, though the legacy case that most document reference is apparmor=0/1
which is handled by __apparmor_enabled_setup()
still best not to break apparmor.enabled
> Should I send a v2 without the "initialized" check or is this okay to
> leave as-is with the redundant check?
>
redundant is fine, it will help protect against shooting ourselves in
the foot if some one ever tries changing the 0444
you can have my Acked-by: John Johansen <john.johansen@canonical.com>
^ permalink raw reply
* Re: [GIT PULL] tpmdd fixes for Linux v5.1
From: James Morris @ 2019-04-08 22:13 UTC (permalink / raw)
To: Dan Williams
Cc: Jarkko Sakkinen, Linux Kernel Mailing List, linux-security-module,
linux-integrity, linux-nvdimm
In-Reply-To: <CAA9_cmdyERPGwz-S3XuyJXnYrHM5V1fb_BCf_NS_kgQdN-YS=A@mail.gmail.com>
On Mon, 8 Apr 2019, Dan Williams wrote:
> On Fri, Mar 29, 2019 at 11:42 AM James Morris <jmorris@namei.org> wrote:
> >
> > On Fri, 29 Mar 2019, Jarkko Sakkinen wrote:
> >
> > > Hi James,
> > >
> > > These are critical fixes for v5.1. Contains also couple of new selftests for
> > > v5.1 features (partial reads in /dev/tpm0). I hope these could still reach
> > > the release. Thanks.
> >
> > Applied to
> > git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-tpm
>
> Hi James,
>
> Friendly ping on when might this go to Linus?
>
> I was hoping that 3d0b1a381f6e ("KEYS: trusted: allow trusted.ko to
> initialize w/o a TPM") would have hit -rc4. The NVDIMM subsystem has
> been broken since -rc1.
Oops, I queued these for the wrong tree, will send to Linus today.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Randy Dunlap @ 2019-04-08 22:15 UTC (permalink / raw)
To: Alexander Potapenko, yamada.masahiro, jmorris, serge
Cc: linux-security-module, linux-kbuild, ndesaulniers, kcc, dvyukov,
keescook, sspatil, kernel-hardening
In-Reply-To: <20190408170418.148554-2-glider@google.com>
On 4/8/19 10:04 AM, Alexander Potapenko wrote:
> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> new file mode 100644
> index 000000000000..5e49a55382ad
> --- /dev/null
> +++ b/security/Kconfig.initmem
> @@ -0,0 +1,29 @@
> +menu "Initialize all memory"
> +
> +config CC_HAS_AUTO_VAR_INIT
> + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> +
> +config INIT_ALL_MEMORY
> + bool "Initialize all memory"
> + default n
> + help
> + Enforce memory initialization to mitigate infoleaks and make
> + the control-flow bugs depending on uninitialized values more
> + deterministic.
> +
> +if INIT_ALL_MEMORY
> +
> +config INIT_ALL_STACK
> + bool "Initialize all stack"
> + depends on INIT_ALL_MEMORY
This "depends on" is redundant with the "if INIT_ALL_MEMORY" above it.
Similar comment for patch 2/2 and INIT_ALL_HEAP.
> + depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
> + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> + default y
> + help
> + Initialize uninitialized stack data with a fixed pattern
> + (0x00 in GCC, 0xAA in Clang).
> +
> +endif # INIT_ALL_MEMORY
> +endmenu
--
~Randy
^ permalink raw reply
* [GIT PULL] TPM fixes for v5.1
From: James Morris @ 2019-04-08 23:07 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-security-module, Jarkko Sakkinen
Please pull these TPM fixes, from Jarkko:
"These are critical fixes for v5.1. Contains also couple of new selftests
for v5.1 features (partial reads in /dev/tpm0)."
---
The following changes since commit fd008d1a7a204695f0e5e003af16448bb9c34b7b:
Merge branch 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6 (2019-04-07 19:51:09 -1000)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git fixes-v5.1
for you to fetch changes up to 6da70580af9612accf042b37564d73e787af39b4:
selftests/tpm2: Open tpm dev in unbuffered mode (2019-04-08 15:58:55 -0700)
----------------------------------------------------------------
Jarkko Sakkinen (2):
tpm: turn on TPM on suspend for TPM 1.x
KEYS: trusted: allow trusted.ko to initialize w/o a TPM
Tadeusz Struk (3):
tpm: fix an invalid condition in tpm_common_poll
selftests/tpm2: Extend tests to cover partial reads
selftests/tpm2: Open tpm dev in unbuffered mode
Yue Haibing (1):
tpm: Fix the type of the return value in calc_tpm2_event_size()
ndesaulniers@google.com (1):
KEYS: trusted: fix -Wvarags warning
drivers/char/tpm/eventlog/tpm2.c | 4 +-
drivers/char/tpm/tpm-dev-common.c | 9 ++++-
drivers/char/tpm/tpm-interface.c | 14 +++----
include/keys/trusted.h | 2 +-
security/keys/trusted.c | 32 +++++++++++----
tools/testing/selftests/tpm2/tpm2.py | 5 ++-
tools/testing/selftests/tpm2/tpm2_tests.py | 63 ++++++++++++++++++++++++++++++
7 files changed, 108 insertions(+), 21 deletions(-)
^ permalink raw reply
* Re: [GIT PULL] TPM fixes for v5.1
From: pr-tracker-bot @ 2019-04-09 3:15 UTC (permalink / raw)
To: James Morris
Cc: Linus Torvalds, linux-kernel, linux-security-module,
Jarkko Sakkinen
In-Reply-To: <alpine.LRH.2.21.1904090905570.20894@namei.org>
The pull request you sent on Tue, 9 Apr 2019 09:07:10 +1000 (AEST):
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git fixes-v5.1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a556810d8e06aa2da8bbe22da3d105eb5a0d0c7d
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"
From: David Rheinsberg @ 2019-04-09 6:21 UTC (permalink / raw)
To: Kees Cook
Cc: James Morris, John Johansen, Serge E. Hallyn,
linux-security-module, linux-kernel
In-Reply-To: <20190408160706.GA18786@beast>
Hi
On Mon, Apr 8, 2019 at 6:07 PM Kees Cook <keescook@chromium.org> wrote:
>
> Before commit c5459b829b71 ("LSM: Plumb visibility into optional "enabled"
> state"), /sys/module/apparmor/parameters/enabled would show "Y" or "N"
> since it was using the "bool" handler. After being changed to "int",
> this switched to "1" or "0", breaking the userspace AppArmor detection
> of dbus-broker. This restores the Y/N output while keeping the LSM
> infrastructure happy.
>
> Before:
> $ cat /sys/module/apparmor/parameters/enabled
> 1
>
> After:
> $ cat /sys/module/apparmor/parameters/enabled
> Y
>
> Reported-by: David Rheinsberg <david.rheinsberg@gmail.com>
> Link: https://lkml.kernel.org/r/CADyDSO6k8vYb1eryT4g6+EHrLCvb68GAbHVWuULkYjcZcYNhhw@mail.gmail.com
> Fixes: c5459b829b71 ("LSM: Plumb visibility into optional "enabled" state")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> This fix, if John is okay with it, is needed in v5.1 to correct the
> userspace regression reported by David.
> ---
> security/apparmor/lsm.c | 49 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
This looks good to me. Thanks a lot! If this makes v5.1, I will leave
the apparmor-detection in dbus-broker as it is, unless someone asks me
to parse 0/1 as well?
I cannot judge whether the apparmor_initialized check is correct, but
for the parameter parsing:
Reviewed-by: David Rheinsberg <david.rheinsberg@gmail.com>
Thanks
David
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..87500bde5a92 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1336,9 +1336,16 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> bool aa_g_paranoid_load = true;
> module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
>
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp);
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp);
> +#define param_check_aaintbool param_check_int
> +static const struct kernel_param_ops param_ops_aaintbool = {
> + .set = param_set_aaintbool,
> + .get = param_get_aaintbool
> +};
> /* Boot time disable flag */
> static int apparmor_enabled __lsm_ro_after_init = 1;
> -module_param_named(enabled, apparmor_enabled, int, 0444);
> +module_param_named(enabled, apparmor_enabled, aaintbool, 0444);
>
> static int __init apparmor_enabled_setup(char *str)
> {
> @@ -1413,6 +1420,46 @@ static int param_get_aauint(char *buffer, const struct kernel_param *kp)
> return param_get_uint(buffer, kp);
> }
>
> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */
> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> + int error;
> +
> + if (apparmor_initialized)
> + return -EPERM;
> +
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(&kp_local, kp, sizeof(kp_local));
> + kp_local.arg = &value;
> +
> + error = param_set_bool(val, &kp_local);
> + if (!error)
> + *((int *)kp->arg) = *((bool *)kp_local.arg);
> + return error;
> +}
> +
> +/*
> + * To avoid changing /sys/module/apparmor/parameters/enabled from Y/N to
> + * 1/0, this converts the "int that is actually bool" back to bool for
> + * display in the /sys filesystem, while keeping it "int" for the LSM
> + * infrastructure.
> + */
> +static int param_get_aaintbool(char *buffer, const struct kernel_param *kp)
> +{
> + struct kernel_param kp_local;
> + bool value;
> +
> + /* Create local copy, with arg pointing to bool type. */
> + value = !!*((int *)kp->arg);
> + memcpy(&kp_local, kp, sizeof(kp_local));
> + kp_local.arg = &value;
> +
> + return param_get_bool(buffer, &kp_local);
> +}
> +
> static int param_get_audit(char *buffer, const struct kernel_param *kp)
> {
> if (!apparmor_enabled)
> --
> 2.17.1
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-04-09 8:29 UTC (permalink / raw)
To: Randy Dunlap
Cc: Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, Linux Kbuild mailing list,
Nick Desaulniers, Kostya Serebryany, Dmitriy Vyukov, Kees Cook,
Sandeep Patil, Kernel Hardening
In-Reply-To: <adb23353-2e02-ecd2-7506-50f4db814b25@infradead.org>
On Tue, Apr 9, 2019 at 12:15 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 4/8/19 10:04 AM, Alexander Potapenko wrote:
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > new file mode 100644
> > index 000000000000..5e49a55382ad
> > --- /dev/null
> > +++ b/security/Kconfig.initmem
> > @@ -0,0 +1,29 @@
> > +menu "Initialize all memory"
> > +
> > +config CC_HAS_AUTO_VAR_INIT
> > + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> > +
> > +config INIT_ALL_MEMORY
> > + bool "Initialize all memory"
> > + default n
> > + help
> > + Enforce memory initialization to mitigate infoleaks and make
> > + the control-flow bugs depending on uninitialized values more
> > + deterministic.
> > +
> > +if INIT_ALL_MEMORY
> > +
> > +config INIT_ALL_STACK
> > + bool "Initialize all stack"
> > + depends on INIT_ALL_MEMORY
>
> This "depends on" is redundant with the "if INIT_ALL_MEMORY" above it.
Ack, thank you!
> Similar comment for patch 2/2 and INIT_ALL_HEAP.
>
> > + depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
> > + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> > + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> > + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> > + default y
> > + help
> > + Initialize uninitialized stack data with a fixed pattern
> > + (0x00 in GCC, 0xAA in Clang).
> > +
> > +endif # INIT_ALL_MEMORY
> > +endmenu
>
>
> --
> ~Randy
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Masahiro Yamada @ 2019-04-09 8:37 UTC (permalink / raw)
To: Alexander Potapenko
Cc: James Morris, Serge E. Hallyn, linux-security-module,
Linux Kbuild mailing list, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Kees Cook, sspatil, Kernel Hardening
In-Reply-To: <20190408170418.148554-2-glider@google.com>
On Tue, Apr 9, 2019 at 2:04 AM Alexander Potapenko <glider@google.com> wrote:
>
> CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
> that force heap and stack initialization.
> The rationale behind doing so is to reduce the severity of bugs caused
> by using uninitialized memory.
>
> CONFIG_INIT_ALL_STACK turns on stack initialization based on
> -ftrivial-auto-var-init in Clang builds and on
> -fplugin-arg-structleak_plugin-byref-all in GCC builds.
>
> -ftrivial-auto-var-init is a Clang flag that provides trivial
> initializers for uninitialized local variables, variable fields and
> padding.
>
> It has three possible values:
> pattern - uninitialized locals are filled with a fixed pattern
> (mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
> for more details) likely to cause crashes when uninitialized value is
> used;
> zero (it's still debated whether this flag makes it to the official
> Clang release) - uninitialized locals are filled with zeroes;
> uninitialized (default) - uninitialized locals are left intact.
>
> The proposed config builds the kernel with
> -ftrivial-auto-var-init=pattern.
>
> Developers have the possibility to opt-out of this feature on a
> per-variable basis by using __attribute__((uninitialized)).
>
> For GCC builds, CONFIG_INIT_ALL_STACK is simply wired up to
> CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. No opt-out is possible at the
> moment.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Sandeep Patil <sspatil@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> ---
> v2:
> - addressed Kees Cook's comments: added GCC support
> v3: addressed Masahiro Yamada's comments:
> - dropped per-file opt-out mechanism
> - fixed GCC_PLUGINS dependencies
> ---
> Makefile | 3 ++-
> scripts/Makefile.initmem | 10 ++++++++++
> security/Kconfig | 1 +
> security/Kconfig.initmem | 29 +++++++++++++++++++++++++++++
> 4 files changed, 42 insertions(+), 1 deletion(-)
> create mode 100644 scripts/Makefile.initmem
> create mode 100644 security/Kconfig.initmem
>
> diff --git a/Makefile b/Makefile
> index f070e0d65186..028ca37878fd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
>
> export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
> -export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
> +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
> export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
> export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> @@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
> include scripts/Makefile.kasan
> include scripts/Makefile.extrawarn
> include scripts/Makefile.ubsan
> +include scripts/Makefile.initmem
>
> # Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
> # last assignments
> diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
> new file mode 100644
> index 000000000000..a6253d78fe35
> --- /dev/null
> +++ b/scripts/Makefile.initmem
> @@ -0,0 +1,10 @@
> +ifdef CONFIG_INIT_ALL_STACK
> +
> +# Clang's -ftrivial-auto-var-init=pattern flag initializes the
> +# uninitialized parts of local variables (including fields and padding)
> +# with a fixed pattern (0xAA in most cases).
> +ifdef CONFIG_CC_HAS_AUTO_VAR_INIT
> + CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
> +endif
> +
> +endif
Now that you dropped per-file control,
why is this necessary?
Also, CFLAGS_INITMEM is not wired up to anything,
so this patch has no effect.
I think you can add the following to top Makefile, and that's it.
ifdef CONFIG_AUTO_VAR_INIT
KBUILD_CFLAGS += -ftrivial-auto-var-init=pattern
endif
> diff --git a/security/Kconfig b/security/Kconfig
> index e4fe2f3c2c65..cc12a39424dd 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
> If you wish for all usermode helper programs to be disabled,
> specify an empty string here (i.e. "").
>
> +source "security/Kconfig.initmem"
> source "security/selinux/Kconfig"
> source "security/smack/Kconfig"
> source "security/tomoyo/Kconfig"
> diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> new file mode 100644
> index 000000000000..5e49a55382ad
> --- /dev/null
> +++ b/security/Kconfig.initmem
> @@ -0,0 +1,29 @@
> +menu "Initialize all memory"
> +
> +config CC_HAS_AUTO_VAR_INIT
> + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> +
> +config INIT_ALL_MEMORY
> + bool "Initialize all memory"
> + default n
> + help
> + Enforce memory initialization to mitigate infoleaks and make
> + the control-flow bugs depending on uninitialized values more
> + deterministic.
> +
> +if INIT_ALL_MEMORY
> +
> +config INIT_ALL_STACK
> + bool "Initialize all stack"
> + depends on INIT_ALL_MEMORY
> + depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
> + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> + default y
> + help
> + Initialize uninitialized stack data with a fixed pattern
> + (0x00 in GCC, 0xAA in Clang).
> +
I am not sure about the benefit of these umbrella CONFIGs
since these make 'depends on' and 'select' complicated.
I will leave it to Kees, though.
> +endif # INIT_ALL_MEMORY
> +endmenu
> --
> 2.21.0.392.gf8f6787159e-goog
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Alexander Potapenko @ 2019-04-09 8:55 UTC (permalink / raw)
To: Kees Cook
Cc: Laura Abbott, Masahiro Yamada, James Morris, Serge E. Hallyn,
linux-security-module, linux-kbuild, Nick Desaulniers,
Kostya Serebryany, Dmitry Vyukov, Sandeep Patil, Kernel Hardening
In-Reply-To: <CAGXu5jJUWKiyW-_ygHpg9rJc7wQ2XTzjH71CtJfdefLu2zZHEg@mail.gmail.com>
On Mon, Apr 8, 2019 at 7:14 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Apr 8, 2019 at 9:43 AM Laura Abbott <labbott@redhat.com> wrote:
> > I've looked at doing something similar in the past (failing to find
> > the thread this morning...) and while this will work, it has pretty
> > serious performance issues. It's not actually the poisoning which
> > is expensive but that turning on debugging removes the cpu slab
> > which has significant performance penalties.
> >
> > I'd rather go back to the proposal of just poisoning the slab
> > at alloc/free without using SLAB_POISON.
>
> I still agree this would make the most sense. Fundamentally it's not a
> debugging feature.
Wasn't it you who suggested using SLAB_POISON in the first round of comments? :)
I actually have a working implementation that piggybacks on existing
__GFP_ZERO code to initialize page_alloc() and SLUB allocations:
https://github.com/google/kmsan/commit/4907af529ad525378a0728435c96d3812f71e594
https://github.com/google/kmsan/commit/69618a9668bcf27700cc5da42eebf8ab50d1f56a
I'd better cook a patch based on that.
There's also some overhead when allocations are initialized twice (in
page_alloc() and kmalloc()) or thrice (page_alloc(), kmalloc() and
e.g. sock_alloc_send_pskb())
We can introduce another GFP flag explicitly telling the allocator to
not initialize the memory chunk if we know it'll be initialized by a
higher level allocator
(something along the lines of
https://github.com/google/kmsan/commit/4fc8cff79d868c29688c8a4186e504fda362f6fd)
> --
> Kees Cook
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-04-09 9:02 UTC (permalink / raw)
To: Masahiro Yamada
Cc: James Morris, Serge E. Hallyn, linux-security-module,
Linux Kbuild mailing list, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Kees Cook, Sandeep Patil, Kernel Hardening
In-Reply-To: <CAK7LNAS3+FKpgBvvxKDukD1xcHUZ1o3TsE1BjjVq5P13MrO7Fw@mail.gmail.com>
On Tue, Apr 9, 2019 at 10:38 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Tue, Apr 9, 2019 at 2:04 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
> > that force heap and stack initialization.
> > The rationale behind doing so is to reduce the severity of bugs caused
> > by using uninitialized memory.
> >
> > CONFIG_INIT_ALL_STACK turns on stack initialization based on
> > -ftrivial-auto-var-init in Clang builds and on
> > -fplugin-arg-structleak_plugin-byref-all in GCC builds.
> >
> > -ftrivial-auto-var-init is a Clang flag that provides trivial
> > initializers for uninitialized local variables, variable fields and
> > padding.
> >
> > It has three possible values:
> > pattern - uninitialized locals are filled with a fixed pattern
> > (mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
> > for more details) likely to cause crashes when uninitialized value is
> > used;
> > zero (it's still debated whether this flag makes it to the official
> > Clang release) - uninitialized locals are filled with zeroes;
> > uninitialized (default) - uninitialized locals are left intact.
> >
> > The proposed config builds the kernel with
> > -ftrivial-auto-var-init=pattern.
> >
> > Developers have the possibility to opt-out of this feature on a
> > per-variable basis by using __attribute__((uninitialized)).
> >
> > For GCC builds, CONFIG_INIT_ALL_STACK is simply wired up to
> > CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. No opt-out is possible at the
> > moment.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Kostya Serebryany <kcc@google.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Sandeep Patil <sspatil@android.com>
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kbuild@vger.kernel.org
> > Cc: kernel-hardening@lists.openwall.com
> > ---
> > v2:
> > - addressed Kees Cook's comments: added GCC support
> > v3: addressed Masahiro Yamada's comments:
> > - dropped per-file opt-out mechanism
> > - fixed GCC_PLUGINS dependencies
> > ---
> > Makefile | 3 ++-
> > scripts/Makefile.initmem | 10 ++++++++++
> > security/Kconfig | 1 +
> > security/Kconfig.initmem | 29 +++++++++++++++++++++++++++++
> > 4 files changed, 42 insertions(+), 1 deletion(-)
> > create mode 100644 scripts/Makefile.initmem
> > create mode 100644 security/Kconfig.initmem
> >
> > diff --git a/Makefile b/Makefile
> > index f070e0d65186..028ca37878fd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
> >
> > export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> > export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
> > -export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
> > +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
> > export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
> > export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> > export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> > @@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
> > include scripts/Makefile.kasan
> > include scripts/Makefile.extrawarn
> > include scripts/Makefile.ubsan
> > +include scripts/Makefile.initmem
> >
> > # Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
> > # last assignments
> > diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
> > new file mode 100644
> > index 000000000000..a6253d78fe35
> > --- /dev/null
> > +++ b/scripts/Makefile.initmem
> > @@ -0,0 +1,10 @@
> > +ifdef CONFIG_INIT_ALL_STACK
> > +
> > +# Clang's -ftrivial-auto-var-init=pattern flag initializes the
> > +# uninitialized parts of local variables (including fields and padding)
> > +# with a fixed pattern (0xAA in most cases).
> > +ifdef CONFIG_CC_HAS_AUTO_VAR_INIT
> > + CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
> > +endif
> > +
> > +endif
>
> Now that you dropped per-file control,
> why is this necessary?
> Also, CFLAGS_INITMEM is not wired up to anything,
> so this patch has no effect.
There's CFLAGS_INITMEM in the top Makefile, so I suppose it should be working.
But I agree there's no point in adding extra files for a single build flag.
>
> I think you can add the following to top Makefile, and that's it.
>
> ifdef CONFIG_AUTO_VAR_INIT
> KBUILD_CFLAGS += -ftrivial-auto-var-init=pattern
> endif
>
>
>
>
> > diff --git a/security/Kconfig b/security/Kconfig
> > index e4fe2f3c2c65..cc12a39424dd 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
> > If you wish for all usermode helper programs to be disabled,
> > specify an empty string here (i.e. "").
> >
> > +source "security/Kconfig.initmem"
> > source "security/selinux/Kconfig"
> > source "security/smack/Kconfig"
> > source "security/tomoyo/Kconfig"
> > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > new file mode 100644
> > index 000000000000..5e49a55382ad
> > --- /dev/null
> > +++ b/security/Kconfig.initmem
> > @@ -0,0 +1,29 @@
> > +menu "Initialize all memory"
> > +
> > +config CC_HAS_AUTO_VAR_INIT
> > + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> > +
> > +config INIT_ALL_MEMORY
> > + bool "Initialize all memory"
> > + default n
> > + help
> > + Enforce memory initialization to mitigate infoleaks and make
> > + the control-flow bugs depending on uninitialized values more
> > + deterministic.
> > +
> > +if INIT_ALL_MEMORY
> > +
> > +config INIT_ALL_STACK
> > + bool "Initialize all stack"
> > + depends on INIT_ALL_MEMORY
> > + depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
> > + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> > + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> > + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> > + default y
> > + help
> > + Initialize uninitialized stack data with a fixed pattern
> > + (0x00 in GCC, 0xAA in Clang).
> > +
>
>
> I am not sure about the benefit of these umbrella CONFIGs
> since these make 'depends on' and 'select' complicated.
> I will leave it to Kees, though.
>
>
>
> > +endif # INIT_ALL_MEMORY
> > +endmenu
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Best Regards
> Masahiro Yamada
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3 1/2] initmem: introduce CONFIG_INIT_ALL_MEMORY and CONFIG_INIT_ALL_STACK
From: Alexander Potapenko @ 2019-04-09 9:03 UTC (permalink / raw)
To: Masahiro Yamada
Cc: James Morris, Serge E. Hallyn, linux-security-module,
Linux Kbuild mailing list, Nick Desaulniers, Kostya Serebryany,
Dmitry Vyukov, Kees Cook, Sandeep Patil, Kernel Hardening
In-Reply-To: <CAG_fn=WbvdqsXOr-TKY4kiw2Fd7zfbd5McgC2DEFvJwM7CR3sQ@mail.gmail.com>
On Tue, Apr 9, 2019 at 11:02 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Tue, Apr 9, 2019 at 10:38 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Tue, Apr 9, 2019 at 2:04 AM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > CONFIG_INIT_ALL_MEMORY is going to be an umbrella config for options
> > > that force heap and stack initialization.
> > > The rationale behind doing so is to reduce the severity of bugs caused
> > > by using uninitialized memory.
> > >
> > > CONFIG_INIT_ALL_STACK turns on stack initialization based on
> > > -ftrivial-auto-var-init in Clang builds and on
> > > -fplugin-arg-structleak_plugin-byref-all in GCC builds.
> > >
> > > -ftrivial-auto-var-init is a Clang flag that provides trivial
> > > initializers for uninitialized local variables, variable fields and
> > > padding.
> > >
> > > It has three possible values:
> > > pattern - uninitialized locals are filled with a fixed pattern
> > > (mostly 0xAA on 64-bit platforms, see https://reviews.llvm.org/D54604
> > > for more details) likely to cause crashes when uninitialized value is
> > > used;
> > > zero (it's still debated whether this flag makes it to the official
> > > Clang release) - uninitialized locals are filled with zeroes;
> > > uninitialized (default) - uninitialized locals are left intact.
> > >
> > > The proposed config builds the kernel with
> > > -ftrivial-auto-var-init=pattern.
> > >
> > > Developers have the possibility to opt-out of this feature on a
> > > per-variable basis by using __attribute__((uninitialized)).
> > >
> > > For GCC builds, CONFIG_INIT_ALL_STACK is simply wired up to
> > > CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. No opt-out is possible at the
> > > moment.
> > >
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> > > Cc: James Morris <jmorris@namei.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Cc: Kostya Serebryany <kcc@google.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Sandeep Patil <sspatil@android.com>
> > > Cc: linux-security-module@vger.kernel.org
> > > Cc: linux-kbuild@vger.kernel.org
> > > Cc: kernel-hardening@lists.openwall.com
> > > ---
> > > v2:
> > > - addressed Kees Cook's comments: added GCC support
> > > v3: addressed Masahiro Yamada's comments:
> > > - dropped per-file opt-out mechanism
> > > - fixed GCC_PLUGINS dependencies
> > > ---
> > > Makefile | 3 ++-
> > > scripts/Makefile.initmem | 10 ++++++++++
> > > security/Kconfig | 1 +
> > > security/Kconfig.initmem | 29 +++++++++++++++++++++++++++++
> > > 4 files changed, 42 insertions(+), 1 deletion(-)
> > > create mode 100644 scripts/Makefile.initmem
> > > create mode 100644 security/Kconfig.initmem
> > >
> > > diff --git a/Makefile b/Makefile
> > > index f070e0d65186..028ca37878fd 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -448,7 +448,7 @@ export HOSTCXX KBUILD_HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
> > >
> > > export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> > > export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
> > > -export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN
> > > +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN CFLAGS_INITMEM
> > > export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
> > > export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE
> > > export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL
> > > @@ -840,6 +840,7 @@ KBUILD_ARFLAGS := $(call ar-option,D)
> > > include scripts/Makefile.kasan
> > > include scripts/Makefile.extrawarn
> > > include scripts/Makefile.ubsan
> > > +include scripts/Makefile.initmem
> > >
> > > # Add any arch overrides and user supplied CPPFLAGS, AFLAGS and CFLAGS as the
> > > # last assignments
> > > diff --git a/scripts/Makefile.initmem b/scripts/Makefile.initmem
> > > new file mode 100644
> > > index 000000000000..a6253d78fe35
> > > --- /dev/null
> > > +++ b/scripts/Makefile.initmem
> > > @@ -0,0 +1,10 @@
> > > +ifdef CONFIG_INIT_ALL_STACK
> > > +
> > > +# Clang's -ftrivial-auto-var-init=pattern flag initializes the
> > > +# uninitialized parts of local variables (including fields and padding)
> > > +# with a fixed pattern (0xAA in most cases).
> > > +ifdef CONFIG_CC_HAS_AUTO_VAR_INIT
> > > + CFLAGS_INITMEM := -ftrivial-auto-var-init=pattern
> > > +endif
> > > +
> > > +endif
> >
> > Now that you dropped per-file control,
> > why is this necessary?
> > Also, CFLAGS_INITMEM is not wired up to anything,
> > so this patch has no effect.
> There's CFLAGS_INITMEM in the top Makefile, so I suppose it should be working.
Ah, now I see. Yes, I was too hasty with this patchset.
> But I agree there's no point in adding extra files for a single build flag.
> >
> > I think you can add the following to top Makefile, and that's it.
> >
> > ifdef CONFIG_AUTO_VAR_INIT
> > KBUILD_CFLAGS += -ftrivial-auto-var-init=pattern
> > endif
> >
> >
> >
> >
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index e4fe2f3c2c65..cc12a39424dd 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -230,6 +230,7 @@ config STATIC_USERMODEHELPER_PATH
> > > If you wish for all usermode helper programs to be disabled,
> > > specify an empty string here (i.e. "").
> > >
> > > +source "security/Kconfig.initmem"
> > > source "security/selinux/Kconfig"
> > > source "security/smack/Kconfig"
> > > source "security/tomoyo/Kconfig"
> > > diff --git a/security/Kconfig.initmem b/security/Kconfig.initmem
> > > new file mode 100644
> > > index 000000000000..5e49a55382ad
> > > --- /dev/null
> > > +++ b/security/Kconfig.initmem
> > > @@ -0,0 +1,29 @@
> > > +menu "Initialize all memory"
> > > +
> > > +config CC_HAS_AUTO_VAR_INIT
> > > + def_bool $(cc-option,-ftrivial-auto-var-init=pattern)
> > > +
> > > +config INIT_ALL_MEMORY
> > > + bool "Initialize all memory"
> > > + default n
> > > + help
> > > + Enforce memory initialization to mitigate infoleaks and make
> > > + the control-flow bugs depending on uninitialized values more
> > > + deterministic.
> > > +
> > > +if INIT_ALL_MEMORY
> > > +
> > > +config INIT_ALL_STACK
> > > + bool "Initialize all stack"
> > > + depends on INIT_ALL_MEMORY
> > > + depends on CC_HAS_AUTO_VAR_INIT || (HAVE_GCC_PLUGINS && PLUGIN_HOSTCC != "")
> > > + select GCC_PLUGINS if !CC_HAS_AUTO_VAR_INIT
> > > + select GCC_PLUGIN_STRUCTLEAK if !CC_HAS_AUTO_VAR_INIT
> > > + select GCC_PLUGIN_STRUCTLEAK_BYREF_ALL if !CC_HAS_AUTO_VAR_INIT
> > > + default y
> > > + help
> > > + Initialize uninitialized stack data with a fixed pattern
> > > + (0x00 in GCC, 0xAA in Clang).
> > > +
> >
> >
> > I am not sure about the benefit of these umbrella CONFIGs
> > since these make 'depends on' and 'select' complicated.
> > I will leave it to Kees, though.
> >
> >
> >
> > > +endif # INIT_ALL_MEMORY
> > > +endmenu
> > > --
> > > 2.21.0.392.gf8f6787159e-goog
> > >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* Re: [PATCH v3 2/2] initmem: introduce CONFIG_INIT_ALL_HEAP
From: Mark Rutland @ 2019-04-09 9:32 UTC (permalink / raw)
To: Alexander Potapenko
Cc: yamada.masahiro, jmorris, serge, linux-security-module,
linux-kbuild, ndesaulniers, kcc, dvyukov, keescook, sspatil,
kernel-hardening
In-Reply-To: <20190408170418.148554-3-glider@google.com>
On Mon, Apr 08, 2019 at 07:04:18PM +0200, Alexander Potapenko wrote:
> This config option enables CONFIG_SLUB_DEBUG and CONFIG_PAGE_POISONING
> without the need to pass any boot parameters.
>
> No performance optimizations are done at the moment to reduce double
> initialization of memory regions.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kostya Serebryany <kcc@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Sandeep Patil <sspatil@android.com>
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: kernel-hardening@lists.openwall.com
> ---
> v3:
> - addressed comments by Masahiro Yamada (Kconfig fixes)
> ---
> mm/page_poison.c | 5 +++++
> mm/slub.c | 2 ++
> security/Kconfig.initmem | 11 +++++++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/mm/page_poison.c b/mm/page_poison.c
> index 21d4f97cb49b..a1985f33f635 100644
> --- a/mm/page_poison.c
> +++ b/mm/page_poison.c
> @@ -12,9 +12,14 @@ static bool want_page_poisoning __read_mostly;
>
> static int __init early_page_poison_param(char *buf)
> {
> +#ifdef CONFIG_INIT_ALL_HEAP
> + want_page_poisoning = true;
> + return 0;
> +#else
> if (!buf)
> return -EINVAL;
> return strtobool(buf, &want_page_poisoning);
> +#endif
> }
> early_param("page_poison", early_page_poison_param);
IIUC this is only called if page_poison is passed on the command line,
so want_page_poisoning won't be set automatically unless that's passed.
Presumably you want to initialize it at definition with:
static bool want_page_poisoning __read_mostly = IS_ENABLED(CONFIG_INIT_ALL_HEAP);
with:
#ifndef CONFIG_INIT_ALL_HEAP
static int __init early_page_poison_param(char *buf)
{
...
}
early_param("page_poison", early_page_poison_param);
#endif
... so that it can't be disabled?
Thanks,
Mark.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox