[RFC 0/6] some compile- and run-time format checking

[RFC 0/6] some compile- and run-time format checking

[Rasmus Villemoes]

Consider these strictly RFC/POC.

I tried resurrecting my format_template plugin from two years ago, and
it rebased pretty cleanly. It also compiles with gcc 6.3, and has the
expected effect when one tries to trigger it, so it seems to work ok
(I think there was some build bot issue back then, maybe there still
is).

The last four patches are something I threw together rather quickly.
They compile and the few test cases pass, but I obviously need to find
some places to actually use fmtcheck() to see if it's worth adding.

Rasmus Villemoes (6):
  plugins: implement format_template attribute
  compiler.h: add __format_template
  compiler.h: add __attribute__((format_arg)) shorthand
  lib/vsprintf.c: add fmtcheck utility
  kernel.h: implement fmtmatch() wrapper around fmtcheck()
  lib/test_printf.c: add a few fmtcheck() test cases

 arch/Kconfig                                 |  18 ++
 drivers/hwmon/applesmc.c                     |   2 +-
 drivers/staging/speakup/spk_types.h          |   2 +-
 include/linux/compiler-gcc.h                 |   1 +
 include/linux/compiler.h                     |  10 +
 include/linux/kernel.h                       |  13 ++
 include/linux/smpboot.h                      |   2 +-
 include/linux/usb.h                          |   2 +-
 lib/test_printf.c                            |  41 ++++
 lib/vsprintf.c                               |  63 +++++
 scripts/Makefile.gcc-plugins                 |   2 +
 scripts/gcc-plugins/format_template_plugin.c | 331 +++++++++++++++++++++++++++
 12 files changed, 483 insertions(+), 4 deletions(-)
 create mode 100644 scripts/gcc-plugins/format_template_plugin.c

-- 
2.11.0

[RFC 1/6] plugins: implement format_template attribute

[Rasmus Villemoes]

Most format strings in the kernel are string literals, so the compiler
and other static analyzers can do type checking. This plugin covers a
few of the remaining cases, by introducing a format_template
attribute.

Consider struct usb_class_driver. Its member 'name' is used as a
format string in usb_register_dev(), and that use obviously expects
that the format string contains a single "%d" (or maybe %u). So the
idea is that we simply attach __format_template("%d") to the
declaration of the name member of struct usb_class_driver. We can then
check that any static initialization of that member is with a string
literal with the same set of specifiers. Moreover, we can use the
format template string to do type checking at the call site(s) in lieu
of a string literal.

For now, this only implements the former - mostly because I'm lazy and
don't want to write my own format checking code (again), and I suppose
there should be an internal gcc function I could (ab)use to say "check
this variadic function call, but use _this_ as format string".

Also, this only applies to struct members currently, but it should
also be possible to attach it to function parameters - e.g. the
namefmt parameter to kthread_create_on_cpu should have
__format_template("%u"); its only caller is __smpboot_create_thread
which passes struct smp_hotplug_thread->thread_comm, which in turn
should also have that attribute.

While strictly speaking "%*s" and "%d %s" both expect (int, const
char*), they're morally distinct, so I don't want to treat them as
equivalent. If this is ever a problem, I think one should let the
attribute take an optional flag argument, which could then control how
strict or lax the checking should be.

I'm not sure how much this affects compilation time, but there's not
really any point in building with this all the time - it should
suffice that the various build bots do it once in a while. Even
without the plugin, the __format_template(...) in the headers serves
as concise documentation.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 arch/Kconfig                                 |  18 ++
 scripts/Makefile.gcc-plugins                 |   2 +
 scripts/gcc-plugins/format_template_plugin.c | 331 +++++++++++++++++++++++++++
 3 files changed, 351 insertions(+)
 create mode 100644 scripts/gcc-plugins/format_template_plugin.c

diff --git a/arch/Kconfig b/arch/Kconfig
index 057370a0ac4e..71c582eaeb69 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -517,6 +517,24 @@ config GCC_PLUGIN_RANDSTRUCT_PERFORMANCE
 	  in structures.  This reduces the performance hit of RANDSTRUCT
 	  at the cost of weakened randomization.
 
+config GCC_PLUGIN_FORMAT_TEMPLATE
+	bool "Enable format_template attribute"
+	depends on GCC_PLUGINS
+	help
+	  This plugin implements a format_template attribute which can
+	  be attached to struct members which are supposed to hold a
+	  (printf) format string. This allows the compiler to check
+	  that (a) any string statically assigned to such a struct
+	  member has format specifiers compatible with those in the
+	  template and (b) when such a struct member is used as the
+	  format argument to a printf function, use the template in
+	  lieu of a string literal to do type checking of the variadic
+	  arguments.
+
+	  Even without using the plugin, attaching the format_template
+	  attribute can be beneficial, since it serves as
+	  documentation.
+
 config HAVE_CC_STACKPROTECTOR
 	bool
 	help
diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index b2a95af7df18..2f9bc96aab90 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -35,6 +35,8 @@ ifdef CONFIG_GCC_PLUGINS
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT)	+= -DRANDSTRUCT_PLUGIN
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_RANDSTRUCT_PERFORMANCE)	+= -fplugin-arg-randomize_layout_plugin-performance-mode
 
+  gcc-plugin-$(CONFIG_GCC_PLUGIN_FORMAT_TEMPLATE)	+= format_template_plugin.so
+
   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/format_template_plugin.c b/scripts/gcc-plugins/format_template_plugin.c
