From patchwork Wed Nov 8 22:30:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 10049573 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 67ACB60381 for ; Wed, 8 Nov 2017 22:31:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 57A9A2A41A for ; Wed, 8 Nov 2017 22:31:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4A7B12A662; Wed, 8 Nov 2017 22:31:23 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id AFEE929994 for ; Wed, 8 Nov 2017 22:31:21 +0000 (UTC) Received: (qmail 17475 invoked by uid 550); 8 Nov 2017 22:30:57 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 17420 invoked from network); 8 Nov 2017 22:30:56 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=gATDgjDmoH9yGb8HwrYSp0UviHkCEzEUpsaftvyCqrU=; b=jSkf9p9RhCa/cait1lLzxSK+BQvjVqYDikGr8nGQDNplZihqo2kmgK5HBsuIRRIkcb c3bncP/iVI9AtacS1gXCnllnlrkD51eP6c6iQgl8VepSS8iPkfYsPZc/BXOnUtuyYZyS sB3ziT59XKdUg17Jx8czirFogvBtjvWMq2s08= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=gATDgjDmoH9yGb8HwrYSp0UviHkCEzEUpsaftvyCqrU=; b=YQX/Bv8Bdx5kAMFdo0/Foc6WZeJSmZOQFnc628pq+lmt+uItvXRp1UFlRVBV2h61q0 sRo+9inBWrkqpvMSPFoS5JKDnNVx7pUiHZtlMLiUYeDYgol/ooqSsqDQgAQFDqKGnT2c gTubFpBGIRwty5HAgXDK7NFltY22ayFV9KszhirS4QyvyKJJBjVTIO4vqgl5KbHeJ65W S8Cmx+O5F2r+m93nTmlkVk5wN8Q+vZ4JHV9Umm8e690bfYzI6M/ROeEuQAHKvw97ZJl1 9W0hmnzBVEtZJCCLr/mqlozYcQy12KFnAgK9DJet2UXgXGgy/NDrAzkKWpvdw/tSABDK otQQ== X-Gm-Message-State: AJaThX7HRDAYzR+ArwsIgmcsViYwBtSlR5uw5sVJNwXoY9sA64UrXkd4 kmG7mqdTb2q5V/5GUcJS5nnigqXA X-Google-Smtp-Source: ABhQp+QYtRs7QYpiUy8UdSRjshrsVWciXUM478oOpDjrC61rxRmE6qe97TDrzOQHCQIvJ9p09OwrgQ== X-Received: by 10.223.185.33 with SMTP id k30mr1729909wrf.40.1510180244458; Wed, 08 Nov 2017 14:30:44 -0800 (PST) From: Rasmus Villemoes To: kernel-hardening@lists.openwall.com Cc: linux-kernel@vger.kernel.org, Andrew Morton , Kees Cook , Rasmus Villemoes Date: Wed, 8 Nov 2017 23:30:15 +0100 Message-Id: <20171108223020.24487-2-linux@rasmusvillemoes.dk> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20171108223020.24487-1-linux@rasmusvillemoes.dk> References: <20171108223020.24487-1-linux@rasmusvillemoes.dk> Subject: [kernel-hardening] [RFC 1/6] plugins: implement format_template attribute X-Virus-Scanned: ClamAV using ClamSMTP 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 --- 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 +#include + +#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, ®ister_attributes, NULL); + + return 0; +}