linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gcc-plugins: Add structleak for more stack initialization
@ 2017-01-13 22:02 Kees Cook
  2017-01-14 10:03 ` PaX Team
  2017-01-16 11:54 ` Mark Rutland
  0 siblings, 2 replies; 17+ messages in thread
From: Kees Cook @ 2017-01-13 22:02 UTC (permalink / raw)
  To: kernel-hardening
  Cc: PaX Team, Emese Revfy, AKASHI, Takahiro, Mark Rutland,
	park jinbum, Daniel Micay, linux-kernel

This plugin detects any structures that contain __user attributes and
makes sure it is being fulling initialized so that a specific class of
information exposure is eliminated. (For example, the exposure of siginfo
in CVE-2013-2141 would have been blocked by this plugin.)

Ported from grsecurity/PaX. This version adds a verbose option to the
plugin and the Kconfig.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/Kconfig                            |  22 +++
 include/linux/compiler.h                |   6 +-
 scripts/Makefile.gcc-plugins            |   4 +
 scripts/gcc-plugins/structleak_plugin.c | 246 ++++++++++++++++++++++++++++++++
 4 files changed, 277 insertions(+), 1 deletion(-)
 create mode 100644 scripts/gcc-plugins/structleak_plugin.c

diff --git a/arch/Kconfig b/arch/Kconfig
index 99839c23d453..f1250ba0b06f 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -410,6 +410,28 @@ config GCC_PLUGIN_LATENT_ENTROPY
 	   * https://grsecurity.net/
 	   * https://pax.grsecurity.net/
 
+config GCC_PLUGIN_STRUCTLEAK
+	bool "Force initialization of variables containing userspace addresses"
+	depends on GCC_PLUGINS
+	help
+	  This plugin zero-initializes any structures that containing a
+	  __user attribute. This can prevent some classes of information
+	  exposures.
+
+	  This plugin was ported from grsecurity/PaX. More information at:
+	   * https://grsecurity.net/
+	   * https://pax.grsecurity.net/
+
+config GCC_PLUGIN_STRUCTLEAK_VERBOSE
+	bool "Report initialized variables"
+	depends on GCC_PLUGIN_STRUCTLEAK
+	depends on !COMPILE_TEST
+	help
+	  This option will cause a warning to be printed each time the
+	  structleak plugin finds a variable it thinks needs to be
+	  initialized. Since not all existing initializers are detected
+	  by the plugin, this can produce false positive warnings.
+
 config HAVE_CC_STACKPROTECTOR
 	bool
 	help
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index cf0fa5d86059..91c30cba984e 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -27,7 +27,11 @@ extern void __chk_user_ptr(const volatile void __user *);
 extern void __chk_io_ptr(const volatile void __iomem *);
 # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
 #else /* __CHECKER__ */
-# define __user
+# ifdef STRUCTLEAK_PLUGIN
+#  define __user __attribute__((user))
+# else
+#  define __user
+# endif
 # define __kernel
 # define __safe
 # define __force
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index 060d2cb373db..a084f7a511d8 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -25,6 +25,10 @@ ifdef CONFIG_GCC_PLUGINS
     endif
   endif
 
+  gcc-plugin-$(CONFIG_GCC_PLUGIN_STRUCTLEAK)	+= structleak_plugin.so
+  gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE)	+= -fplugin-arg-structleak_plugin-verbose
+  gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK)	+= -DSTRUCTLEAK_PLUGIN
+
   GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
   export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
diff --git a/scripts/gcc-plugins/structleak_plugin.c b/scripts/gcc-plugins/structleak_plugin.c
new file mode 100644
index 000000000000..deddb7291a26
--- /dev/null
+++ b/scripts/gcc-plugins/structleak_plugin.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright 2013-2017 by PaX Team <pageexec@freemail.hu>
+ * Licensed under the GPL v2
+ *
+ * Note: the choice of the license means that the compilation process is
+ *       NOT 'eligible' as defined by gcc's library exception to the GPL v3,
+ *       but for the kernel it doesn't matter since it doesn't link against
+ *       any of the gcc libraries
+ *
+ * gcc plugin to forcibly initialize certain local variables that could
+ * otherwise leak kernel stack to userland if they aren't properly initialized
+ * by later code
+ *
+ * Homepage: http://pax.grsecurity.net/
+ *
+ * Options:
+ * -fplugin-arg-structleak_plugin-disable
+ * -fplugin-arg-structleak_plugin-verbose
+ *
+ * Usage:
+ * $ # for 4.5/4.6/C based 4.7
+ * $ gcc -I`gcc -print-file-name=plugin`/include -I`gcc -print-file-name=plugin`/include/c-family -fPIC -shared -O2 -o structleak_plugin.so structleak_plugin.c
+ * $ # for C++ based 4.7/4.8+
+ * $ g++ -I`g++ -print-file-name=plugin`/include -I`g++ -print-file-name=plugin`/include/c-family -fPIC -shared -O2 -o structleak_plugin.so structleak_plugin.c
+ * $ gcc -fplugin=./structleak_plugin.so test.c -O2
+ *
+ * TODO: eliminate redundant initializers
+ *       increase type coverage
+ */
+
+#include "gcc-common.h"
+
+/* unused C type flag in all versions 4.5-6 */
+#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
+
+__visible int plugin_is_GPL_compatible;
+
+static struct plugin_info structleak_plugin_info = {
+	.version	= "201607271510vanilla",
+	.help		= "disable\tdo not activate plugin\n"
+			   "verbose\tprint all initialized variables\n",
+};
+
+static bool verbose;
+
+static tree handle_user_attribute(tree *node, tree name, tree args, int flags, bool *no_add_attrs)
+{
+	*no_add_attrs = true;
+
+	/* check for types? for now accept everything linux has to offer */
+	if (TREE_CODE(*node) != FIELD_DECL)
+		return NULL_TREE;
+
+	*no_add_attrs = false;
+	return NULL_TREE;
+}
+
+static struct attribute_spec user_attr = {
+	.name			= "user",
+	.min_length		= 0,
+	.max_length		= 0,
+	.decl_required		= false,
+	.type_required		= false,
+	.function_type_required	= false,
+	.handler		= handle_user_attribute,
+#if BUILDING_GCC_VERSION >= 4007
+	.affects_type_identity	= true
+#endif
+};
+
+static void register_attributes(void *event_data, void *data)
+{
+	register_attribute(&user_attr);
+}
+
+static tree get_field_type(tree field)
+{
+	return strip_array_types(TREE_TYPE(field));
+}
+
+static bool is_userspace_type(tree type)
+{
+	tree field;
+
+	for (field = TYPE_FIELDS(type); field; field = TREE_CHAIN(field)) {
+		tree fieldtype = get_field_type(field);
+		enum tree_code code = TREE_CODE(fieldtype);
+
+		if (code == RECORD_TYPE || code == UNION_TYPE)
+			if (is_userspace_type(fieldtype))
+				return true;
+
+		if (lookup_attribute("user", DECL_ATTRIBUTES(field)))
+			return true;
+	}
+	return false;
+}
+
+static void finish_type(void *event_data, void *data)
+{
+	tree type = (tree)event_data;
+
+	if (type == NULL_TREE || type == error_mark_node)
+		return;
+
+#if BUILDING_GCC_VERSION >= 5000
+	if (TREE_CODE(type) == ENUMERAL_TYPE)
+		return;
+#endif
+
+	if (TYPE_USERSPACE(type))
+		return;
+
+	if (is_userspace_type(type))
+		TYPE_USERSPACE(type) = 1;
+}
+
+static void initialize(tree var)
+{
+	basic_block bb;
+	gimple_stmt_iterator gsi;
+	tree initializer;
+	gimple init_stmt;
+
+	/* this is the original entry bb before the forced split */
+	bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
+
+	/* first check if variable is already initialized, warn otherwise */
+	for (gsi = gsi_start_bb(bb); !gsi_end_p(gsi); gsi_next(&gsi)) {
+		gimple stmt = gsi_stmt(gsi);
+		tree rhs1;
+
+		/* we're looking for an assignment of a single rhs... */
+		if (!gimple_assign_single_p(stmt))
+			continue;
+		rhs1 = gimple_assign_rhs1(stmt);
+#if BUILDING_GCC_VERSION >= 4007
+		/* ... of a non-clobbering expression... */
+		if (TREE_CLOBBER_P(rhs1))
+			continue;
+#endif
+		/* ... to our variable... */
+		if (gimple_get_lhs(stmt) != var)
+			continue;
+		/* if it's an initializer then we're good */
+		if (TREE_CODE(rhs1) == CONSTRUCTOR)
+			return;
+	}
+
+	/* these aren't the 0days you're looking for */
+	if (verbose)
+		inform(DECL_SOURCE_LOCATION(var),
+			"userspace variable will be forcibly initialized");
+
+	/* build the initializer expression */
+	initializer = build_constructor(TREE_TYPE(var), NULL);
+
+	/* build the initializer stmt */
+	init_stmt = gimple_build_assign(var, initializer);
+	gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+	gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
+	update_stmt(init_stmt);
+}
+
+static unsigned int structleak_execute(void)
+{
+	basic_block bb;
+	unsigned int ret = 0;
+	tree var;
+	unsigned int i;
+
+	/* split the first bb where we can put the forced initializers */
+	gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+	bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
+	if (!single_pred_p(bb)) {
+		split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+		gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
+	}
+
+	/* enumarate all local variables and forcibly initialize our targets */
+	FOR_EACH_LOCAL_DECL(cfun, i, var) {
+		tree type = TREE_TYPE(var);
+
+		gcc_assert(DECL_P(var));
+		if (!auto_var_in_fn_p(var, current_function_decl))
+			continue;
+
+		/* only care about structure types */
+		if (TREE_CODE(type) != RECORD_TYPE && TREE_CODE(type) != UNION_TYPE)
+			continue;
+
+		/* if the type is of interest, examine the variable */
+		if (TYPE_USERSPACE(type))
+			initialize(var);
+	}
+
+	return ret;
+}
+
+#define PASS_NAME structleak
+#define NO_GATE
+#define PROPERTIES_REQUIRED PROP_cfg
+#define TODO_FLAGS_FINISH TODO_verify_il | TODO_verify_ssa | TODO_verify_stmts | TODO_dump_func | TODO_remove_unused_locals | TODO_update_ssa | TODO_ggc_collect | TODO_verify_flow
+#include "gcc-generate-gimple-pass.h"
+
+__visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gcc_version *version)
+{
+	int i;
+	const char * const plugin_name = plugin_info->base_name;
+	const int argc = plugin_info->argc;
+	const struct plugin_argument * const argv = plugin_info->argv;
+	bool enable = true;
+
+	PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
+
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	if (strncmp(lang_hooks.name, "GNU C", 5) && !strncmp(lang_hooks.name, "GNU C+", 6)) {
+		inform(UNKNOWN_LOCATION, G_("%s supports C only, not %s"), plugin_name, lang_hooks.name);
+		enable = false;
+	}
+
+	for (i = 0; i < argc; ++i) {
+		if (!strcmp(argv[i].key, "disable")) {
+			enable = false;
+			continue;
+		}
+		if (!strcmp(argv[i].key, "verbose")) {
+			verbose = true;
+			continue;
+		}
+		error(G_("unknown option '-fplugin-arg-%s-%s'"), plugin_name, argv[i].key);
+	}
+
+	register_callback(plugin_name, PLUGIN_INFO, NULL, &structleak_plugin_info);
+	if (enable) {
+		register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL, &structleak_pass_info);
+		register_callback(plugin_name, PLUGIN_FINISH_TYPE, finish_type, NULL);
+	}
+	register_callback(plugin_name, PLUGIN_ATTRIBUTES, register_attributes, NULL);
+
+	return 0;
+}
-- 
2.7.4