new file mode 100644
index 000000000000..09a798773cfd
--- /dev/null
+++ b/scripts/gcc-plugins/format_template_plugin.c
@@ -0,0 +1,331 @@
+#include <string.h>
+#include <assert.h>
+
+#include "gcc-common.h"
+#include "c-family/c-pragma.h"
+
+int plugin_is_GPL_compatible;
+
+static struct plugin_info format_template_plugin_info = {
+	.version	= "20151209",
+	.help		= "format_template_plugin\n",
+};
+
+static tree handle_format_template_attribute(tree *node, tree name, tree args, int __unused flags, bool *no_add_attrs)
+{
+	tree tmpl, orig_attr;
+
+	*no_add_attrs = true;
+	switch (TREE_CODE(*node)) {
+	case FIELD_DECL:
+		break;
+	default:
+		error("%qE attribute only applies to struct members", name);
+		return NULL_TREE;
+	}
+
+	tmpl = TREE_VALUE(args);
+	if (TREE_CODE(tmpl) != STRING_CST) {
+		error("%qE parameter of the %qE attribute is not a string constant", args, name);
+		return NULL_TREE;
+	}
+
+	orig_attr = lookup_attribute("format_template", DECL_ATTRIBUTES(*node));
+	if (orig_attr) {
+		error("%qE attribute applied twice", name);
+		return NULL_TREE;
+	}
+	else
+		*no_add_attrs = false;
+
+	return NULL_TREE;
+}
+
+static struct attribute_spec format_template_attr = {
+	.name				= "format_template",
+	.min_length			= 1,
+	.max_length			= 1,
+	.decl_required			= true,
+	.type_required			= false,
+	.function_type_required		= false,
+	.handler			= handle_format_template_attribute,
+#if BUILDING_GCC_VERSION >= 4007
+	.affects_type_identity		= false
+#endif
+};
+
+static void register_attributes(void __unused *event_data, void __unused *data)
+{
+	register_attribute(&format_template_attr);
+}
+
+static void define_feature_macro(void __unused *event_data, void __unused *data)
+{
+	cpp_define(parse_in, "HAVE_ATTRIBUTE_FORMAT_TEMPLATE");
+}
+
+enum {
+	QUAL_NONE,
+	QUAL_SHORT, /* h */
+	QUAL_BYTE, /* hh, == QUAL_SHORT+1*/
+	QUAL_LONG, /* l */
+	QUAL_LONGLONG, /* ll, == QUAL_LONG+1 */
+	QUAL_MAX, /* j */
+	QUAL_SIZE, /* z */
+	QUAL_PTRDIFF, /* t */
+};
+#define FW_P_NONE (-1)
+#define FW_P_ARG (-2)
+
+struct spec_iter {
+	const char *spec;
+	int len;
+
+	int field_width;
+	int precision;
+	int qual;
+	char type;
+
+};
+
+static inline void
+spec_iter_init(struct spec_iter *spec, const char *s)
+{
+	spec->spec = s;
+	spec->len = 0;
+}
+
+static void get_fw_p(const char **c, int *dst, int prec)
+{
+	*dst = FW_P_NONE;
+	if (prec) {
+		if (**c != '.')
+			return;
+		++(*c);
+		*dst = 0;
+	}
+	if (**c == '*') {
+		++(*c);
+		*dst = FW_P_ARG;
+		return;
+	}
+	if (!ISDIGIT(**c))
+		return;
+	*dst = **c - '0';
+	++(*c);
+	while (ISDIGIT(**c)) {
+		/* should do if (*dst > 10000) warn("insane explicit field width/precision"); */
+		*dst *= 10;
+		*dst += **c - '0';
+		++(*c);
+	}
+}
+
+static int spec_next(struct spec_iter *spec)
+{
+	const char *c;
+	int slen = 0;
+
+	spec->spec += spec->len;
+again:
+	c = strchrnul(spec->spec, '%');
+	slen += c - spec->spec;
+	if (!c[0]) {
+		spec->spec = NULL;
+		return slen;
+	}
+	assert(c[0] == '%');
+	if (c[1] == '%') {
+		slen++;
+		spec->spec = c+2;
+		goto again;
+	}
+
+	spec->spec = c;
+	++c;
+	/* skip flags */
+	while (strchr("#0- +", *c))
+		++c;
+
+	get_fw_p(&c, &spec->field_width, 0);
+	get_fw_p(&c, &spec->precision, 1);
+
+	spec->qual = QUAL_NONE;
+	switch (*c) {
+	case 'h': spec->qual = QUAL_SHORT; break;
+	case 'l': spec->qual = QUAL_LONG; break;
+#if 0 /* The kernel doesn't grok the j qualifier */
+	case 'j': spec->qual = QUAL_MAX; break;
+#endif
+	case 'z': spec->qual = QUAL_SIZE; break;
+	case 't': spec->qual = QUAL_PTRDIFF; break;
+	}
+	if (spec->qual) {
+		++c;
+		if (*c == c[-1] && (*c == 'h' || *c == 'l')) {
+			spec->qual++;
+			++c;
+		}
+	}
+
+	spec->type = *c++;
+	spec->len = c - spec->spec;
+
+	switch (spec->type) {
+	case 'd':
+	case 'u':
+	case 'x':
+	case 'X':
+	case 'o':
+	case 'c':
+	case 's':
+		break;
+	/*
+	 * Disallowing %p is the safe and sane thing to do, given the
+	 * different interpretations based on following alphanumerics.
+	 */
+	case 'p':
+	/*
+	 * Why are there two with the same meaning? %i is the lesser
+	 * used one and should just die.
+	 */
+	case 'i':
+		error("unsupported specifier '%.*s' in template or initializer", spec->len, spec->spec);
+	default:
+		error("invalid specifier '%.*s' in template or initializer", spec->len, spec->spec);
+	}
+
+	return slen;
+}
+
+static bool specs_compatible(const struct spec_iter *a, const struct spec_iter *b)
+{
+	if (a->qual != b->qual)
+		return false;
+	if (a->field_width != b->field_width)
+		return false;
+	if (a->precision != b->precision)
+		return false;
+	if (a->type != b->type)
+		return false;
+	return true;
+}
+
+static void check_literal(tree attr, const char *str)
+{
+	const char *pattern = TREE_STRING_POINTER(TREE_VALUE(TREE_VALUE(attr)));
+	struct spec_iter sp, ss;
+	int i;
+
+	spec_iter_init(&sp, pattern);
+	spec_iter_init(&ss, str);
+
+	/*
+	 * Walk over the pattern and string in lockstep.
+	 */
+	for (i = 1; ; ++i) {
+		spec_next(&sp);
+		spec_next(&ss);
+		/*
+		 * It's ok for the template to have more specifiers
+		 * than the actual string. But issue warning(s)
+		 * anyway, conditional on -Wformat-extra-args.
+		 */
+		if (ss.spec == NULL) {
+			while (sp.spec != NULL) {
+			       warning(OPT_Wformat_extra_args,
+				       "format template '%s' contains extra specifier '%.*s' compared to initializer string '%s'",
+				       pattern, sp.len, sp.spec, str);
+			       spec_next(&sp);
+			}
+			return;
+		}
+		/*
+		 * It's absolutely not ok for the actual string to
+		 * have more specifiers than the template.
+		 */
+		if (!sp.spec) {
+			do {
+				error("initializer string '%s' contains extra format specifier '%.*s' compared to format template '%s'",
+					str, ss.len, ss.spec, pattern);
+				spec_next(&ss);
+			} while (ss.spec != NULL);
+			return;
+		}
+		if (!specs_compatible(&ss, &sp)) {
+			error("specifier %d in '%s' ('%.*s') incompatible with format template '%s'",
+			      i, str, ss.len, ss.spec, pattern);
+		}
+	}
+}
+
+static void check_declaration(void *event_data, void *data __unused)
+{
+	tree decl = (tree)event_data;
+	tree ini, type;
+	unsigned idx;
+	tree field, value;
+
+	switch (TREE_CODE(decl)) {
+	case VAR_DECL:
+		break;
+	default:
+		return;
+	}
+
+	ini = DECL_INITIAL(decl);
+	if (!ini)
+		return;
+
+	type = TREE_TYPE(decl);
+	if (TREE_CODE(type) != RECORD_TYPE)
+		return;
+
+	if (TREE_CODE(ini) != CONSTRUCTOR) {
+		// warning(0, "weird, initializer is not a CONSTRUCTOR, tree_code=%d", TREE_CODE(ini));
+		return;
+	}
+
+	FOR_EACH_CONSTRUCTOR_ELT(CONSTRUCTOR_ELTS(ini), idx, field, value) {
+		tree attr;
+
+		/* if (TREE_CODE(value) != STRING_CST) */
+		/* 	continue; */
+
+		attr = lookup_attribute("format_template", DECL_ATTRIBUTES(field));
+		if (!attr)
+			continue;
+
+		/*
+		 * Hm, apparently the string literal is hidden behind
+		 * a NOP_EXPR and a ADDR_EXPR.
+		 */
+		STRIP_NOPS(value);
+		if (TREE_CODE(value) == ADDR_EXPR)
+			value = TREE_OPERAND(value, 0);
+
+		if (TREE_CODE(value) != STRING_CST)
+			continue;
+
+		check_literal(attr, TREE_STRING_POINTER(value));
+
+	}
+
+}
+
+int plugin_init(struct plugin_name_args *plugin_info, struct plugin_gcc_version *version)
+{
+	const char *const plugin_name = plugin_info->base_name;
+
+	if (!plugin_default_version_check(version, &gcc_version)) {
+		error(G_("incompatible gcc/plugin versions"));
+		return 1;
+	}
+
+	register_callback(plugin_name, PLUGIN_INFO, NULL, &format_template_plugin_info);
+	register_callback(plugin_name, PLUGIN_START_UNIT, &define_feature_macro, NULL);
+	register_callback(plugin_name, PLUGIN_FINISH_DECL, &check_declaration, NULL);
+	register_callback(plugin_name, PLUGIN_ATTRIBUTES, &register_attributes, NULL);
+
+	return 0;
+}
-- 
2.11.0

[RFC 2/6] compiler.h: add __format_template

[Rasmus Villemoes]

Most format strings in the kernel are explicit string literals at the
use site and can thus be checked by gcc and other static
checkers. There are cases, however, where the format string comes from
some struct member and is implicitly assumed to contain certain
specifiers.

A gcc plugin provides the attribute format_template for that case. One
attaches the attribute to the struct member in question, providing a
template consisting of the expected format specifiers. The plugin then
checks that all static initializations of that struct member is
consistent with the template; moreover, when the format string in a
printf-like function call comes from such an annotated struct member,
one cannot do static analysis of the actual runtime string, but one
can check that the template is consistent with the actual arguments.

Apply the attribute to a few struct members:

struct smp_hotplug_thread::thread_comm
struct usb_class_driver::name
struct applesmc_node_group::format
struct num_var_t::synth_fmt

Modifying kernel/softirq.c slightly, this is what one gets if one
violates the template:

kernel/softirq.c:751:1: error: initializer string 'ksoftirqd/%u+%d' contains extra format specifier '%d' compared to format template 'foobar/%u'

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/hwmon/applesmc.c            | 2 +-
 drivers/staging/speakup/spk_types.h | 2 +-
 include/linux/compiler.h            | 6 ++++++
 include/linux/smpboot.h             | 2 +-
 include/linux/usb.h                 | 2 +-
 5 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 5c677ba44014..9e6d23cba23e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -110,7 +110,7 @@ struct applesmc_dev_attr {
 
 /* Dynamic device node group */
 struct applesmc_node_group {
-	char *format;				/* format string */
+	char *format __format_template("%d");	/* format string */
 	void *show;				/* show function */
 	void *store;				/* store function */
 	int option;				/* function argument */
diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index c50de6035a9a..156000c0c4d3 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -108,7 +108,7 @@ struct st_var_header {
 };
 
 struct num_var_t {
-	char *synth_fmt;
+	char *synth_fmt __format_template("%d");
 	int default_val;
 	int low;
 	int high;
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 202710420d6d..478914ad280b 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -625,4 +625,10 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 	(_________p1); \
 })
 
+#ifdef HAVE_ATTRIBUTE_FORMAT_TEMPLATE
+#define __format_template(x) __attribute__((__format_template__(x)))
+#else
+#define __format_template(x)
+#endif
+
 #endif /* __LINUX_COMPILER_H */
diff --git a/include/linux/smpboot.h b/include/linux/smpboot.h
index c174844cf663..967285b9637d 100644
--- a/include/linux/smpboot.h
+++ b/include/linux/smpboot.h
@@ -42,7 +42,7 @@ struct smp_hotplug_thread {
 	void				(*unpark)(unsigned int cpu);
 	cpumask_var_t			cpumask;
 	bool				selfparking;
-	const char			*thread_comm;
+	const char			*thread_comm __format_template("foobar/%u");
 };
 
 int smpboot_register_percpu_thread_cpumask(struct smp_hotplug_thread *plug_thread,
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9c63792a8134..8d48c0cd1c01 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1236,7 +1236,7 @@ extern struct bus_type usb_bus_type;
  * parameters used for them.
  */
 struct usb_class_driver {
-	char *name;
+	char *name __format_template("foobar_%d");
 	char *(*devnode)(struct device *dev, umode_t *mode);
 	const struct file_operations *fops;
 	int minor_base;
-- 
2.11.0

[RFC 3/6] compiler.h: add __attribute__((format_arg)) shorthand

[Rasmus Villemoes]

The __format_arg attribute tells gcc that it can use a specific
argument to the annotated function as the format string for the
purpose of type-checking a surrounding __printf function call, e.g.

  sprintf(buf, fmtcheck(what->ever, "%d %lx", 0), i, m)

makes gcc check that i and m are indeed an int and a long; with

  sprintf(buf, what->ever, i, m)

the compiler cannot do any type checking.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/compiler-gcc.h | 1 +
 include/linux/compiler.h     | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index bb78e5bdff26..69a233f9fa26 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -128,6 +128,7 @@
 #define __maybe_unused		__attribute__((unused))
 #define __always_unused		__attribute__((unused))
 #define __mode(x)               __attribute__((mode(x)))
+#define __format_arg(n)         __attribute__((format_arg(n)))
 
 /* gcc version specific checks */
 
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 478914ad280b..2c8793f72fb0 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -631,4 +631,8 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
 #define __format_template(x)
 #endif
 
+#ifndef __format_arg
+#define __format_arg(n)
+#endif
+
 #endif /* __LINUX_COMPILER_H */
-- 
2.11.0

[RFC 4/6] lib/vsprintf.c: add fmtcheck utility

[Rasmus Villemoes]

We have a few places in the kernel where a *printf function is used with
a non-constant format string, making the ordinary static type checking
done by gcc et al. impossible. Some things can still be caught at build
time with appropriate instrumentation (I'm sure one can do much better
than the format_template plugin), but that still leaves a number of
places unchecked. So this patch adds a function for doing run-time
verification of a given format string against a template.

The fmtcheck() function takes two format string arguments and checks
whether they contain the same printf specifiers. If they do, the
first (the string-to-be-checked) string is returned. If not, the
second (the template) is returned. Regardless of which string is
returned at run-time, the __format_arg attribute allows the compiler to
do type-checking if the fmtcheck() function is used inside a *printf
call, e.g.

  sprintf(buf, fmtcheck(what->ever, "%d %lx", 0), i, m)

We actually make fmtcheck() a macro that tries very hard to ensure the
template argument is a string literal - partly to help avoid mixing up
the two "const char*" arguments, partly because much of the point of
this sanity checking vanishes if the template is not a literal (e.g.,
the __format_arg annotation becomes useless).

We don't treat "%*.*s" and "%d %d %s" as equivalent, despite them
taking the same vararg types, since they're morally very distinct. In
fact, at least for now, we don't even treat "%d" and "%u" as
equivalent. We can relax that, possibly via FMTCHECK_* flags, but let's
first see which users there might be and what they'd want.

If either string contains a %p, we really should check the following
alphanumerics to see which (if any) extension is used and check that
they match as well. For now, just complain loudly, partly because I'm
lazy, partly because I don't know any in-tree code that might use
fmtcheck() with a %p in the template, and I can't really imagine
anyone would use a %pXX extension in a non-constant format string.

I don't know if WARN is too violent; maybe just pr_warn would be ok.

The BSDs (and libbsd on linux) contain a fmtcheck() function; I took the
name and return semantics from that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kernel.h |  6 +++++
 lib/vsprintf.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4b484ab9e163..d7c6f9a9c024 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -460,6 +460,12 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
 extern __printf(2, 0)
 const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
+extern __format_arg(2) __attribute_const__
+const char *_fmtcheck(const char *fmt, const char *tmpl, unsigned flags);
+#define fmtcheck(fmt, tmpl, flags) _fmtcheck(fmt, "" tmpl "", flags)
+#define FMTCHECK_SILENT        0x01
+#define FMTCHECK_NO_EXTRA_ARGS 0x02
+
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..db50acf682e7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3030,3 +3030,66 @@ int sscanf(const char *buf, const char *fmt, ...)
 	return i;
 }
 EXPORT_SYMBOL(sscanf);
+
+static int
+next_interesting_spec(const char **s, struct printf_spec *spec)
+{
+	int len;
+
+	while (1) {
+		len = format_decode(*s, spec);
+		if (!len)
+			return 0;
+		*s += len;
+		if (spec->type == FORMAT_TYPE_NONE ||
+		    spec->type == FORMAT_TYPE_PERCENT_CHAR)
+			continue;
+		return len;
+	}
+}
+
+const char *
+_fmtcheck(const char *fmt, const char *tmpl, unsigned flags)
+{
+	const char *f = fmt;
+	const char *t = tmpl;
+	struct printf_spec fspec = {0}, tspec = {0};
+	int flen, tlen;
+	int warn = !(flags & FMTCHECK_SILENT);
+
+	while (1) {
+		flen = next_interesting_spec(&f, &fspec);
+		tlen = next_interesting_spec(&t, &tspec);
+		if (!flen) {
+			/*
+			 * The given format string doesn't have any
+			 * more specifiers. It's ok from a type-safety
+			 * POV for the template to have extra, but
+			 * optionally warn about it (e.g., a single %d
+			 * may be required).
+			 */
+			if (tlen && (flags & FMTCHECK_NO_EXTRA_ARGS) && warn)
+				WARN_ONCE(warn, "template '%s' expects more arguments than '%s'\n",
+					tmpl, fmt);
+			return fmt;
+		}
+		if (!tlen) {
+			WARN_ONCE(warn, "format string '%s' expects more arguments than template '%s'",
+				  fmt, tmpl);
+			return tmpl;
+		}
+		WARN_ONCE(warn && (fspec.type == FORMAT_TYPE_PTR || tspec.type == FORMAT_TYPE_PTR),
+			  "don't use %%p in non-constant format strings");
+		/*
+		 * Should we also care about flags, field width,
+		 * precision? Should we even care about base?
+		 */
+		if (fspec.type != tspec.type ||
+		    fspec.base != tspec.base) {
+			WARN_ONCE(warn, "format string '%s' incompatible with template '%s'",
+				  fmt, tmpl);
+			return tmpl;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(_fmtcheck);
-- 
2.11.0

[RFC 5/6] kernel.h: implement fmtmatch() wrapper around fmtcheck()

[Rasmus Villemoes]

I couldn't come up with a better name, suggestions welcome.

Some users may prefer to EINVAL rather than using the the template as a
fallback for printf'ing. fmtmatch() is simply a shorthand for
fmtcheck(a, b, c) == a.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kernel.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d7c6f9a9c024..5eae5ecb590d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -466,6 +466,13 @@ const char *_fmtcheck(const char *fmt, const char *tmpl, unsigned flags);
 #define FMTCHECK_SILENT        0x01
 #define FMTCHECK_NO_EXTRA_ARGS 0x02
 
+#define fmtmatch(fmt, tmpl, flags) _fmtmatch(fmt, "" tmpl "", flags)
+static inline bool
+_fmtmatch(const char *fmt, const char *tmpl, unsigned flags)
+{
+	return _fmtcheck(fmt, tmpl, flags) == fmt;
+}
+
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
-- 
2.11.0

[RFC 6/6] lib/test_printf.c: add a few fmtcheck() test cases

[Rasmus Villemoes]

It should be trivial to add more test cases, once we figure out the
exact rules for being compatible or not. Perhaps we'll have to extend
the struct test with a flags element if we add flags that affect the
return value.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 563f10e6876a..0c8490f3a9b3 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -483,6 +483,45 @@ test_pointer(void)
 	flags();
 }
 
+static void __init
+test_fmtcheck(void)
+{
+	struct test { const char *fmt; const char *tmpl; };
+	static const struct test compatible[] __initconst = {
+		{"", ""},
+		{"wlan%d", "%d"},
+		{"aa%llxbb", "cc%Lxdd%%"},
+	};
+	static const struct test incompatible[] __initconst = {
+		{"a %d b %lx", "%d %x"},
+		{"%llo", "%Lx"},
+	};
+	unsigned i;
+	const struct test *t;
+	const char *ret;
+
+	for (i = 0; i < ARRAY_SIZE(compatible); ++i) {
+		total_tests++;
+		t = &compatible[i];
+		ret = _fmtcheck(t->fmt, t->tmpl, FMTCHECK_SILENT);
+		if (ret != t->fmt) {
+			failed_tests++;
+			pr_warn("'%s' and '%s' deemed incompatible by fmtcheck()",
+				t->fmt, t->tmpl);
+		}
+	}
+	for (i = 0; i < ARRAY_SIZE(incompatible); ++i) {
+		total_tests++;
+		t = &incompatible[i];
+		ret = _fmtcheck(t->fmt, t->tmpl, FMTCHECK_SILENT);
+		if (ret != t->tmpl) {
+			failed_tests++;
+			pr_warn("'%s' and '%s' deemed compatible by fmtcheck()",
+				t->fmt, t->tmpl);
+		}
+	}
+}
+
 static int __init
 test_printf_init(void)
 {
@@ -496,6 +535,8 @@ test_printf_init(void)
 	test_string();
 	test_pointer();
 
+	test_fmtcheck();
+
 	kfree(alloced_buffer);
 
 	if (failed_tests == 0)
-- 
2.11.0

Re: [RFC 4/6] lib/vsprintf.c: add fmtcheck utility

[Kees Cook]

On Wed, Nov 8, 2017 at 2:30 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> We have a few places in the kernel where a *printf function is used with
> a non-constant format string, making the ordinary static type checking
> done by gcc et al. impossible. Some things can still be caught at build
> time with appropriate instrumentation (I'm sure one can do much better
> than the format_template plugin), but that still leaves a number of
> places unchecked. So this patch adds a function for doing run-time
> verification of a given format string against a template.
>
> The fmtcheck() function takes two format string arguments and checks
> whether they contain the same printf specifiers. If they do, the
> first (the string-to-be-checked) string is returned. If not, the
> second (the template) is returned. Regardless of which string is
> returned at run-time, the __format_arg attribute allows the compiler to
> do type-checking if the fmtcheck() function is used inside a *printf
> call, e.g.
>
>   sprintf(buf, fmtcheck(what->ever, "%d %lx", 0), i, m)

Cool, I like this. I wonder if there are any "hot paths" that would
actually make this runtime checking expensive? Seems like anything
that hot shouldn't be using sprintf anyway...

>
> We actually make fmtcheck() a macro that tries very hard to ensure the
> template argument is a string literal - partly to help avoid mixing up
> the two "const char*" arguments, partly because much of the point of
> this sanity checking vanishes if the template is not a literal (e.g.,
> the __format_arg annotation becomes useless).

I wonder how much work it would be to instrument vsnprintf() to warn
about all non-const format strings that are being processed so we
could find all the places where fmtcheck() (and the struct annotation)
are needed.

> We don't treat "%*.*s" and "%d %d %s" as equivalent, despite them
> taking the same vararg types, since they're morally very distinct. In
> fact, at least for now, we don't even treat "%d" and "%u" as
> equivalent. We can relax that, possibly via FMTCHECK_* flags, but let's
> first see which users there might be and what they'd want.
>
> If either string contains a %p, we really should check the following
> alphanumerics to see which (if any) extension is used and check that
> they match as well. For now, just complain loudly, partly because I'm
> lazy, partly because I don't know any in-tree code that might use
> fmtcheck() with a %p in the template, and I can't really imagine
> anyone would use a %pXX extension in a non-constant format string.

Yeah, seems reasonable for the first pass at this.

> I don't know if WARN is too violent; maybe just pr_warn would be ok.

I think WARN gets noticed much more by build and runtime testing
tools, so I think that's the right thing to do here. A mismatch really
should be noticed.

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC 0/6] some compile- and run-time format checking

[Kees Cook]

On Wed, Nov 8, 2017 at 2:30 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Consider these strictly RFC/POC.
>
> I tried resurrecting my format_template plugin from two years ago, and
> it rebased pretty cleanly. It also compiles with gcc 6.3, and has the
> expected effect when one tries to trigger it, so it seems to work ok
> (I think there was some build bot issue back then, maybe there still
> is).
>
> The last four patches are something I threw together rather quickly.
> They compile and the few test cases pass, but I obviously need to find
> some places to actually use fmtcheck() to see if it's worth adding.
>
> Rasmus Villemoes (6):
>   plugins: implement format_template attribute
>   compiler.h: add __format_template

Could you split these two off and send separately? This seems like a
fine thing to get in now. Probably the second patch should be split up
between adding __format_template, and additions of its usage. Do you
have any good ways to find and extract all the dynamic format strings
we need to mark?

>   compiler.h: add __attribute__((format_arg)) shorthand
>   lib/vsprintf.c: add fmtcheck utility
>   kernel.h: implement fmtmatch() wrapper around fmtcheck()
>   lib/test_printf.c: add a few fmtcheck() test cases

I like this approach a lot. Combined with checking if a format string
is in read-only memory, this could really close the door on format
string exposures.

Thanks!

-Kees

>
>  arch/Kconfig                                 |  18 ++
>  drivers/hwmon/applesmc.c                     |   2 +-
>  drivers/staging/speakup/spk_types.h          |   2 +-
>  include/linux/compiler-gcc.h                 |   1 +
>  include/linux/compiler.h                     |  10 +
>  include/linux/kernel.h                       |  13 ++
>  include/linux/smpboot.h                      |   2 +-
>  include/linux/usb.h                          |   2 +-
>  lib/test_printf.c                            |  41 ++++
>  lib/vsprintf.c                               |  63 +++++
>  scripts/Makefile.gcc-plugins                 |   2 +
>  scripts/gcc-plugins/format_template_plugin.c | 331 +++++++++++++++++++++++++++
>  12 files changed, 483 insertions(+), 4 deletions(-)
>  create mode 100644 scripts/gcc-plugins/format_template_plugin.c
>
> --
> 2.11.0
>



-- 
Kees Cook
Pixel Security

Re: [RFC 0/6] some compile- and run-time format checking

[Rasmus Villemoes]

On 9 November 2017 at 02:11, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Nov 8, 2017 at 2:30 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> Rasmus Villemoes (6):
>>   plugins: implement format_template attribute
>>   compiler.h: add __format_template
>
> Could you split these two off and send separately? This seems like a
> fine thing to get in now.

Will do.

> Probably the second patch should be split up
> between adding __format_template, and additions of its usage.

Yeah.

> Do you have any good ways to find and extract all the dynamic format strings
> we need to mark?

IIRC, I just did a git grep for designated initializers where the RHS
was a string literal containing a % char. Not sure that counts as a
good way :)

Doing that now finds stuff like
drivers/scsi/hisi_sas/hisi_sas_v2_hw.c, where the .msg member cannot
be annotated with a single template. Maybe one can work around that by
replacing .msg with an anon union of msg_onebit/msg_multibit, with
each their own template; I don't know if that will work, or if it will
be deemed too much churn (it doesn't provide that much safety, since
it would then just rely on accessing the right union member). Maybe
the run-time checking is best for that case.

Rasmus

[RFC PATCH 0/7] runtime format string checking

[Rasmus Villemoes]

This is a resurrection of something I sent out about a year ago. In
the relatively few places where we use a non-literal as format string,
we can annotate the source with a fmtcheck() call that will (a) at
build time, allow the compiler to check the variadic arguments against
the template, and (b) at runtime, check that the format specifiers
present in the actual format string match those in the template (and
if not, WARN and use the template to ensure runtime type safety).

Finding places to annotate is just

  make -j8 KCFLAGS='-Wformat-nonliteral'

So far, in about half the places I looked, one might as well get
completely rid of the non-literal format string. Patches 5,6,7 are
some examples of where one might add fmtcheck() calls. I don't think
we can get to a state where we can unconditionally add
-Wformat-nonliteral to the build, but I think there's a lot of
low-hanging fruit.

This is on top of Miguel's compiler attributes series [1], which I
hope will land in mainline soon.

[1] https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-4.20-rc1

Rasmus Villemoes (7):
  compiler_attributes.h: add __attribute__((format_arg)) shorthand
  lib/vsprintf.c: add fmtcheck utility
  kernel.h: implement fmtmatch() wrapper around fmtcheck()
  lib/test_printf.c: add a few fmtcheck() test cases
  kernel/kthread.c: do runtime check of format string in
    kthread_create_on_cpu()
  nfs: use fmtcheck() in root_nfs_data
  drivers: hwmon: add runtime format string checking

 drivers/hwmon/hwmon.c               |  3 +-
 fs/nfs/nfsroot.c                    |  2 +-
 include/linux/compiler_attributes.h | 13 ++++++
 include/linux/kernel.h              | 25 +++++++++++
 kernel/kthread.c                    |  4 +-
 lib/Kconfig.debug                   |  9 ++++
 lib/test_printf.c                   | 43 +++++++++++++++++++
 lib/vsprintf.c                      | 65 +++++++++++++++++++++++++++++
 8 files changed, 160 insertions(+), 4 deletions(-)

-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Rasmus Villemoes]

The __format_arg attribute tells gcc that it can use a specific
argument to the annotated function as the format string for the
purpose of type-checking a surrounding __printf function call. For
example, assuming one has a fmtcheck function declared as

  const char *fmtcheck(const char *, const char *, unsigned) __format_arg(2);

and this is used in

  sprintf(buf, fmtcheck(what->ever, "%d %lx", 0), i, m)

gcc checks that the varargs (i and m) matches the second argument to the
fmtcheck function, i.e. that they are (int, long). With

  sprintf(buf, what->ever, i, m)

the compiler cannot do any type checking.

Even a static inline fmtcheck() that just returns its first argument
would provide documentation for which specifiers what->ever is supposed
to contain, but we'll implement an actual run-time check later.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/compiler_attributes.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 6b28c1b7310c..08264df52322 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -32,6 +32,7 @@
 # define __GCC4_has_attribute___assume_aligned__      (__GNUC_MINOR__ >= 9)
 # define __GCC4_has_attribute___designated_init__     0
 # define __GCC4_has_attribute___externally_visible__  1
+# define __GCC4_has_attribute___format_arg__          1
 # define __GCC4_has_attribute___noclone__             1
 # define __GCC4_has_attribute___optimize__            1
 # define __GCC4_has_attribute___nonstring__           0
@@ -140,6 +141,18 @@
 #define __printf(a, b)                  __attribute__((__format__(printf, a, b)))
 #define __scanf(a, b)                   __attribute__((__format__(scanf, a, b)))
 
+/*
+ * Optional
+ *
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format_005farg-function-attribute
+ * clang: apparently supported, but undocumented
+ */
+#if __has_attribute(__format_arg__)
+# define __format_arg(n) __attribute__((__format_arg__(n)))
+#else
+# define __format_arg(n)
+#endif
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-gnu_005finline-function-attribute
  * clang: https://clang.llvm.org/docs/AttributeReference.html#gnu-inline
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 2/7] lib/vsprintf.c: add fmtcheck utility

[Rasmus Villemoes]

We have a few places in the kernel where a *printf function is used with
a non-constant format string, making the ordinary static type checking
done by gcc et al. impossible. With extra instrumentation, some things
can still be caught at build time, but that still leaves a number of
places unchecked. So this patch adds a function for doing run-time
verification of a given format string against a template.

The fmtcheck() function takes two format string arguments and checks
whether they contain the same printf specifiers. If they do, the
first (the string-to-be-checked) string is returned. If not, the
second (the template) is returned - the resulting formatted string is
likely garbage, but this should still be better than using arguments of
the wrong type.

Regardless of which string is returned at run-time, the __format_arg
attribute allows the compiler to do type-checking if the fmtcheck()
function is used inside a *printf call, e.g.

  sprintf(buf, fmtcheck(what->ever, "%d %lx", 0), i, m)

This also serves as documentation for whoever creates the string found
at what->ever that it should contain these two specifiers.

We actually make fmtcheck() a macro that tries very hard to ensure the
template argument is a string literal - partly to help avoid mixing up
the two "const char*" arguments, partly because much of the point of
this sanity checking vanishes if the template is not a literal (e.g.,
the __format_arg annotation becomes useless).

We don't treat "%*.*s" and "%d %d %s" as equivalent, despite them
taking the same vararg types, since they're morally very distinct. In
fact, at least for now, we don't even treat "%d" and "%u" as
equivalent. We can relax that, possibly via FMTCHECK_* flags, but let's
first see which users there might be and what they'd want.

If either string contains a %p, we really should check the following
alphanumerics to see which (if any) extension is used and check that
they match as well. For now, just complain loudly, partly because I'm
lazy, partly because I don't know any in-tree code that might use
fmtcheck() with a %p in the template, and I can't really imagine
anyone would use a %pXX extension in a non-constant format string.

I'm making this optional, but default y, since I don't suppose
fmtcheck() will ever appear in a hot path.

The BSDs (and libbsd on linux) contain a fmtcheck() function; I took the
name and return semantics from that.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kernel.h | 18 ++++++++++++
 lib/Kconfig.debug      |  9 ++++++
 lib/vsprintf.c         | 65 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d6aac75b51ba..8e9154e100c3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -495,6 +495,24 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
 extern __printf(2, 0)
 const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
+#define FMTCHECK_SILENT        0x01
+#define FMTCHECK_NO_EXTRA_ARGS 0x02
+#ifdef CONFIG_FMTCHECK
+__format_arg(2)
+const char *_fmtcheck(const char *fmt, const char *tmpl, unsigned flags);
+#else
+static inline __format_arg(2) const char *
+_fmtcheck(const char *fmt, const char *tmpl, unsigned flags)
+{
+	return fmt;
+}
+#endif
+/*
+ * Use of fmtcheck is pointless if the template is not a string
+ * literal, so try to enforce that.
+ */
+#define fmtcheck(fmt, tmpl, flags) _fmtcheck(fmt, "" tmpl "", flags)
+
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4966c4fbe7f7..adfd431c6876 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1037,6 +1037,15 @@ config DEBUG_PREEMPT
 	  if kernel code uses it in a preemption-unsafe way. Also, the kernel
 	  will detect preemption count underflows.
 
+config FMTCHECK
+	bool "Runtime format string checking"
+	default y
+	help
+	  If you say Y here, the kernel performs runtime sanity checks
+	  of non-constant format strings against builtin templates,
+	  issuing a warning and using the template as a fallback in
+	  case of mismatch.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d5b3a3f95c01..81b7cda71158 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3201,3 +3201,68 @@ int sscanf(const char *buf, const char *fmt, ...)
 	return i;
 }
 EXPORT_SYMBOL(sscanf);