-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-13 22:02 [PATCH] gcc-plugins: Add structleak for more stack initialization Kees Cook
@ 2017-01-14 10:03 ` PaX Team
  2017-01-16 15:24   ` Mark Rutland
  2017-01-17 17:48   ` Kees Cook
  2017-01-16 11:54 ` Mark Rutland
  1 sibling, 2 replies; 17+ messages in thread
From: PaX Team @ 2017-01-14 10:03 UTC (permalink / raw)
  To: kernel-hardening, Kees Cook
  Cc: Emese Revfy, AKASHI, Takahiro, Mark Rutland, park jinbum,
	Daniel Micay, linux-kernel, spender

On 13 Jan 2017 at 14:02, Kees Cook wrote:

> This plugin detects any structures that contain __user attributes and
> makes sure it is being fulling initialized so that a specific class of
> information exposure is eliminated. (For example, the exposure of siginfo
> in CVE-2013-2141 would have been blocked by this plugin.)

why the conditional? the plugin was specifically written to block that bug
and block it did ;).

> +config GCC_PLUGIN_STRUCTLEAK
> +	bool "Force initialization of variables containing userspace addresses"
> +	depends on GCC_PLUGINS
> +	help
> +	  This plugin zero-initializes any structures that containing a
> +	  __user attribute. This can prevent some classes of information
> +	  exposures.

i see that you completely ditched the description in PaX, is there a reason
for it? your text isn't correct as is because

- the __user attribute (which is an implementation choice, see below) doesn't
  apply to structures but pointers only (as it does for sparse IIRC)

- a structure is a type, but the plugin initializes variables, not types
  (the latter makes little sense)

- the plugin doesn't initialize 'any structures' (well, variables), only locals
  and only at function scope (subject to further evolution as discussed earlier).

> +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> +	bool "Report initialized variables"
> +	depends on GCC_PLUGIN_STRUCTLEAK
> +	depends on !COMPILE_TEST
> +	help
> +	  This option will cause a warning to be printed each time the
> +	  structleak plugin finds a variable it thinks needs to be
> +	  initialized. Since not all existing initializers are detected
> +	  by the plugin, this can produce false positive warnings.

there are no false positives, a variable either has a constructor or it does not ;)

> +/* unused C type flag in all versions 4.5-6 */
> +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)

FYI, this is a sort of abuse/hack of tree flags and should not be implemented this
way in the upstream kernel as it's a finite resource and needs careful verification
against all supported gcc versions (these flags are meant for language fronteds, i
kinda got lucky to have a few of them unusued but it's not a robust future-proof
approach). instead an attribute should be used to mark these types. whether that
can/should be __user itself is a good question since that's another hack where the
plugin 'hijacks' a sparse address space atttribute (for which gcc 4.6+ has its own
facilities and that the checker gcc plugin makes use of thus it's not compatible
with structleak as is).

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-13 22:02 [PATCH] gcc-plugins: Add structleak for more stack initialization Kees Cook
  2017-01-14 10:03 ` PaX Team
@ 2017-01-16 11:54 ` Mark Rutland
  2017-01-16 12:26   ` [kernel-hardening] " Mark Rutland
                     ` (2 more replies)
  1 sibling, 3 replies; 17+ messages in thread
From: Mark Rutland @ 2017-01-16 11:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: kernel-hardening, PaX Team, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, dave.martin

Hi,

[adding Dave, so retaining full context below]

On Fri, Jan 13, 2017 at 02:02:56PM -0800, Kees Cook wrote:
> This plugin detects any structures that contain __user attributes and
> makes sure it is being fulling initialized so that a specific class of

Nit: s/fulling/fully/

> information exposure is eliminated. (For example, the exposure of siginfo
> in CVE-2013-2141 would have been blocked by this plugin.)
> 
> Ported from grsecurity/PaX. This version adds a verbose option to the
> plugin and the Kconfig.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/Kconfig                            |  22 +++
>  include/linux/compiler.h                |   6 +-
>  scripts/Makefile.gcc-plugins            |   4 +
>  scripts/gcc-plugins/structleak_plugin.c | 246 ++++++++++++++++++++++++++++++++
>  4 files changed, 277 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/gcc-plugins/structleak_plugin.c

I tried giving this a go, but I got the build failure below:

----
[mark@leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
arch/arm64/Makefile:43: Detected assembler with broken .inst; disassembly will be unreliable
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  UPD     include/generated/utsrelease.h
  HOSTCXX -fPIC scripts/gcc-plugins/structleak_plugin.o
scripts/gcc-plugins/structleak_plugin.c: In function ‘int plugin_init(plugin_name_args*, plugin_gcc_version*)’:
scripts/gcc-plugins/structleak_plugin.c:214:12: error: ‘structleak’ was not declared in this scope
  PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
            ^
scripts/gcc-plugins/structleak_plugin.c:214:72: error: ‘PASS_INFO’ was not declared in this scope
  PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
                                                                        ^
scripts/gcc-plugins/structleak_plugin.c:240:68: error: ‘structleak_pass_info’ was not declared in this scope
   register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL, &structleak_pass_info);
                                                                    ^
make[1]: *** [scripts/gcc-plugins/structleak_plugin.o] Error 1
make: *** [gcc-plugins] Error 2
----

I'm using the Linaro 15.08 x86_64 aarch64-linux-gnu- cross toolchain. I believe
that can be found at:

https://releases.linaro.org/components/toolchain/binaries/5.1-2015.08/aarch64-linux-gnu/gcc-linaro-5.1-2015.08-x86_64_aarch64-linux-gnu.tar.xz

Unfortunately, due to that (and lack of a good testcase), I can't give
much substantial feedback on this.

> diff --git a/arch/Kconfig b/arch/Kconfig
> index 99839c23d453..f1250ba0b06f 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -410,6 +410,28 @@ config GCC_PLUGIN_LATENT_ENTROPY
>  	   * https://grsecurity.net/
>  	   * https://pax.grsecurity.net/
>  
> +config GCC_PLUGIN_STRUCTLEAK
> +	bool "Force initialization of variables containing userspace addresses"
> +	depends on GCC_PLUGINS
> +	help
> +	  This plugin zero-initializes any structures that containing a
> +	  __user attribute. This can prevent some classes of information
> +	  exposures.
> +
> +	  This plugin was ported from grsecurity/PaX. More information at:
> +	   * https://grsecurity.net/
> +	   * https://pax.grsecurity.net/
> +
> +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> +	bool "Report initialized variables"

It might be better to say "Report forcefully initialized variables", to make it
clear that this is only reporting initialization performed by the plugin.

> +	depends on GCC_PLUGIN_STRUCTLEAK
> +	depends on !COMPILE_TEST
> +	help
> +	  This option will cause a warning to be printed each time the
> +	  structleak plugin finds a variable it thinks needs to be
> +	  initialized. Since not all existing initializers are detected
> +	  by the plugin, this can produce false positive warnings.
> +
>  config HAVE_CC_STACKPROTECTOR
>  	bool
>  	help
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index cf0fa5d86059..91c30cba984e 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -27,7 +27,11 @@ extern void __chk_user_ptr(const volatile void __user *);
>  extern void __chk_io_ptr(const volatile void __iomem *);
>  # define ACCESS_PRIVATE(p, member) (*((typeof((p)->member) __force *) &(p)->member))
>  #else /* __CHECKER__ */
> -# define __user
> +# ifdef STRUCTLEAK_PLUGIN
> +#  define __user __attribute__((user))
> +# else
> +#  define __user
> +# endif
>  # define __kernel
>  # define __safe
>  # define __force
> diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
> index 060d2cb373db..a084f7a511d8 100644
> --- a/scripts/Makefile.gcc-plugins
> +++ b/scripts/Makefile.gcc-plugins
> @@ -25,6 +25,10 @@ ifdef CONFIG_GCC_PLUGINS
>      endif
>    endif
>  
> +  gcc-plugin-$(CONFIG_GCC_PLUGIN_STRUCTLEAK)	+= structleak_plugin.so
> +  gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK_VERBOSE)	+= -fplugin-arg-structleak_plugin-verbose
> +  gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STRUCTLEAK)	+= -DSTRUCTLEAK_PLUGIN
> +
>    GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
>  
>    export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
> diff --git a/scripts/gcc-plugins/structleak_plugin.c b/scripts/gcc-plugins/structleak_plugin.c
> new file mode 100644
> index 000000000000..deddb7291a26
> --- /dev/null
> +++ b/scripts/gcc-plugins/structleak_plugin.c
> @@ -0,0 +1,246 @@
> +/*
> + * Copyright 2013-2017 by PaX Team <pageexec@freemail.hu>
> + * Licensed under the GPL v2
> + *
> + * Note: the choice of the license means that the compilation process is
> + *       NOT 'eligible' as defined by gcc's library exception to the GPL v3,
> + *       but for the kernel it doesn't matter since it doesn't link against
> + *       any of the gcc libraries

It's my understanding that some architectures do link against libgcc, so we
might want some kind of guard to avoid mishaps. e.g.
ARCH_CAN_USE_NON_GPLV3_GCC_PLUGINS.

> + *
> + * gcc plugin to forcibly initialize certain local variables that could
> + * otherwise leak kernel stack to userland if they aren't properly initialized
> + * by later code
> + *
> + * Homepage: http://pax.grsecurity.net/
> + *
> + * Options:
> + * -fplugin-arg-structleak_plugin-disable
> + * -fplugin-arg-structleak_plugin-verbose
> + *
> + * Usage:
> + * $ # for 4.5/4.6/C based 4.7
> + * $ gcc -I`gcc -print-file-name=plugin`/include -I`gcc -print-file-name=plugin`/include/c-family -fPIC -shared -O2 -o structleak_plugin.so structleak_plugin.c
> + * $ # for C++ based 4.7/4.8+
> + * $ g++ -I`g++ -print-file-name=plugin`/include -I`g++ -print-file-name=plugin`/include/c-family -fPIC -shared -O2 -o structleak_plugin.so structleak_plugin.c
> + * $ gcc -fplugin=./structleak_plugin.so test.c -O2
> + *
> + * TODO: eliminate redundant initializers
> + *       increase type coverage
> + */
> +
> +#include "gcc-common.h"
> +
> +/* unused C type flag in all versions 4.5-6 */
> +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
> +
> +__visible int plugin_is_GPL_compatible;
> +
> +static struct plugin_info structleak_plugin_info = {
> +	.version	= "201607271510vanilla",

Has this been modified at all in the porting process? Maybe it would be
good to include KERNELVERSION here?

> +	.help		= "disable\tdo not activate plugin\n"
> +			   "verbose\tprint all initialized variables\n",
> +};
> +
> +static bool verbose;
> +
> +static tree handle_user_attribute(tree *node, tree name, tree args, int flags, bool *no_add_attrs)
> +{
> +	*no_add_attrs = true;
> +
> +	/* check for types? for now accept everything linux has to offer */
> +	if (TREE_CODE(*node) != FIELD_DECL)
> +		return NULL_TREE;
> +
> +	*no_add_attrs = false;
> +	return NULL_TREE;
> +}
> +
> +static struct attribute_spec user_attr = {
> +	.name			= "user",
> +	.min_length		= 0,
> +	.max_length		= 0,
> +	.decl_required		= false,
> +	.type_required		= false,
> +	.function_type_required	= false,
> +	.handler		= handle_user_attribute,
> +#if BUILDING_GCC_VERSION >= 4007
> +	.affects_type_identity	= true
> +#endif
> +};
> +
> +static void register_attributes(void *event_data, void *data)
> +{
> +	register_attribute(&user_attr);
> +}
> +
> +static tree get_field_type(tree field)
> +{
> +	return strip_array_types(TREE_TYPE(field));
> +}
> +
> +static bool is_userspace_type(tree type)
> +{
> +	tree field;
> +
> +	for (field = TYPE_FIELDS(type); field; field = TREE_CHAIN(field)) {
> +		tree fieldtype = get_field_type(field);
> +		enum tree_code code = TREE_CODE(fieldtype);
> +
> +		if (code == RECORD_TYPE || code == UNION_TYPE)
> +			if (is_userspace_type(fieldtype))
> +				return true;
> +
> +		if (lookup_attribute("user", DECL_ATTRIBUTES(field)))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static void finish_type(void *event_data, void *data)
> +{
> +	tree type = (tree)event_data;
> +
> +	if (type == NULL_TREE || type == error_mark_node)
> +		return;
> +
> +#if BUILDING_GCC_VERSION >= 5000
> +	if (TREE_CODE(type) == ENUMERAL_TYPE)
> +		return;
> +#endif
> +
> +	if (TYPE_USERSPACE(type))
> +		return;
> +
> +	if (is_userspace_type(type))
> +		TYPE_USERSPACE(type) = 1;
> +}
> +
> +static void initialize(tree var)
> +{
> +	basic_block bb;
> +	gimple_stmt_iterator gsi;
> +	tree initializer;
> +	gimple init_stmt;
> +
> +	/* this is the original entry bb before the forced split */
> +	bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
> +
> +	/* first check if variable is already initialized, warn otherwise */
> +	for (gsi = gsi_start_bb(bb); !gsi_end_p(gsi); gsi_next(&gsi)) {
> +		gimple stmt = gsi_stmt(gsi);
> +		tree rhs1;
> +
> +		/* we're looking for an assignment of a single rhs... */
> +		if (!gimple_assign_single_p(stmt))
> +			continue;
> +		rhs1 = gimple_assign_rhs1(stmt);
> +#if BUILDING_GCC_VERSION >= 4007
> +		/* ... of a non-clobbering expression... */
> +		if (TREE_CLOBBER_P(rhs1))
> +			continue;
> +#endif
> +		/* ... to our variable... */
> +		if (gimple_get_lhs(stmt) != var)
> +			continue;
> +		/* if it's an initializer then we're good */
> +		if (TREE_CODE(rhs1) == CONSTRUCTOR)
> +			return;
> +	}
> +
> +	/* these aren't the 0days you're looking for */
> +	if (verbose)
> +		inform(DECL_SOURCE_LOCATION(var),
> +			"userspace variable will be forcibly initialized");
> +
> +	/* build the initializer expression */
> +	initializer = build_constructor(TREE_TYPE(var), NULL);
> +
> +	/* build the initializer stmt */
> +	init_stmt = gimple_build_assign(var, initializer);
> +	gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> +	gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
> +	update_stmt(init_stmt);

I assume that this is only guaranteed to initialise fields in a struct,
and not padding, is that correct? I ask due to the issue described in:

https://lwn.net/Articles/417989/

As a further step, it would be interesting to see if we could fix
padding for __user variables, but that certainly shouldn't hold this
back.

> +}
> +
> +static unsigned int structleak_execute(void)
> +{
> +	basic_block bb;
> +	unsigned int ret = 0;
> +	tree var;
> +	unsigned int i;
> +
> +	/* split the first bb where we can put the forced initializers */
> +	gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> +	bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
> +	if (!single_pred_p(bb)) {
> +		split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> +		gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> +	}
> +
> +	/* enumarate all local variables and forcibly initialize our targets */

Nit: s/enumarate/enumerate/

> +	FOR_EACH_LOCAL_DECL(cfun, i, var) {
> +		tree type = TREE_TYPE(var);
> +
> +		gcc_assert(DECL_P(var));
> +		if (!auto_var_in_fn_p(var, current_function_decl))
> +			continue;
> +
> +		/* only care about structure types */
> +		if (TREE_CODE(type) != RECORD_TYPE && TREE_CODE(type) != UNION_TYPE)
> +			continue;
> +
> +		/* if the type is of interest, examine the variable */
> +		if (TYPE_USERSPACE(type))
> +			initialize(var);
> +	}
> +
> +	return ret;
> +}
> +
> +#define PASS_NAME structleak
> +#define NO_GATE
> +#define PROPERTIES_REQUIRED PROP_cfg
> +#define TODO_FLAGS_FINISH TODO_verify_il | TODO_verify_ssa | TODO_verify_stmts | TODO_dump_func | TODO_remove_unused_locals | TODO_update_ssa | TODO_ggc_collect | TODO_verify_flow
> +#include "gcc-generate-gimple-pass.h"
> +
> +__visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gcc_version *version)
> +{
> +	int i;
> +	const char * const plugin_name = plugin_info->base_name;
> +	const int argc = plugin_info->argc;
> +	const struct plugin_argument * const argv = plugin_info->argv;
> +	bool enable = true;
> +
> +	PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
> +
> +	if (!plugin_default_version_check(version, &gcc_version)) {
> +		error(G_("incompatible gcc/plugin versions"));
> +		return 1;
> +	}
> +
> +	if (strncmp(lang_hooks.name, "GNU C", 5) && !strncmp(lang_hooks.name, "GNU C+", 6)) {
> +		inform(UNKNOWN_LOCATION, G_("%s supports C only, not %s"), plugin_name, lang_hooks.name);
> +		enable = false;
> +	}
> +
> +	for (i = 0; i < argc; ++i) {
> +		if (!strcmp(argv[i].key, "disable")) {
> +			enable = false;
> +			continue;
> +		}
> +		if (!strcmp(argv[i].key, "verbose")) {
> +			verbose = true;
> +			continue;
> +		}
> +		error(G_("unknown option '-fplugin-arg-%s-%s'"), plugin_name, argv[i].key);
> +	}
> +
> +	register_callback(plugin_name, PLUGIN_INFO, NULL, &structleak_plugin_info);
> +	if (enable) {
> +		register_callback(plugin_name, PLUGIN_PASS_MANAGER_SETUP, NULL, &structleak_pass_info);
> +		register_callback(plugin_name, PLUGIN_FINISH_TYPE, finish_type, NULL);
> +	}
> +	register_callback(plugin_name, PLUGIN_ATTRIBUTES, register_attributes, NULL);
> +
> +	return 0;
> +}
> -- 
> 2.7.4

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [kernel-hardening] Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 11:54 ` Mark Rutland
@ 2017-01-16 12:26   ` Mark Rutland
  2017-01-16 19:22   ` PaX Team
  2017-01-17 17:56   ` Kees Cook
  2 siblings, 0 replies; 17+ messages in thread
From: Mark Rutland @ 2017-01-16 12:26 UTC (permalink / raw)
  To: Kees Cook
  Cc: kernel-hardening, PaX Team, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, dave.martin

On Mon, Jan 16, 2017 at 11:54:35AM +0000, Mark Rutland wrote:
> Hi,
> 
> [adding Dave, so retaining full context below]
> 
> On Fri, Jan 13, 2017 at 02:02:56PM -0800, Kees Cook wrote:
> > This plugin detects any structures that contain __user attributes and
> > makes sure it is being fulling initialized so that a specific class of
> 
> Nit: s/fulling/fully/
> 
> > information exposure is eliminated. (For example, the exposure of siginfo
> > in CVE-2013-2141 would have been blocked by this plugin.)
> > 
> > Ported from grsecurity/PaX. This version adds a verbose option to the
> > plugin and the Kconfig.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  arch/Kconfig                            |  22 +++
> >  include/linux/compiler.h                |   6 +-
> >  scripts/Makefile.gcc-plugins            |   4 +
> >  scripts/gcc-plugins/structleak_plugin.c | 246 ++++++++++++++++++++++++++++++++
> >  4 files changed, 277 insertions(+), 1 deletion(-)
> >  create mode 100644 scripts/gcc-plugins/structleak_plugin.c
> 
> I tried giving this a go, but I got the build failure below:

Looking again, I see that there was another patch to add PASS_INFO() and
other bits that the patch required. Sorry for the noise there.

In the mean time, I cribbed from the latent entropy plugin, and built
with the below applied:

----
diff --git a/scripts/gcc-plugins/structleak_plugin.c b/scripts/gcc-plugins/structleak_plugin.c
index deddb72..1e01763 100644
--- a/scripts/gcc-plugins/structleak_plugin.c
+++ b/scripts/gcc-plugins/structleak_plugin.c
@@ -210,8 +210,12 @@ __visible int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gc
        const int argc = plugin_info->argc;
        const struct plugin_argument * const argv = plugin_info->argv;
        bool enable = true;
-
-       PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
+       struct register_pass_info structleak_pass_info = {
+               .pass  = make_structleak_pass(),
+               .reference_pass_name = "early_optimizations",
+               .ref_pass_instance_number = 1,
+               .pos_op = PASS_POS_INSERT_BEFORE,
+       };
 
        if (!plugin_default_version_check(version, &gcc_version)) {
                error(G_("incompatible gcc/plugin versions"));
----

With verbose mode, I see quite a few initializations, mostly in signal
handling. Something to add to the queue of things to investigate...

Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-14 10:03 ` PaX Team
@ 2017-01-16 15:24   ` Mark Rutland
  2017-01-16 19:08     ` Daniel Micay
  2017-01-16 19:30     ` PaX Team
  2017-01-17 17:48   ` Kees Cook
  1 sibling, 2 replies; 17+ messages in thread
From: Mark Rutland @ 2017-01-16 15:24 UTC (permalink / raw)
  To: PaX Team
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, spender

Hi,

On Sat, Jan 14, 2017 at 11:03:14AM +0100, PaX Team wrote:
> On 13 Jan 2017 at 14:02, Kees Cook wrote:
> 
> > +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> > +	bool "Report initialized variables"
> > +	depends on GCC_PLUGIN_STRUCTLEAK
> > +	depends on !COMPILE_TEST
> > +	help
> > +	  This option will cause a warning to be printed each time the
> > +	  structleak plugin finds a variable it thinks needs to be
> > +	  initialized. Since not all existing initializers are detected
> > +	  by the plugin, this can produce false positive warnings.
> 
> there are no false positives, a variable either has a constructor or it does not ;)

... or it has no constructor, but is clobbered by a memset before it is
possibly copied. ;)

For example:

arch/arm64/kernel/fpsimd.c: In function 'do_fpsimd_exc':
arch/arm64/kernel/fpsimd.c:106:12: note: userspace variable will be forcibly initialized
  siginfo_t info;

... where the code looks like:

void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
{
	siginfo_t info;
	unsigned int si_code = 0;

	if (esr & FPEXC_IOF)
		si_code = FPE_FLTINV;
	else if (esr & FPEXC_DZF)
		si_code = FPE_FLTDIV;
	else if (esr & FPEXC_OFF)
		si_code = FPE_FLTOVF;
	else if (esr & FPEXC_UFF)
		si_code = FPE_FLTUND;
	else if (esr & FPEXC_IXF)
		si_code = FPE_FLTRES;

	memset(&info, 0, sizeof(info));
	info.si_signo = SIGFPE;
	info.si_code = si_code;
	info.si_addr = (void __user *)instruction_pointer(regs);

	send_sig_info(SIGFPE, &info, current);
}

... so it's clear to a human that info is initialised prior to use,
though not by an explicit field initializer.

> > +/* unused C type flag in all versions 4.5-6 */
> > +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
> 
> FYI, this is a sort of abuse/hack of tree flags and should not be implemented this
> way in the upstream kernel as it's a finite resource and needs careful verification
> against all supported gcc versions (these flags are meant for language fronteds, i
> kinda got lucky to have a few of them unusued but it's not a robust future-proof
> approach). instead an attribute should be used to mark these types. whether that
> can/should be __user itself is a good question since that's another hack where the
> plugin 'hijacks' a sparse address space atttribute (for which gcc 4.6+ has its own
> facilities and that the checker gcc plugin makes use of thus it's not compatible
> with structleak as is).

To me, it seems that the __user annotation can only be an indicator of
an issue by chance. We have structures with __user pointers in structs
that will never be copied to userspace, and conversely we have structs
that don't contain a __user field, but will be copied to userspace.

Maybe it happens that structs in more complex systems are more likely to
contain some __user pointer. Was that part of the rationale?

I wonder if there's any analysis we can do of data passing into
copy_to_user() and friends. I guess we can't follow the data flow across
compilation units, but we might be able to follow it well enough if we
added a new attribute that described whether data was to be copied to
userspace.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 15:24   ` Mark Rutland
@ 2017-01-16 19:08     ` Daniel Micay
  2017-01-16 19:30     ` PaX Team
  1 sibling, 0 replies; 17+ messages in thread
From: Daniel Micay @ 2017-01-16 19:08 UTC (permalink / raw)
  To: Mark Rutland, PaX Team
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, linux-kernel, spender

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

On Mon, 2017-01-16 at 15:24 +0000, Mark Rutland wrote:
> Hi,
> 
> On Sat, Jan 14, 2017 at 11:03:14AM +0100, PaX Team wrote:
> > On 13 Jan 2017 at 14:02, Kees Cook wrote:
> > 
> > > +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
> > > +	bool "Report initialized variables"
> > > +	depends on GCC_PLUGIN_STRUCTLEAK
> > > +	depends on !COMPILE_TEST
> > > +	help
> > > +	  This option will cause a warning to be printed each
> > > time the
> > > +	  structleak plugin finds a variable it thinks needs to
> > > be
> > > +	  initialized. Since not all existing initializers are
> > > detected
> > > +	  by the plugin, this can produce false positive
> > > warnings.
> > 
> > there are no false positives, a variable either has a constructor or
> > it does not ;)
> 
> ... or it has no constructor, but is clobbered by a memset before it
> is
> possibly copied. ;)
> 
> For example:
> 
> arch/arm64/kernel/fpsimd.c: In function 'do_fpsimd_exc':
> arch/arm64/kernel/fpsimd.c:106:12: note: userspace variable will be
> forcibly initialized
>   siginfo_t info;
> 
> ... where the code looks like:
> 
> void do_fpsimd_exc(unsigned int esr, struct pt_regs *regs)
> {
> 	siginfo_t info;
> 	unsigned int si_code = 0;
> 
> 	if (esr & FPEXC_IOF)
> 		si_code = FPE_FLTINV;
> 	else if (esr & FPEXC_DZF)
> 		si_code = FPE_FLTDIV;
> 	else if (esr & FPEXC_OFF)
> 		si_code = FPE_FLTOVF;
> 	else if (esr & FPEXC_UFF)
> 		si_code = FPE_FLTUND;
> 	else if (esr & FPEXC_IXF)
> 		si_code = FPE_FLTRES;
> 
> 	memset(&info, 0, sizeof(info));
> 	info.si_signo = SIGFPE;
> 	info.si_code = si_code;
> 	info.si_addr = (void __user *)instruction_pointer(regs);
> 
> 	send_sig_info(SIGFPE, &info, current);
> }
> 
> ... so it's clear to a human that info is initialised prior to use,
> though not by an explicit field initializer.