+
+#ifdef CONFIG_FMTCHECK
+static int
+next_interesting_spec(const char **s, struct printf_spec *spec)
+{
+	int len;
+
+	while (1) {
+		len = format_decode(*s, spec);
+		if (!len)
+			return 0;
+		*s += len;
+		if (spec->type == FORMAT_TYPE_NONE ||
+		    spec->type == FORMAT_TYPE_PERCENT_CHAR)
+			continue;
+		return len;
+	}
+}
+
+const char *
+_fmtcheck(const char *fmt, const char *tmpl, unsigned flags)
+{
+	const char *f = fmt;
+	const char *t = tmpl;
+	struct printf_spec fspec = {0}, tspec = {0};
+	int flen, tlen;
+	int warn = !(flags & FMTCHECK_SILENT);
+
+	while (1) {
+		flen = next_interesting_spec(&f, &fspec);
+		tlen = next_interesting_spec(&t, &tspec);
+		if (!flen) {
+			/*
+			 * The given format string doesn't have any
+			 * more specifiers. It's ok from a type-safety
+			 * POV for the template to have extra, but
+			 * optionally warn about it (e.g., a single %d
+			 * may be required).
+			 */
+			if (tlen && (flags & FMTCHECK_NO_EXTRA_ARGS) && warn)
+				WARN_ONCE(warn, "template '%s' expects more arguments than '%s'\n",
+					tmpl, fmt);
+			return fmt;
+		}
+		if (!tlen) {
+			WARN_ONCE(warn, "format string '%s' expects more arguments than template '%s'",
+				  fmt, tmpl);
+			return tmpl;
+		}
+		WARN_ONCE(warn && (fspec.type == FORMAT_TYPE_PTR || tspec.type == FORMAT_TYPE_PTR),
+			  "don't use %%p in non-constant format strings");
+		/*
+		 * Should we also care about flags, field width,
+		 * precision? Should we even care about base?
+		 */
+		if (fspec.type != tspec.type ||
+		    fspec.base != tspec.base) {
+			WARN_ONCE(warn, "format string '%s' incompatible with template '%s'",
+				  fmt, tmpl);
+			return tmpl;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(_fmtcheck);
+#endif
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 3/7] kernel.h: implement fmtmatch() wrapper around fmtcheck()

[Rasmus Villemoes]

Some users may prefer to check a "user-supplied" string upfront and
return EINVAL rather than using the the template as a fallback for
printf'ing later. fmtmatch() is simply a shorthand for

  fmtcheck(a, b, c | FMTCHECK_SILENT) == a.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kernel.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 8e9154e100c3..c2b710426227 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -507,11 +507,18 @@ _fmtcheck(const char *fmt, const char *tmpl, unsigned flags)
 	return fmt;
 }
 #endif
+
+static inline bool
+_fmtmatch(const char *fmt, const char *tmpl, unsigned flags)
+{
+       return _fmtcheck(fmt, tmpl, flags | FMTCHECK_SILENT) == fmt;
+}
 /*
  * Use of fmtcheck is pointless if the template is not a string
  * literal, so try to enforce that.
  */
 #define fmtcheck(fmt, tmpl, flags) _fmtcheck(fmt, "" tmpl "", flags)
+#define fmtmatch(fmt, tmpl, flags) _fmtmatch(fmt, "" tmpl "", flags)
 
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 4/7] lib/test_printf.c: add a few fmtcheck() test cases