It's obvious to the compiler too, not just a human. The compiler can
optimize it out when it's so clearly not required, although translation
units will get in the way without LTO. There's existing sophisticated
analysis for automatic initialization with full coverage. That's part of
why I think it makes sense to have a toggle for heuristics vs. full
coverage (incl. non-function-scope locals, that's just a heuristic too).

> > > +/* unused C type flag in all versions 4.5-6 */
> > > +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
> > 
> > FYI, this is a sort of abuse/hack of tree flags and should not be
> > implemented this
> > way in the upstream kernel as it's a finite resource and needs
> > careful verification
> > against all supported gcc versions (these flags are meant for
> > language fronteds, i
> > kinda got lucky to have a few of them unusued but it's not a robust
> > future-proof
> > approach). instead an attribute should be used to mark these types.
> > whether that
> > can/should be __user itself is a good question since that's another
> > hack where the
> > plugin 'hijacks' a sparse address space atttribute (for which gcc
> > 4.6+ has its own
> > facilities and that the checker gcc plugin makes use of thus it's
> > not compatible
> > with structleak as is).
> 
> To me, it seems that the __user annotation can only be an indicator of
> an issue by chance. We have structures with __user pointers in structs
> that will never be copied to userspace, and conversely we have structs
> that don't contain a __user field, but will be copied to userspace.
> 
> Maybe it happens that structs in more complex systems are more likely
> to
> contain some __user pointer. Was that part of the rationale?
> 
> I wonder if there's any analysis we can do of data passing into
> copy_to_user() and friends. I guess we can't follow the data flow
> across
> compilation units, but we might be able to follow it well enough if we
> added a new attribute that described whether data was to be copied to
> userspace.
> 
> Thanks,
> Mark.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 11:54 ` Mark Rutland
  2017-01-16 12:26   ` [kernel-hardening] " Mark Rutland
@ 2017-01-16 19:22   ` PaX Team
  2017-01-17 10:42     ` Dave P Martin
  2017-01-17 17:56   ` Kees Cook
  2 siblings, 1 reply; 17+ messages in thread
From: PaX Team @ 2017-01-16 19:22 UTC (permalink / raw)
  To: Kees Cook, Mark Rutland
  Cc: kernel-hardening, Emese Revfy, AKASHI, Takahiro, park jinbum,
	Daniel Micay, linux-kernel, dave.martin, spender

On 16 Jan 2017 at 11:54, Mark Rutland wrote:

> > + * Copyright 2013-2017 by PaX Team <pageexec@freemail.hu>
> > + * Licensed under the GPL v2
> > + *
> > + * Note: the choice of the license means that the compilation process is
> > + *       NOT 'eligible' as defined by gcc's library exception to the GPL v3,
> > + *       but for the kernel it doesn't matter since it doesn't link against
> > + *       any of the gcc libraries
> 
> It's my understanding that some architectures do link against libgcc, so we
> might want some kind of guard to avoid mishaps. e.g.
> ARCH_CAN_USE_NON_GPLV3_GCC_PLUGINS.

AFAIK, plugins aren't enabled on any such archs yet and if/when they get
enabled, the better approach would be to simply reimplement those helper
routines in the kernel itself like some archs already do.

> > +	/* build the initializer expression */
> > +	initializer = build_constructor(TREE_TYPE(var), NULL);
> > +
> > +	/* build the initializer stmt */
> > +	init_stmt = gimple_build_assign(var, initializer);
> > +	gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
> > +	gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
> > +	update_stmt(init_stmt);
> 
> I assume that this is only guaranteed to initialise fields in a struct,
> and not padding, is that correct? I ask due to the issue described in:
> 
> https://lwn.net/Articles/417989/

the 'issue' is that before C11 the standard didn't make it clear that in
case of a partial initializer list the compiler has to initialize not only
the remaining fields but also padding as well.

as for what the above code does, it fixes this as well since gcc doesn't
emit the constructor itself which will then match the pattern.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 15:24   ` Mark Rutland
  2017-01-16 19:08     ` Daniel Micay
@ 2017-01-16 19:30     ` PaX Team
  2017-01-17 17:48       ` Mark Rutland
  1 sibling, 1 reply; 17+ messages in thread
From: PaX Team @ 2017-01-16 19:30 UTC (permalink / raw)
  To: Mark Rutland
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, spender

On 16 Jan 2017 at 15:24, Mark Rutland wrote:

> To me, it seems that the __user annotation can only be an indicator of
> an issue by chance. We have structures with __user pointers in structs
> that will never be copied to userspace, and conversely we have structs
> that don't contain a __user field, but will be copied to userspace.
> 
> Maybe it happens that structs in more complex systems are more likely to
> contain some __user pointer. Was that part of the rationale?

it's as i explained in an earlier email: we wanted to pattern match a
specific bug situation and this was the easiest way (as you can see,
the plugin's code is very simple, not much effort went into it).

> I wonder if there's any analysis we can do of data passing into
> copy_to_user() and friends. I guess we can't follow the data flow across
> compilation units, but we might be able to follow it well enough if we
> added a new attribute that described whether data was to be copied to
> userspace.

there're are all kinds of data flow analyses you can do within and even
across translation units (summary info a'la size overflow hash tables or
LTO). i never went into that direction because i think the security goal
can be achieved without the performance impact of forced initialization.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 19:22   ` PaX Team
@ 2017-01-17 10:42     ` Dave P Martin
       [not found]       ` <587E4FDD.31940.D47F642@pageexec.freemail.hu>
  0 siblings, 1 reply; 17+ messages in thread
From: Dave P Martin @ 2017-01-17 10:42 UTC (permalink / raw)
  To: PaX Team
  Cc: Kees Cook, Mark Rutland, kernel-hardening, Emese Revfy, AKASHI,
	Takahiro, park jinbum, Daniel Micay, linux-kernel, spender

On Mon, Jan 16, 2017 at 08:22:24PM +0100, PaX Team wrote:
> On 16 Jan 2017 at 11:54, Mark Rutland wrote:

[...]

> > I assume that this is only guaranteed to initialise fields in a struct,
> > and not padding, is that correct? I ask due to the issue described in:
> >
> > https://lwn.net/Articles/417989/
>
> the 'issue' is that before C11 the standard didn't make it clear that in
> case of a partial initializer list the compiler has to initialize not only
> the remaining fields but also padding as well.

Which version of the C11 spec clarifies this?

The one I'm looking at (n1570, Apr 12 2011) says:

"6.7.9 21 If there are fewer initializers in a brace-enclosed list than
there are elements or members of an aggregate, or fewer characters in a
string literal used to initialize an array of known size than there are
elements in the array, the remainder of the aggregate shall be
initialized implicitly the same as objects that have static storage
duration."

What is meant by "the remainder of the object" is unclear.  Is this
just the tail of the object, or all unitialised members --
which may be sparse in the case of an initialiser with designators?
And is padding (and if so, which padding) considered to be part of (the
remainder of) the object for the purpose of this paragraph?

This can be read with the interpretation you suggest, but the wording
doesn't seem rock-solid.  For the kernel, I guess it's sufficient if
GCC commits to this interpretation though.


A related, perhaps more interesting issue is that C11 still explicitly
states that padding is undefined after assignment:

"6.2.6.1 6 When a value is stored in an object of structure or union
type, including in a member object, the bytes of the object
representation that correspond to any padding bytes take unspecified
values.  51)"

"51) Thus, for example, structure assignment need not copy any padding
bits."

which means that in:

{
        struct foo foo1 = FOO1_INTIALIZER;
        struct foo foo2;

        foo2 = foo1;
}

the contents of padding in foo2 is unspecified.

Similarly, piecemeal initialisation of an object by assigning to every
member leaves the padding unspecified, e.g.:

{
        struct timeval tv;

        tv.tv_sec = secs;
        tv.tv_usec = usecs;
}

(On most or all arches struct timeval contains no padding, but relying
implicitly on this is still a bit iffy.)


It's highly likely that the kernel passes objects resulting from one of
the above to put_user() and friends.  It would be good if we had a way
to catch at least some of these.

(I don't know whether that falls naturally under this plugin.)

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 19:30     ` PaX Team
@ 2017-01-17 17:48       ` Mark Rutland
  2017-01-17 18:54         ` PaX Team
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2017-01-17 17:48 UTC (permalink / raw)
  To: PaX Team
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, spender

On Mon, Jan 16, 2017 at 08:30:29PM +0100, PaX Team wrote:
> On 16 Jan 2017 at 15:24, Mark Rutland wrote:
> 
> > To me, it seems that the __user annotation can only be an indicator of
> > an issue by chance. We have structures with __user pointers in structs
> > that will never be copied to userspace, and conversely we have structs
> > that don't contain a __user field, but will be copied to userspace.
> > 
> > Maybe it happens that structs in more complex systems are more likely to
> > contain some __user pointer. Was that part of the rationale?
> 
> it's as i explained in an earlier email: we wanted to pattern match a
> specific bug situation and this was the easiest way (as you can see,
> the plugin's code is very simple, not much effort went into it).

Ok.

That being the case, (and given the relevant bug has now been fixed),
it's not clear to me what the value of this is today. i.e. given the
general case, is this preventing many leaks?

> > I wonder if there's any analysis we can do of data passing into
> > copy_to_user() and friends. I guess we can't follow the data flow across
> > compilation units, but we might be able to follow it well enough if we
> > added a new attribute that described whether data was to be copied to
> > userspace.
> 
> there're are all kinds of data flow analyses you can do within and even
> across translation units (summary info a'la size overflow hash tables or
> LTO). 

Sure.

> i never went into that direction because i think the security goal can
> be achieved without the performance impact of forced initialization.

Was there a particular technique you had in mind?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-14 10:03 ` PaX Team
  2017-01-16 15:24   ` Mark Rutland
@ 2017-01-17 17:48   ` Kees Cook
  1 sibling, 0 replies; 17+ messages in thread
From: Kees Cook @ 2017-01-17 17:48 UTC (permalink / raw)
  To: PaX Team
  Cc: kernel-hardening, Emese Revfy, AKASHI, Takahiro, Mark Rutland,
	park jinbum, Daniel Micay, LKML

On Sat, Jan 14, 2017 at 2:03 AM, PaX Team <pageexec@freemail.hu> wrote:
> On 13 Jan 2017 at 14:02, Kees Cook wrote:
>
>> This plugin detects any structures that contain __user attributes and
>> makes sure it is being fulling initialized so that a specific class of
>> information exposure is eliminated. (For example, the exposure of siginfo
>> in CVE-2013-2141 would have been blocked by this plugin.)
>
> why the conditional? the plugin was specifically written to block that bug
> and block it did ;).