[Rasmus Villemoes]

It should be trivial to add more test cases, once we figure out the
exact rules for being compatible or not. Perhaps we'll have to extend
the struct test with a flags element if we add flags that affect the
return value.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 53527ea822b5..5b6ba0e817d5 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -535,6 +535,47 @@ test_pointer(void)
 	flags();
 }
 
+static void __init
+test_fmtcheck(void)
+{
+#ifdef CONFIG_FMTCHECK
+	struct test { const char *fmt; const char *tmpl; };
+	static const struct test compatible[] __initconst = {
+		{"", ""},
+		{"wlan%d", "%d"},
+		{"aa%llxbb", "cc%Lxdd%%"},
+	};
+	static const struct test incompatible[] __initconst = {
+		{"a %d b %lx", "%d %x"},
+		{"%llo", "%Lx"},
+	};
+	unsigned i;
+	const struct test *t;
+	const char *ret;
+
+	for (i = 0; i < ARRAY_SIZE(compatible); ++i) {
+		total_tests++;
+		t = &compatible[i];
+		ret = _fmtcheck(t->fmt, t->tmpl, FMTCHECK_SILENT);
+		if (ret != t->fmt) {
+			failed_tests++;
+			pr_warn("'%s' and '%s' deemed incompatible by fmtcheck()",
+				t->fmt, t->tmpl);
+		}
+	}
+	for (i = 0; i < ARRAY_SIZE(incompatible); ++i) {
+		total_tests++;
+		t = &incompatible[i];
+		ret = _fmtcheck(t->fmt, t->tmpl, FMTCHECK_SILENT);
+		if (ret != t->tmpl) {
+			failed_tests++;
+			pr_warn("'%s' and '%s' deemed compatible by fmtcheck()",
+				t->fmt, t->tmpl);
+		}
+	}
+#endif
+}
+
 static int __init
 test_printf_init(void)
 {
@@ -548,6 +589,8 @@ test_printf_init(void)
 	test_string();
 	test_pointer();
 
+	test_fmtcheck();
+
 	kfree(alloced_buffer);
 
 	if (failed_tests == 0)
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 5/7] kernel/kthread.c: do runtime check of format string in kthread_create_on_cpu()

[Rasmus Villemoes]

One is supposed to pass in a format string containing (at most) one %u
instance. Use fmtcheck() to enforce that at runtime, WARNing and falling
back to a harmless "kthread/%u" in case verification fails.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 kernel/kthread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 087d18d771b5..fddfe605632b 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -441,8 +441,8 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 {
 	struct task_struct *p;
 
-	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
-				   cpu);
+	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu),
+				   fmtcheck(namefmt, "kthread/%u", 0), cpu);
 	if (IS_ERR(p))
 		return p;
 	kthread_bind(p, cpu);
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 6/7] nfs: use fmtcheck() in root_nfs_data

[Rasmus Villemoes]

tmp is initially the string "/tftpboot/%s", but it may be changed from
the calls to root_nfs_parse_options. While an nfsroot= command line
option can probably be trusted (or the user gets to keep both pieces),
it's also possible for contents to come via a BOOTP option.

Do a sanity check of fmt to ensure it doesn't contain odd printf
specifiers that would make snprintf go off into the weeds. The lack of
the FMTCHECK_NO_EXTRA_ARGS flag (i.e., the last 0 argument) means we
allow either no specifiers or precisely one occurrence of %s in tmp.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 fs/nfs/nfsroot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
index effaa4247b91..71db0149eb49 100644
--- a/fs/nfs/nfsroot.c
+++ b/fs/nfs/nfsroot.c
@@ -261,7 +261,7 @@ static int __init root_nfs_data(char *cmdline)
 	 * mess into nfs_root_device.
 	 */
 	len = snprintf(nfs_export_path, sizeof(nfs_export_path),
-				tmp, utsname()->nodename);
+				fmtcheck(tmp, "%s", 0), utsname()->nodename);
 	if (len >= (int)sizeof(nfs_export_path))
 		goto out_devnametoolong;
 	len = snprintf(nfs_root_device, sizeof(nfs_root_device),
-- 
2.19.1.6.gbde171bbf5

[RFC PATCH 7/7] drivers: hwmon: add runtime format string checking

[Rasmus Villemoes]

With -Wformat-nonliteral, gcc complains

drivers/hwmon/hwmon.c: In function ‘hwmon_genattr’:
drivers/hwmon/hwmon.c:282:6: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
      index + hwmon_attr_base(type));