I can rephrase this. I just wanted to give an example. How about:

 This plugin detects any structures that contain __user attributes and
 makes sure it is being fulling initialized so that a specific class of
 information exposure is eliminated. (This plugin was originally
 designed to block the exposure of siginfo in CVE-2013-2141.)

>> +config GCC_PLUGIN_STRUCTLEAK
>> +     bool "Force initialization of variables containing userspace addresses"
>> +     depends on GCC_PLUGINS
>> +     help
>> +       This plugin zero-initializes any structures that containing a
>> +       __user attribute. This can prevent some classes of information
>> +       exposures.
>
> i see that you completely ditched the description in PaX, is there a reason
> for it?

Yup, details below...

I didn't use the "bool" text because it wasn't accurate:

        bool "Forcibly initialize local variables copied to userland"

This implies "all", but it's not, and it has nothing to do with stuff
copied to userland. It's just looking a __user, with no examination of
copy_to_user() calls.

The rest:

          By saying Y here the kernel will zero initialize some local
          variables that are going to be copied to userland.  This in

Like the bool text, this isn't accurate. I can be specific about the
"some", but the "copied to userland" just isn't true.

          turn prevents unintended information leakage from the kernel
          stack should later code forget to explicitly set all parts of
          the copied variable.

I could reuse this part, sure. I tend to like to use the term
"exposure" instead of "leak", though, so at this point I just rewrote
the description of the "why".

          The tradeoff is less performance impact than PAX_MEMORY_STACKLEAK
          at a much smaller coverage.

I left this out because upstream doesn't have STACKLEAK to compare against yet.

          Note that the implementation requires a gcc with plugin support,
          i.e., gcc 4.5 or newer.  You may need to install the supporting
          headers explicitly in addition to the normal gcc package.

I left this out because it's redundant with the description of
CONFIG_GCC_PLUGINS.

> your text isn't correct as is because
>
> - the __user attribute (which is an implementation choice, see below) doesn't
>   apply to structures but pointers only (as it does for sparse IIRC)
> - a structure is a type, but the plugin initializes variables, not types
>   (the latter makes little sense)
> - the plugin doesn't initialize 'any structures' (well, variables), only locals
>   and only at function scope (subject to further evolution as discussed earlier).

Including mention of "__user" in Kconfig help text is already a bit
"too technical", so I was trying to be concise. I could easily expand
this to "any structure variables with __user pointers in function
scope", but it seemed like unneeded details. (This level of detail
isn't present in the PaX Kconfig either...)

>> +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
>> +     bool "Report initialized variables"
>> +     depends on GCC_PLUGIN_STRUCTLEAK
>> +     depends on !COMPILE_TEST
>> +     help
>> +       This option will cause a warning to be printed each time the
>> +       structleak plugin finds a variable it thinks needs to be
>> +       initialized. Since not all existing initializers are detected
>> +       by the plugin, this can produce false positive warnings.
>
> there are no false positives, a variable either has a constructor or it does not ;)

Well, as pointed out, there are plenty of false positives where the
plug reports the need to initialize the variable when it doesn't. It
doesn't report that it's missing a constructor. :) This is a pragmatic
description of what is happening, and since the plugin does sometimes
needlessly insert initializations where none are needed, that really
seems like a false positive to me. :)

>> +/* unused C type flag in all versions 4.5-6 */
>> +#define TYPE_USERSPACE(TYPE) TYPE_LANG_FLAG_5(TYPE)
>
> FYI, this is a sort of abuse/hack of tree flags and should not be implemented this
> way in the upstream kernel as it's a finite resource and needs careful verification
> against all supported gcc versions (these flags are meant for language fronteds, i
> kinda got lucky to have a few of them unusued but it's not a robust future-proof
> approach). instead an attribute should be used to mark these types. whether that
> can/should be __user itself is a good question since that's another hack where the
> plugin 'hijacks' a sparse address space atttribute (for which gcc 4.6+ has its own
> facilities and that the checker gcc plugin makes use of thus it's not compatible
> with structleak as is).

Yeah, I wasn't too happy about that lang flag usage either. It seems
reasonable to just build an attribute on the fly. IIRC, initify does
this already to mark things it has already processed?

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-16 11:54 ` Mark Rutland
  2017-01-16 12:26   ` [kernel-hardening] " Mark Rutland
  2017-01-16 19:22   ` PaX Team
@ 2017-01-17 17:56   ` Kees Cook
  2 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2017-01-17 17:56 UTC (permalink / raw)
  To: Mark Rutland
  Cc: kernel-hardening, PaX Team, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, LKML, Dave Martin

On Mon, Jan 16, 2017 at 3:54 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> [adding Dave, so retaining full context below]
>
> On Fri, Jan 13, 2017 at 02:02:56PM -0800, Kees Cook wrote:
>> This plugin detects any structures that contain __user attributes and
>> makes sure it is being fulling initialized so that a specific class of
>
> Nit: s/fulling/fully/

Whoops, thanks, fixed.

>> information exposure is eliminated. (For example, the exposure of siginfo
>> in CVE-2013-2141 would have been blocked by this plugin.)
>>
>> Ported from grsecurity/PaX. This version adds a verbose option to the
>> plugin and the Kconfig.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  arch/Kconfig                            |  22 +++
>>  include/linux/compiler.h                |   6 +-
>>  scripts/Makefile.gcc-plugins            |   4 +
>>  scripts/gcc-plugins/structleak_plugin.c | 246 ++++++++++++++++++++++++++++++++
>>  4 files changed, 277 insertions(+), 1 deletion(-)
>>  create mode 100644 scripts/gcc-plugins/structleak_plugin.c
>
> I tried giving this a go, but I got the build failure below:
>
> ----
> [mark@leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
> arch/arm64/Makefile:43: Detected assembler with broken .inst; disassembly will be unreliable
>   CHK     include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   UPD     include/generated/utsrelease.h
>   HOSTCXX -fPIC scripts/gcc-plugins/structleak_plugin.o
> scripts/gcc-plugins/structleak_plugin.c: In function ‘int plugin_init(plugin_name_args*, plugin_gcc_version*)’:
> scripts/gcc-plugins/structleak_plugin.c:214:12: error: ‘structleak’ was not declared in this scope
>   PASS_INFO(structleak, "early_optimizations", 1, PASS_POS_INSERT_BEFORE);
>             ^

Sorry, yes, this depends on the gcc-plugins changes in -next.

> [...]
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 99839c23d453..f1250ba0b06f 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -410,6 +410,28 @@ config GCC_PLUGIN_LATENT_ENTROPY
>>          * https://grsecurity.net/
>>          * https://pax.grsecurity.net/
>>
>> +config GCC_PLUGIN_STRUCTLEAK
>> +     bool "Force initialization of variables containing userspace addresses"
>> +     depends on GCC_PLUGINS
>> +     help
>> +       This plugin zero-initializes any structures that containing a
>> +       __user attribute. This can prevent some classes of information
>> +       exposures.
>> +
>> +       This plugin was ported from grsecurity/PaX. More information at:
>> +        * https://grsecurity.net/
>> +        * https://pax.grsecurity.net/
>> +
>> +config GCC_PLUGIN_STRUCTLEAK_VERBOSE
>> +     bool "Report initialized variables"
>
> It might be better to say "Report forcefully initialized variables", to make it
> clear that this is only reporting initialization performed by the plugin.

Sounds good, changed.

> [...]
>> +     /* these aren't the 0days you're looking for */
>> +     if (verbose)
>> +             inform(DECL_SOURCE_LOCATION(var),
>> +                     "userspace variable will be forcibly initialized");
>> +
>> +     /* build the initializer expression */
>> +     initializer = build_constructor(TREE_TYPE(var), NULL);
>> +
>> +     /* build the initializer stmt */
>> +     init_stmt = gimple_build_assign(var, initializer);
>> +     gsi = gsi_after_labels(single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> +     gsi_insert_before(&gsi, init_stmt, GSI_NEW_STMT);
>> +     update_stmt(init_stmt);
>
> I assume that this is only guaranteed to initialise fields in a struct,
> and not padding, is that correct? I ask due to the issue described in:
>
> https://lwn.net/Articles/417989/
>
> As a further step, it would be interesting to see if we could fix
> padding for __user variables, but that certainly shouldn't hold this
> back.

This spawned a whole thread. :) For non-C11, yeah, a plugin to do this
would be nice. That's already on the KSPP TODO list, but from what I
can tell it either requires walking every variable's structure to
check for sizes and padding or do explicitly add a constructor for
every variable and hope that the optimization phase does the right
thing. ;)