Add a runtime check to ensure that the template indeed has a single %d
printf specifier. Using fmtcheck() also makes gcc verify that the
expression 'index + hwmon_attr_base(type)' is suitable for %d.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/hwmon/hwmon.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 33d51281272b..ec6f5f36b5fc 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -278,7 +278,8 @@ static struct attribute *hwmon_genattr(struct device *dev,
 	if (type == hwmon_chip) {
 		name = (char *)template;
 	} else {
-		scnprintf(hattr->name, sizeof(hattr->name), template,
+		scnprintf(hattr->name, sizeof(hattr->name),
+			  fmtcheck(template, "type%dwhat", 0),
 			  index + hwmon_attr_base(type));
 		name = hattr->name;
 	}
-- 
2.19.1.6.gbde171bbf5

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Miguel Ojeda]

Hi Rasmus,

On Sat, Oct 27, 2018 at 1:24 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 6b28c1b7310c..08264df52322 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -32,6 +32,7 @@
>  # define __GCC4_has_attribute___assume_aligned__      (__GNUC_MINOR__ >= 9)
>  # define __GCC4_has_attribute___designated_init__     0
>  # define __GCC4_has_attribute___externally_visible__  1
> +# define __GCC4_has_attribute___format_arg__          1

Thank you Rasmus for doing this already on top of this tree!

>  # define __GCC4_has_attribute___noclone__             1
>  # define __GCC4_has_attribute___optimize__            1
>  # define __GCC4_has_attribute___nonstring__           0
> @@ -140,6 +141,18 @@
>  #define __printf(a, b)                  __attribute__((__format__(printf, a, b)))
>  #define __scanf(a, b)                   __attribute__((__format__(scanf, a, b)))
>
> +/*
> + * Optional

I did quick check and gcc >= 4.1, clang >= 3.0, icc >= 13 compilers
seem to support it (or at least recognize it, even if they just ignore
it), so we do not need to make it optional, no? Did I miss some case?

Thanks!

Cheers,
Miguel

Re: [RFC PATCH 7/7] drivers: hwmon: add runtime format string checking

[Guenter Roeck]

Hi,

On Sat, Oct 27, 2018 at 01:24:09AM +0200, Rasmus Villemoes wrote:
> With -Wformat-nonliteral, gcc complains
> 
> drivers/hwmon/hwmon.c: In function ‘hwmon_genattr’:
> drivers/hwmon/hwmon.c:282:6: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
>       index + hwmon_attr_base(type));
> 
> Add a runtime check to ensure that the template indeed has a single %d
> printf specifier. Using fmtcheck() also makes gcc verify that the
> expression 'index + hwmon_attr_base(type)' is suitable for %d.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

I'll defer to others on this one; let me know if the series is otherwise
accepted. Personally I think that the compiler is at fault here
for not detecting that the format string can not be wrong (since it
is declared and used only in this file), and I find the fmtcheck()
confusing/obfuscating.

Guenter

> ---
>  drivers/hwmon/hwmon.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 33d51281272b..ec6f5f36b5fc 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -278,7 +278,8 @@ static struct attribute *hwmon_genattr(struct device *dev,
>  	if (type == hwmon_chip) {
>  		name = (char *)template;
>  	} else {
> -		scnprintf(hattr->name, sizeof(hattr->name), template,
> +		scnprintf(hattr->name, sizeof(hattr->name),
> +			  fmtcheck(template, "type%dwhat", 0),
>  			  index + hwmon_attr_base(type));
>  		name = hattr->name;
>  	}

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Rasmus Villemoes]

On 2018-10-27 14:06, Miguel Ojeda wrote:
> Hi Rasmus,
> 
> On Sat, Oct 27, 2018 at 1:24 AM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> +/*
>> + * Optional
> 
> I did quick check and gcc >= 4.1, clang >= 3.0, icc >= 13 compilers
> seem to support it (or at least recognize it, even if they just ignore
> it), so we do not need to make it optional, no? Did I miss some case?

No idea. I think I didn't really know what was meant by
required/optional. E.g. something like __aligned must be supported and
honoured for correctness, while format_arg is just about getting some
diagnostics, and assume_aligned just allows for certain optimizations,
so it doesn't really matter whether every single compiler understands
them. It does seem that both icc and clang understand format_arg and
issues warnings when the template doesn't match the varargs, so in that
sense we can make it "required", though it seems a little backwards to
define required in terms of what the current range of compilers support.

Rasmus

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Miguel Ojeda]

On Mon, Oct 29, 2018 at 11:20 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> No idea. I think I didn't really know what was meant by
> required/optional.

Yeah, no worries, "optional" here is intended to mean "we cannot
define it unconditionally (yet)". The word leads to confusion, I
agree. I will improve the wording of the comment in the top and/or
change the word after this lands.

Cheers,
Miguel

Re: [RFC PATCH 0/7] runtime format string checking

[Kees Cook]

On Sat, Oct 27, 2018 at 12:24 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> This is a resurrection of something I sent out about a year ago. In
> the relatively few places where we use a non-literal as format string,
> we can annotate the source with a fmtcheck() call that will (a) at
> build time, allow the compiler to check the variadic arguments against
> the template, and (b) at runtime, check that the format specifiers
> present in the actual format string match those in the template (and
> if not, WARN and use the template to ensure runtime type safety).
>
> Finding places to annotate is just
>
>   make -j8 KCFLAGS='-Wformat-nonliteral'
>
> So far, in about half the places I looked, one might as well get
> completely rid of the non-literal format string.

Agreed. When I was still updating format string sites regularly, I had
a couple "just make the build quiet" patches to do this:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=ce9b938574042d09920650cb3c63ec29658edc87
The above seemed to "noisy" to send, but perhaps we should just land
it anyway. They really _should_ be const.

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
This one covers cases where the pointer is pointing to a const string,
so really there's no sense in injecting the "%s", but I was collecting
them to make real ones stand out.

> Patches 5,6,7 are
> some examples of where one might add fmtcheck() calls. I don't think
> we can get to a state where we can unconditionally add
> -Wformat-nonliteral to the build, but I think there's a lot of
> low-hanging fruit.

How much work do you think it'd take to get to a
format-nonliteral-clean build? I think it's worth doing the work if
it's not totally intractable.

-Kees

>
> This is on top of Miguel's compiler attributes series [1], which I
> hope will land in mainline soon.
>
> [1] https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-4.20-rc1
>
> Rasmus Villemoes (7):
>   compiler_attributes.h: add __attribute__((format_arg)) shorthand
>   lib/vsprintf.c: add fmtcheck utility
>   kernel.h: implement fmtmatch() wrapper around fmtcheck()
>   lib/test_printf.c: add a few fmtcheck() test cases
>   kernel/kthread.c: do runtime check of format string in
>     kthread_create_on_cpu()
>   nfs: use fmtcheck() in root_nfs_data
>   drivers: hwmon: add runtime format string checking
>
>  drivers/hwmon/hwmon.c               |  3 +-
>  fs/nfs/nfsroot.c                    |  2 +-
>  include/linux/compiler_attributes.h | 13 ++++++
>  include/linux/kernel.h              | 25 +++++++++++
>  kernel/kthread.c                    |  4 +-
>  lib/Kconfig.debug                   |  9 ++++
>  lib/test_printf.c                   | 43 +++++++++++++++++++
>  lib/vsprintf.c                      | 65 +++++++++++++++++++++++++++++
>  8 files changed, 160 insertions(+), 4 deletions(-)
>
> --
> 2.19.1.6.gbde171bbf5
>



-- 
Kees Cook

Re: [RFC PATCH 0/7] runtime format string checking

[Rasmus Villemoes]

On 2018-10-30 21:58, Kees Cook wrote:
> On Sat, Oct 27, 2018 at 12:24 AM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=ce9b938574042d09920650cb3c63ec29658edc87
> The above seemed to "noisy" to send, but perhaps we should just land
> it anyway. They really _should_ be const.
>

Isn't that 063246641d4a9e9de84a2466fbad50112faf88dc in mainline ;) ?
BTW, I don't agree with all the changes in there: For auto variables, this

-       const char *cur_drv, *drv = "acpi-cpufreq";
+       const char drv[] = "acpi-cpufreq";
+       const char *cur_drv;

makes gcc actually generate that string on the stack instead of just
referring to an anonymous object in .rodata; one gets code gen like

+:      31 c0                   xor    %eax,%eax
+:      48 b8 61 63 70 69 2d    movabs $0x7570632d69706361,%rax # "acpi-cpu"
+:      63 70 75
+:      c7 44 24 0b 66 72 65    movl   $0x71657266,0xb(%rsp) # "freq"
+:      71
+:      c6 44 24 0f 00          movb   $0x0,0xf(%rsp) "\0"
+:      48 89 44 24 03          mov    %rax,0x3(%rsp)

It's not the-end-of-the-world-horrible, but it's better avoided,
especially for patches that are not supposed to change anything. And
longer strings would of course produce even more gunk like the above.
A better fix which also silences -Wformat-security is to declare the
variable itself const, i.e.

const char *const drv = "acpi-cpufreq".

Yes, gcc should be able to infer the constness of drv from the fact that
it's never assigned to elsewhere in the function... I think I saw that
on some gcc todo list at some point.


> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
> This one covers cases where the pointer is pointing to a const string,
> so really there's no sense in injecting the "%s", but I was collecting
> them to make real ones stand out.

I don't agree. Yes, a human can verify that _currently_, only "pencrypt"
and "pdecrypt" can ever reach pcrypt_sysfs_add(). But without the
compiler being smart enough to do that, one will never know if some new
caller shows up, or one of those literals grows a % for some reason.
Adding "%s" doesn't cost much, especially not in cases (like this one)
where the fmt+args end up at kobject_set_name_vargs - for a "%s" +
literal that does a (succesful) kstrdup_const(), so we never even hit
the vsnprintf() engine.

>> Patches 5,6,7 are
>> some examples of where one might add fmtcheck() calls. I don't think
>> we can get to a state where we can unconditionally add
>> -Wformat-nonliteral to the build, but I think there's a lot of
>> low-hanging fruit.
> 
> How much work do you think it'd take to get to a
> format-nonliteral-clean build? I think it's worth doing the work if
> it's not totally intractable.

Probably less than the VLA removal. But it kind of depends on which
tools one allows. I can't see how to do it without something like
fmtcheck() to annotate certain places (e.g. the nfs example). Maybe a
no_fmtcheck() to annotate places which have been manually verified
[modulo the above "but that may change..."] would also be needed
(no_fmtcheck would be the same as fmtcheck for at !CONFIG_FMTCHECK
kernel), similar to how we have no_printk.

I kind of agree with Guenther that the hwmon example is a bad one. It
would be better to have the compiler check all those string literals
against a pattern at build time. Probably the format template plugin can
be extended to apply to any "const char*" declaration, not just those
sitting inside structs. But I'd rather get fmtcheck() in first before
returning to work on that plugin.

Rasmus

Re: [RFC PATCH 0/7] runtime format string checking

[Kees Cook]

On Thu, Nov 1, 2018 at 3:06 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On 2018-10-30 21:58, Kees Cook wrote:
>> On Sat, Oct 27, 2018 at 12:24 AM, Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=ce9b938574042d09920650cb3c63ec29658edc87
>> The above seemed to "noisy" to send, but perhaps we should just land
>> it anyway. They really _should_ be const.
>>
>
> Isn't that 063246641d4a9e9de84a2466fbad50112faf88dc in mainline ;) ?

Oh, hah, so it is. So long ago I forgot. :P

> BTW, I don't agree with all the changes in there: For auto variables, this
>
> -       const char *cur_drv, *drv = "acpi-cpufreq";
> +       const char drv[] = "acpi-cpufreq";
> +       const char *cur_drv;
>
> makes gcc actually generate that string on the stack instead of just
> referring to an anonymous object in .rodata; one gets code gen like
>
> +:      31 c0                   xor    %eax,%eax
> +:      48 b8 61 63 70 69 2d    movabs $0x7570632d69706361,%rax # "acpi-cpu"
> +:      63 70 75
> +:      c7 44 24 0b 66 72 65    movl   $0x71657266,0xb(%rsp) # "freq"
> +:      71
> +:      c6 44 24 0f 00          movb   $0x0,0xf(%rsp) "\0"
> +:      48 89 44 24 03          mov    %rax,0x3(%rsp)

Oh that is nasty. Ugh. I hate the "const but not really ha ha" optimizations. :(

> It's not the-end-of-the-world-horrible, but it's better avoided,
> especially for patches that are not supposed to change anything. And
> longer strings would of course produce even more gunk like the above.
> A better fix which also silences -Wformat-security is to declare the
> variable itself const, i.e.
>
> const char *const drv = "acpi-cpufreq".

Yes, that would be much better. Seems like we could do a really easy
Coccinelle script to fix all of those?

@@
identifier VAR;
expression STRING;
@@

- const char VAR[]
+ const char * const VAR
  = STRING;

yields:
 517 files changed, 890 insertions(+), 891 deletions(-)

Worth doing at the end of -rc2?

> Yes, gcc should be able to infer the constness of drv from the fact that
> it's never assigned to elsewhere in the function... I think I saw that
> on some gcc todo list at some point.

If you find that bug, I'll add it to my gcc bug tracking list. :P

>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
>> This one covers cases where the pointer is pointing to a const string,
>> so really there's no sense in injecting the "%s", but I was collecting
>> them to make real ones stand out.
>
> I don't agree. Yes, a human can verify that _currently_, only "pencrypt"
> and "pdecrypt" can ever reach pcrypt_sysfs_add(). But without the
> compiler being smart enough to do that, one will never know if some new
> caller shows up, or one of those literals grows a % for some reason.
> Adding "%s" doesn't cost much, especially not in cases (like this one)
> where the fmt+args end up at kobject_set_name_vargs - for a "%s" +
> literal that does a (succesful) kstrdup_const(), so we never even hit
> the vsnprintf() engine.

Okay, then I'll forward this to akpm maybe?

>>> Patches 5,6,7 are
>>> some examples of where one might add fmtcheck() calls. I don't think
>>> we can get to a state where we can unconditionally add
>>> -Wformat-nonliteral to the build, but I think there's a lot of
>>> low-hanging fruit.
>>
>> How much work do you think it'd take to get to a
>> format-nonliteral-clean build? I think it's worth doing the work if
>> it's not totally intractable.
>
> Probably less than the VLA removal. But it kind of depends on which
> tools one allows. I can't see how to do it without something like
> fmtcheck() to annotate certain places (e.g. the nfs example). Maybe a
> no_fmtcheck() to annotate places which have been manually verified
> [modulo the above "but that may change..."] would also be needed
> (no_fmtcheck would be the same as fmtcheck for at !CONFIG_FMTCHECK
> kernel), similar to how we have no_printk.
>
> I kind of agree with Guenther that the hwmon example is a bad one. It
> would be better to have the compiler check all those string literals
> against a pattern at build time. Probably the format template plugin can
> be extended to apply to any "const char*" declaration, not just those
> sitting inside structs. But I'd rather get fmtcheck() in first before
> returning to work on that plugin.

Yeah, fair.

-Kees

-- 
Kees Cook

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Miguel Ojeda]

Hi Rasmus,

On Sat, Oct 27, 2018 at 2:06 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> I did quick check and gcc >= 4.1, clang >= 3.0, icc >= 13 compilers
> seem to support it (or at least recognize it, even if they just ignore
> it), so we do not need to make it optional, no? Did I miss some case?

compiler-attributes landed -- do you want to do the v2 of this (i.e.
#defining it unconditionally) or you prefer I simply fix up the patch?

(I will improve the docs as well in another patch about the "optional" naming).

Cheers,
Miguel

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Rasmus Villemoes]

On 2018-11-02 11:36, Miguel Ojeda wrote:
> Hi Rasmus,
> 
> On Sat, Oct 27, 2018 at 2:06 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> I did quick check and gcc >= 4.1, clang >= 3.0, icc >= 13 compilers
>> seem to support it (or at least recognize it, even if they just ignore
>> it), so we do not need to make it optional, no? Did I miss some case?
> 
> compiler-attributes landed -- do you want to do the v2 of this (i.e.
> #defining it unconditionally) or you prefer I simply fix up the patch?

I'll wait a few more days for comments on fmtcheck(), and allow you to
do the 'optional'/'required' clarification in the meantime. I hope to
get fmtcheck() picked up for -next around -rc3 or so.

Rasmus

Re: [RFC PATCH 0/7] runtime format string checking

[Rasmus Villemoes]

[trimming cc list]

On 2018-11-01 23:57, Kees Cook wrote:
> On Thu, Nov 1, 2018 at 3:06 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> referring to an anonymous object in .rodata; one gets code gen like
>>
>> +:      31 c0                   xor    %eax,%eax
>> +:      48 b8 61 63 70 69 2d    movabs $0x7570632d69706361,%rax # "acpi-cpu"
>> +:      63 70 75
>> +:      c7 44 24 0b 66 72 65    movl   $0x71657266,0xb(%rsp) # "freq"
>> +:      71
>> +:      c6 44 24 0f 00          movb   $0x0,0xf(%rsp) "\0"
>> +:      48 89 44 24 03          mov    %rax,0x3(%rsp)
> 
> Oh that is nasty. Ugh. I hate the "const but not really ha ha" optimizations. :(
> 
>> It's not the-end-of-the-world-horrible, but it's better avoided,
>> especially for patches that are not supposed to change anything. And
>> longer strings would of course produce even more gunk like the above.
>> A better fix which also silences -Wformat-security is to declare the
>> variable itself const, i.e.
>>
>> const char *const drv = "acpi-cpufreq".
> 
> Yes, that would be much better. Seems like we could do a really easy
> Coccinelle script to fix all of those?
> 
> @@
> identifier VAR;
> expression STRING;
> @@
> 
> - const char VAR[]
> + const char * const VAR
>   = STRING;
> 
> yields:
>  517 files changed, 890 insertions(+), 891 deletions(-)
> 
> Worth doing at the end of -rc2?

That's a bit too naive. At the very least, you must exclude static
stuff, i.e. restrict to actual auto variables. Otherwise you're making
things worse (a "static const char []" just occupies some space in
.rodata, a "static const char * const" occupies the same space for the
anonymous literal, plus space for a pointer). Furthermore, you must
ensure that nobody does sizeof() on VAR. With a trivial extension of
your script to exclude the "static const char" places, I get

 97 files changed, 151 insertions(+), 151 deletions(-)

but that includes a number of places at file level where VAR actually
has external linkage. Which is most likely not intentional, but those
places would need different fixes. Actually, a lot of them are of the
'version = "1.2 (Feb 3 1995)"' kind which are utterly useless, so should
simply be removed (possibly left in a comment).

There's not a whole lot of difference between

const char *const foo = "read";

and

static const char foo[] = "read";

The former allows the linker to share "read" with other identical
literals (or reuse the tail of "thread"), but the actual strings in
these cases are likely to be unique and not suffixes of others. The
latter is probably more readable (at least it's more common), and in
some cases one can slap on an __initconst, making the memory footprint
go away entirely. And when sizeof() is used,

So I think it's better to take the above 151 cases and do them in small
batches.

>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
>>> This one covers cases where the pointer is pointing to a const string,
>>> so really there's no sense in injecting the "%s", but I was collecting
>>> them to make real ones stand out.
>>
>> I don't agree. [...]
> 
> Okay, then I'll forward this to akpm maybe?

Yes, if all they do is replace f(..., s) by f(..., "%s", s) that should
never hurt. Maybe check if there's a ..._puts() variant that can be used
instead, e.g. seq_puts().

Rasmus

Re: [RFC PATCH 0/7] runtime format string checking

[Kees Cook]

On Fri, Nov 2, 2018 at 1:09 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> That's a bit too naive. At the very least, you must exclude static
> stuff, i.e. restrict to actual auto variables. Otherwise you're making
> things worse (a "static const char []" just occupies some space in
> .rodata, a "static const char * const" occupies the same space for the
> anonymous literal, plus space for a pointer). Furthermore, you must
> ensure that nobody does sizeof() on VAR. With a trivial extension of
> your script to exclude the "static const char" places, I get

Yes, thank you. That's the part I was forgetting and why I was doing
[] over * back then. There are certainly uses of sizeof() on these
strings. So, it seems better to get sizeof() right that the const-ness
right.

>>>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/format-security&id=b7dcfc8f48caaafcc423e5793f7ef61b9bb5c458
>>>> This one covers cases where the pointer is pointing to a const string,
>>>> so really there's no sense in injecting the "%s", but I was collecting
>>>> them to make real ones stand out.
>>>
>>> I don't agree. [...]
>>
>> Okay, then I'll forward this to akpm maybe?
>
> Yes, if all they do is replace f(..., s) by f(..., "%s", s) that should
> never hurt. Maybe check if there's a ..._puts() variant that can be used
> instead, e.g. seq_puts().

Alright, I'll see about bringing that series forward in time...

-Kees

-- 
Kees Cook

Re: [RFC PATCH 0/7] runtime format string checking

[Rasmus Villemoes]

On 2018-11-01 23:57, Kees Cook wrote:

>> Yes, gcc should be able to infer the constness of drv from the fact that
>> it's never assigned to elsewhere in the function... I think I saw that
>> on some gcc todo list at some point.
> 
> If you find that bug, I'll add it to my gcc bug tracking list. :P

I looked into doing it myself (just for the format checking case, not
for variables in general), but gave up after a few hours. So I created
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87879 . I tried adding you
to the cc list, but it seems you don't have a gcc bugzilla account (?).

Looking more into this, as you can see above, it's actually a little
worse than "false positive" -Wformat-nonliteral - see the f2() case.

[At https://godbolt.org/z/KC4ZRK I also included the const char[] case,
which works as the const char* const case wrt. format checking, but for
some reason gcc open-codes the strlen(). But that's a separate issue,
and not one we should care about, since the const char[] thing is wrong
regardless due to the bade code gen]

Rasmus

Re: [RFC PATCH 1/7] compiler_attributes.h: add __attribute__((format_arg)) shorthand

[Miguel Ojeda]

Hi Rasmus,

On Fri, Nov 2, 2018 at 11:43 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 2018-11-02 11:36, Miguel Ojeda wrote:
> > Hi Rasmus,
> >
> > On Sat, Oct 27, 2018 at 2:06 PM Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> >>
> >> I did quick check and gcc >= 4.1, clang >= 3.0, icc >= 13 compilers
> >> seem to support it (or at least recognize it, even if they just ignore
> >> it), so we do not need to make it optional, no? Did I miss some case?
> >
> > compiler-attributes landed -- do you want to do the v2 of this (i.e.
> > #defining it unconditionally) or you prefer I simply fix up the patch?
>
> I'll wait a few more days for comments on fmtcheck(), and allow you to
> do the 'optional'/'required' clarification in the meantime. I hope to
> get fmtcheck() picked up for -next around -rc3 or so.

Any news about fmtcheck()? The "Optional" clarification landed a few
weeks ago, in case you wanted to know. Hopefully it is more clear now.

Cheers,
Miguel