>> +}
>> +
>> +static unsigned int structleak_execute(void)
>> +{
>> +     basic_block bb;
>> +     unsigned int ret = 0;
>> +     tree var;
>> +     unsigned int i;
>> +
>> +     /* split the first bb where we can put the forced initializers */
>> +     gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> +     bb = single_succ(ENTRY_BLOCK_PTR_FOR_FN(cfun));
>> +     if (!single_pred_p(bb)) {
>> +             split_edge(single_succ_edge(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> +             gcc_assert(single_succ_p(ENTRY_BLOCK_PTR_FOR_FN(cfun)));
>> +     }
>> +
>> +     /* enumarate all local variables and forcibly initialize our targets */
>
> Nit: s/enumarate/enumerate/

Fixed.

Thanks for the review!

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
       [not found]       ` <587E4FDD.31940.D47F642@pageexec.freemail.hu>
@ 2017-01-17 18:07         ` Dave P Martin
  2017-01-17 19:25           ` PaX Team
  0 siblings, 1 reply; 17+ messages in thread
From: Dave P Martin @ 2017-01-17 18:07 UTC (permalink / raw)
  To: PaX Team
  Cc: Kees Cook, Mark Rutland, kernel-hardening, Emese Revfy, AKASHI,
	Takahiro, park jinbum, Daniel Micay, linux-kernel, spender

On Tue, Jan 17, 2017 at 06:09:49PM +0100, PaX Team wrote:
> On 17 Jan 2017 at 10:42, Dave P Martin wrote:
>
> > On Mon, Jan 16, 2017 at 08:22:24PM +0100, PaX Team wrote:
> > > the 'issue' is that before C11 the standard didn't make it clear that in
> > > case of a partial initializer list the compiler has to initialize not only
> > > the remaining fields but also padding as well.
> >
> > Which version of the C11 spec clarifies this?
> >
> > The one I'm looking at (n1570, Apr 12 2011) says:
> >
> > "6.7.9 21 If there are fewer initializers in a brace-enclosed list than
> > there are elements or members of an aggregate, or fewer characters in a
> > string literal used to initialize an array of known size than there are
> > elements in the array, the remainder of the aggregate shall be
> > initialized implicitly the same as objects that have static storage
> > duration."
> >
> > What is meant by "the remainder of the object" is unclear.
>
> it's less unclear if you also take into account the rest of the
> sentence that says "[initialized] the same as objects that have
> static storage duration" which in turn is described at 6.7.9p10
> where it says among others:
>
>   "if it is an aggregate, every member is initialized (recursively)
>    according to these rules, and any padding is initialized to zero bits"
>
> the "and any padding is initialized to zero bits" was the addition
> in C11 (which i'm not sure was meant as a new feature for C11 or
> just official enshrinement of what had always been meant).
>
> >  Is this just the tail of the object, or all unitialised members --
> > which may be sparse in the case of an initialiser with designators? And
> > is padding (and if so, which padding) considered to be part of (the
> > remainder of) the object for the purpose of this paragraph?
>
> to me the quote about 'any padding' is for all kinds of padding.
> uninitialized members are described in the same paragraph.
>
> > This can be read with the interpretation you suggest, but the wording
> > doesn't seem rock-solid.  For the kernel, I guess it's sufficient if
> > GCC commits to this interpretation though.
>
> note that i'm not a language lawyer, merely an interpreter, so take
> the above with a grain of salt.

Me neither, but actually I think this interpretation doesn't make sense.
The implication seems to be that padding initialisation of initialised
aggregates with automatic storage is guaranteed _only_ if the
initialiser is incomplete.


Paragraph 19 always applies, but that only asserts "all subobjects that
are not initialized explicitly shall be initialized implicitly the same
as objects that have static storage duration" -- no statement about
padding.

Paragraph 21 guarantees initialisation of padding bytes, but applies
_only_ if the initializer list is incomplete.


As a random test, a recent-ish gcc might actually apply this
interpretation...


tst.c
---8<---

struct foo {
        long x;
        char y;
        long z;
        int w;
};

void f(struct foo *);

void g(long *x, char *y, long *z, int *w)
{
        struct foo o = { .z = *z, .y = *y, .x = *x, .w = *w };

        f(&o);
}

void h(int *y, long *z)
{
        struct foo o = { .z = *z, .y = *y };

        f(&o);
}

--->8---


$ gcc (Debian 6.3.0-2) 6.3.0 20161229
$ gcc -Wall -Wextra -pedantic -O2 -std=c11 -c tst.c
$ objdump -d tst.o

tst.o:     file format elf64-littleaarch64


Disassembly of section .text:

0000000000000000 <g>:
   0:   a9bd7bfd        stp     x29, x30, [sp, #-48]!
   4:   910003fd        mov     x29, sp
   8:   39400024        ldrb    w4, [x1]
   c:   f9400005        ldr     x5, [x0]
  10:   910043a0        add     x0, x29, #0x10
  14:   b9400061        ldr     w1, [x3]
  18:   f9400042        ldr     x2, [x2]
  1c:   390063a4        strb    w4, [x29, #24]
  20:   f9000ba5        str     x5, [x29, #16]
  24:   f90013a2        str     x2, [x29, #32]
  28:   b9002ba1        str     w1, [x29, #40]
  2c:   94000000        bl      0 <f>
  30:   a8c37bfd        ldp     x29, x30, [sp], #48
  34:   d65f03c0        ret

0000000000000038 <h>:
  38:   a9bd7bfd        stp     x29, x30, [sp, #-48]!
  3c:   910003fd        mov     x29, sp
  40:   b9400002        ldr     w2, [x0]
  44:   910043a0        add     x0, x29, #0x10
  48:   f9400021        ldr     x1, [x1]
  4c:   a9017fbf        stp     xzr, xzr, [x29, #16]
  50:   a9027fbf        stp     xzr, xzr, [x29, #32]
  54:   390063a2        strb    w2, [x29, #24]
  58:   f90013a1        str     x1, [x29, #32]
  5c:   94000000        bl      0 <f>
  60:   a8c37bfd        ldp     x29, x30, [sp], #48
  64:   d65f03c0        ret


In g(), where the initialiser is complete, the padding in o is
definitely not zeroed out.  In h(), the padding is all zeroed out,
including padding around the zeoed members -- whether gcc intentionally
guarantees this or it's just a coincidence, I don't know.

This may or may not be a bug, and may or may not have been fixed in a
more recent version.

>
> > A related, perhaps more interesting issue is that C11 still explicitly
> > states that padding is undefined after assignment:
>
> you probably meant 'unspecified'?

Indeed.  Sloppy of me...

> > "6.2.6.1 6 When a value is stored in an object of structure or union
> > type, including in a member object, the bytes of the object
> > representation that correspond to any padding bytes take unspecified
> > values.  51)"
> >
> > "51) Thus, for example, structure assignment need not copy any padding
> > bits."
> >
> > which means that in:
> >
> > {
> >         struct foo foo1 = FOO1_INTIALIZER;
> >         struct foo foo2;
> >
> >         foo2 = foo1;
> > }
> >
> > the contents of padding in foo2 is unspecified.
> >
> > Similarly, piecemeal initialisation of an object by assigning to every
> > member leaves the padding unspecified, e.g.:
> >
> > {
> >         struct timeval tv;
> >
> >         tv.tv_sec = secs;
> >         tv.tv_usec = usecs;
> > }
> >
> > (On most or all arches struct timeval contains no padding, but relying
> > implicitly on this is still a bit iffy.)
> >
> >
> > It's highly likely that the kernel passes objects resulting from one of
> > the above to put_user() and friends.  It would be good if we had a way
> > to catch at least some of these.
>
> both examples would be handled by the plugin if the types involved already
> have a __user annotated field (as neither foo2 nor tv are initialized, only
> assigned to), otherwise it'd need manual annotation and/or more static analysis.

Do you see any false positives here?  I would expect a fair number of
structures that contain __user pointers but that are never copied
wholesale to userspace, but I've not reviewed in detail.

I would also expect false negatives to be common -- structures that lack
__user pointers but _are_ copied to userspace.  But again, if this
plugin is not designed to catch these in the first place, this may be
best addressed in a different way.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-17 17:48       ` Mark Rutland
@ 2017-01-17 18:54         ` PaX Team
  2017-01-18 10:48           ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: PaX Team @ 2017-01-17 18:54 UTC (permalink / raw)
  To: Mark Rutland
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, spender

On 17 Jan 2017 at 17:48, Mark Rutland wrote:

> That being the case, (and given the relevant bug has now been fixed),
> it's not clear to me what the value of this is today. i.e. given the
> general case, is this preventing many leaks?

no idea, i stopped looking at the instrumentation log long ago, but everyone
can enable the debug output (has a very specific comment on it ;) and look at
the results. i keep this plugin around because it costs nothing to maintain
it and the alternative (better) solution doesn't exist yet.

> > i never went into that direction because i think the security goal can
> > be achieved without the performance impact of forced initialization.
> 
> Was there a particular technique you had in mind?

sure, i mentioned it in my SSTIC'12 keynote (page 36):
https://pax.grsecurity.net/docs/PaXTeam-SSTIC12-keynote-20-years-of-PaX.pdf

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-17 18:07         ` Dave P Martin
@ 2017-01-17 19:25           ` PaX Team
  2017-01-17 22:04             ` Dave P Martin
  0 siblings, 1 reply; 17+ messages in thread
From: PaX Team @ 2017-01-17 19:25 UTC (permalink / raw)
  To: Dave P Martin
  Cc: Kees Cook, Mark Rutland, kernel-hardening, Emese Revfy, AKASHI,
	Takahiro, park jinbum, Daniel Micay, linux-kernel, spender

On 17 Jan 2017 at 18:07, Dave P Martin wrote:

> On Tue, Jan 17, 2017 at 06:09:49PM +0100, PaX Team wrote:
> > On 17 Jan 2017 at 10:42, Dave P Martin wrote:
> >
> > > This can be read with the interpretation you suggest, but the wording
> > > doesn't seem rock-solid.  For the kernel, I guess it's sufficient if
> > > GCC commits to this interpretation though.
> >
> > note that i'm not a language lawyer, merely an interpreter, so take
> > the above with a grain of salt.
> 
> Me neither, but actually I think this interpretation doesn't make sense.
> The implication seems to be that padding initialisation of initialised
> aggregates with automatic storage is guaranteed _only_ if the
> initialiser is incomplete.

however illogical it sounds, that's actually what is implied by the standard
and gcc behaves this way. my test case with a bit more verbose output than
your example (gcc shows in comments what each assignment corresponds to):

------ cut --------
gcc -O2 -x c -S -fverbose-asm -o - - <<EOF
struct s1 {
        char c;
        int l;
};

void f(struct s1*);

void g(void)
{
        struct s1 s1 = {};
        struct s1 s2 = { .c = 1};
        struct s1 s3 = { .l = 2};
        struct s1 s4 = { .c = 3, .l = 4};

        f(&s1);
        f(&s2);
        f(&s3);
        f(&s4);
}
EOF
------ cut --------

> This may or may not be a bug, and may or may not have been fixed in a
> more recent version.

i tested 4.5.4, 6.3 and 7.0 from about a month ago on amd64 and had
consistent results as above. i think it's not a bug but an intentional
optimization but i don't feel like digging it out from the gcc sources :).

> > both examples would be handled by the plugin if the types involved already
> > have a __user annotated field (as neither foo2 nor tv are initialized, only
> > assigned to), otherwise it'd need manual annotation and/or more static analysis.
> 
> Do you see any false positives here?  I would expect a fair number of
> structures that contain __user pointers but that are never copied
> wholesale to userspace, but I've not reviewed in detail.

false positive depends on what you consider a goal ;). for me the structleak
plugin had to serve one specific purpose (initialize a local variable that
would otherwise leak unintended kernel stack content back to userland), everything
else didn't matter. now if you want to move the goalpost and initialize those
and only those variables that get copied to userland but aren't otherwise
guaranteed to be completely initialized then you'll need something much smarter
than the current structleak plugin as it doesn't serve that purpose and will
both overinitialize and fail to initialize affected variables...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-17 19:25           ` PaX Team
@ 2017-01-17 22:04             ` Dave P Martin
  0 siblings, 0 replies; 17+ messages in thread
From: Dave P Martin @ 2017-01-17 22:04 UTC (permalink / raw)
  To: PaX Team
  Cc: Kees Cook, Mark Rutland, kernel-hardening, Emese Revfy, AKASHI,
	Takahiro, park jinbum, Daniel Micay, linux-kernel, spender

On Tue, Jan 17, 2017 at 08:25:49PM +0100, PaX Team wrote:
> On 17 Jan 2017 at 18:07, Dave P Martin wrote:
>
> > On Tue, Jan 17, 2017 at 06:09:49PM +0100, PaX Team wrote:
> > > On 17 Jan 2017 at 10:42, Dave P Martin wrote:
> > >
> > > > This can be read with the interpretation you suggest, but the wording
> > > > doesn't seem rock-solid.  For the kernel, I guess it's sufficient if
> > > > GCC commits to this interpretation though.
> > >
> > > note that i'm not a language lawyer, merely an interpreter, so take
> > > the above with a grain of salt.
> >
> > Me neither, but actually I think this interpretation doesn't make sense.
> > The implication seems to be that padding initialisation of initialised
> > aggregates with automatic storage is guaranteed _only_ if the
> > initialiser is incomplete.
>
> however illogical it sounds, that's actually what is implied by the standard
> and gcc behaves this way. my test case with a bit more verbose output than
> your example (gcc shows in comments what each assignment corresponds to):
>
> ------ cut --------
> gcc -O2 -x c -S -fverbose-asm -o - - <<EOF
> struct s1 {
>         char c;
>         int l;
> };
>
> void f(struct s1*);
>
> void g(void)
> {
>         struct s1 s1 = {};
>         struct s1 s2 = { .c = 1};
>         struct s1 s3 = { .l = 2};
>         struct s1 s4 = { .c = 3, .l = 4};
>
>         f(&s1);
>         f(&s2);
>         f(&s3);
>         f(&s4);
> }
> EOF
> ------ cut --------
>
> > This may or may not be a bug, and may or may not have been fixed in a
> > more recent version.
>
> i tested 4.5.4, 6.3 and 7.0 from about a month ago on amd64 and had
> consistent results as above. i think it's not a bug but an intentional
> optimization but i don't feel like digging it out from the gcc sources :).

Interesting.  So I guess we're probably stuck with it, good or bad.

Do you know the rationale behind the inconsistency?

> > > both examples would be handled by the plugin if the types involved already
> > > have a __user annotated field (as neither foo2 nor tv are initialized, only
> > > assigned to), otherwise it'd need manual annotation and/or more static analysis.
> >
> > Do you see any false positives here?  I would expect a fair number of
> > structures that contain __user pointers but that are never copied
> > wholesale to userspace, but I've not reviewed in detail.
>
> false positive depends on what you consider a goal ;). for me the structleak
> plugin had to serve one specific purpose (initialize a local variable that
> would otherwise leak unintended kernel stack content back to userland), everything
> else didn't matter. now if you want to move the goalpost and initialize those
> and only those variables that get copied to userland but aren't otherwise
> guaranteed to be completely initialized then you'll need something much smarter
> than the current structleak plugin as it doesn't serve that purpose and will
> both overinitialize and fail to initialize affected variables...

Sure -- the hope was that structleak might cover this sort of thing
without too much extra effort, but it sounds less realistic now.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] gcc-plugins: Add structleak for more stack initialization
  2017-01-17 18:54         ` PaX Team
@ 2017-01-18 10:48           ` Mark Rutland
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Rutland @ 2017-01-18 10:48 UTC (permalink / raw)
  To: PaX Team
  Cc: kernel-hardening, Kees Cook, Emese Revfy, AKASHI, Takahiro,
	park jinbum, Daniel Micay, linux-kernel, spender

On Tue, Jan 17, 2017 at 07:54:38PM +0100, PaX Team wrote:
> On 17 Jan 2017 at 17:48, Mark Rutland wrote:
> > That being the case, (and given the relevant bug has now been fixed),
> > it's not clear to me what the value of this is today. i.e. given the
> > general case, is this preventing many leaks?
> 
> no idea, i stopped looking at the instrumentation log long ago, but everyone
> can enable the debug output (has a very specific comment on it ;) and look at
> the results. i keep this plugin around because it costs nothing to maintain
> it and the alternative (better) solution doesn't exist yet.

Fair enough; understood.

> > > i never went into that direction because i think the security goal can
> > > be achieved without the performance impact of forced initialization.
> > 
> > Was there a particular technique you had in mind?
> 
> sure, i mentioned it in my SSTIC'12 keynote (page 36):
> https://pax.grsecurity.net/docs/PaXTeam-SSTIC12-keynote-20-years-of-PaX.pdf

Thanks for the pointer.

I'm probably being very naive here, but IIUC the per-task usercopy stack
would require roughly the same analysis to identify relevant variables,
unless all local variables (regardless of initialisation) that fed into
a usercopy would be on the usercopy stack?

Regardless, I can see the benefit of cleanly separating that data from
the rest of the kernel data.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2017-01-18 11:31 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-13 22:02 [PATCH] gcc-plugins: Add structleak for more stack initialization Kees Cook
2017-01-14 10:03 ` PaX Team
2017-01-16 15:24   ` Mark Rutland
2017-01-16 19:08     ` Daniel Micay
2017-01-16 19:30     ` PaX Team
2017-01-17 17:48       ` Mark Rutland
2017-01-17 18:54         ` PaX Team
2017-01-18 10:48           ` Mark Rutland
2017-01-17 17:48   ` Kees Cook
2017-01-16 11:54 ` Mark Rutland
2017-01-16 12:26   ` [kernel-hardening] " Mark Rutland
2017-01-16 19:22   ` PaX Team
2017-01-17 10:42     ` Dave P Martin
     [not found]       ` <587E4FDD.31940.D47F642@pageexec.freemail.hu>
2017-01-17 18:07         ` Dave P Martin
2017-01-17 19:25           ` PaX Team
2017-01-17 22:04             ` Dave P Martin
2017-01-17 17:56   ` Kees Cook

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).