[PATCH 0/4] printf stuff

[PATCH 0/4] printf stuff

[Rasmus Villemoes]

A few printf-related patches I've been sitting on. I also have some
documentation updates, but I'll wait until I see Martin's patch [1] in
-next. There's also the %pb issue [2], but I'm not sure there's consensus
on the best fix for that.

[1] https://lkml.org/lkml/2015/9/24/256
[2] https://lkml.org/lkml/2015/9/16/769

Rasmus Villemoes (4):
  lib/vsprintf.c: handle invalid format specifiers more robustly
  lib/vsprintf.c: also improve sanity check in bstr_printf()
  lib/vsprintf.c: Remove SPECIAL handling in pointer()
  test_printf: test printf family at runtime

 lib/Kconfig.debug |   3 +
 lib/Makefile      |   1 +
 lib/test_printf.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c    |  35 ++++--
 4 files changed, 391 insertions(+), 12 deletions(-)
 create mode 100644 lib/test_printf.c

-- 
2.1.3

[PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly

[Rasmus Villemoes]

If we meet any invalid or unsupported format specifier, 'handling' it
by just printing it as a literal string is not safe: Presumably the
format string and the arguments passed gcc's type checking, but that
means something like sprintf(buf, "%n %pd", &intvar, dentry) would end
up interpreting &intvar as a struct dentry*.

When the offending specifier was %n it used to be at the end of the
format string, but we can't rely on that always being the case. Also,
gcc doesn't complain about some more or less exotic qualifiers (or
'length modifiers' in posix-speak) such as 'j' or 'q', but being
unrecognized by the kernel's printf implementation, they'd be
interpreted as unknown specifiers, and the rest of arguments would be
interpreted wrongly.

So let's complain about anything we don't understand, not just %n, and
stop pretending that we'd be able to make sense of the rest of the
format/arguments. If the offending specifier is in a printk() call we
unfortunately only get a "BUG: recent printk recursion!", but at least
direct users of the sprintf family will be caught.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b43b99..f2590a80937f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1769,14 +1769,14 @@ qualifier:
 
 	case 'n':
 		/*
-		 * Since %n poses a greater security risk than utility, treat
-		 * it as an invalid format specifier. Warn about its use so
-		 * that new instances don't get added.
+		 * Since %n poses a greater security risk than
+		 * utility, treat it as any other invalid or
+		 * unsupported format specifier.
 		 */
-		WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
 		/* Fall-through */
 
 	default:
+		WARN_ONCE(1, "Please remove unsupported %%%c in format string\n", *fmt);
 		spec->type = FORMAT_TYPE_INVALID;
 		return fmt - start;
 	}
@@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_INVALID:
-			if (str < end)
-				*str = '%';
-			++str;
-			break;
+			/*
+			 * Presumably the arguments passed gcc's type
+			 * checking, but there is no safe or sane way
+			 * for us to continue parsing the format and
+			 * fetching from the va_list; the remaining
+			 * specifiers and arguments would be out of
+			 * sync.
+			 */
+			goto out;
 
 		default:
 			switch (spec.type) {
@@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		}
 	}
 
+out:
 	if (size > 0) {
 		if (str < end)
 			*str = '\0';
@@ -2189,9 +2195,10 @@ do {									\
 
 		switch (spec.type) {
 		case FORMAT_TYPE_NONE:
-		case FORMAT_TYPE_INVALID:
 		case FORMAT_TYPE_PERCENT_CHAR:
 			break;
+		case FORMAT_TYPE_INVALID:
+			goto out;
 
 		case FORMAT_TYPE_WIDTH:
 		case FORMAT_TYPE_PRECISION:
@@ -2253,6 +2260,7 @@ do {									\
 		}
 	}
 
+out:
 	return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf;
 #undef save_arg
 }
@@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-		case FORMAT_TYPE_INVALID:
 			if (str < end)
 				*str = '%';
 			++str;
 			break;
 
+		case FORMAT_TYPE_INVALID:
+			goto out;
+
 		default: {
 			unsigned long long num;
 
@@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		} /* switch(spec.type) */
 	} /* while(*fmt) */
 
+out:
 	if (size > 0) {
 		if (str < end)
 			*str = '\0';
-- 
2.1.3

[PATCH 2/4] lib/vsprintf.c: also improve sanity check in bstr_printf()

[Rasmus Villemoes]

Quoting from 2aa2f9e21e4e ("lib/vsprintf.c: improve sanity check in
vsnprintf()"):

    On 64 bit, size may very well be huge even if bit 31 happens to be 0.
    Somehow it doesn't feel right that one can pass a 5 GiB buffer but not a
    3 GiB one.  So cap at INT_MAX as was probably the intention all along.
    This is also the made-up value passed by sprintf and vsprintf.

I should have seen this copy-pasted instance back then, but let's just
do it now.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f2590a80937f..03fa10b4be96 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2294,7 +2294,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 	char *str, *end;
 	const char *args = (const char *)bin_buf;
 
-	if (WARN_ON_ONCE((int) size < 0))
+	if (WARN_ON_ONCE(size > INT_MAX))
 		return 0;
 
 	str = buf;
-- 
2.1.3

[PATCH 3/4] lib/vsprintf.c: Remove SPECIAL handling in pointer()

[Rasmus Villemoes]

As a quick

   git grep -E '%[ +0#-]*#[ +0#-]*(\*|[0-9]+)?(\.(\*|[0-9]+)?)?p'

shows, nobody uses the # flag with %p. Moreover, I think users are
unlikely to show up since gcc will complain with

  warning: `#' flag used with ‘%p’ gnu_printf format [-Wformat]

Since default_width is effectively always 2*sizeof(void*), we can
simplify the prologue of pointer() and save a few instructions.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 03fa10b4be96..98b0d7be3fb7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1457,7 +1457,7 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
+	const int default_width = 2 * sizeof(void *);
 
 	if (!ptr && *fmt != 'K') {
 		/*
-- 
2.1.3

[PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

This adds a simple module for testing the kernel's printf
facilities. Previously, some %p extensions have caused a wrong return
value in case the entire output didn't fit and/or been unusable in
kasprintf(). This should help catch such issues. Also, it should help
ensure that changes to the formatting algorithms don't break anything.

I'm not sure if we have a struct dentry or struct file lying around at
boot time or if we can fake one, but most %p extensions should be
testable, as should the ordinary number and string formatting.

The nature of vararg functions means we can't use a more conventional
table-driven approach.

For now, this is mostly a skeleton; contributions are very
welcome. Some tests are/will be slightly annoying to write, since the
expected output depends on stuff like CONFIG_*, sizeof(long), runtime
values etc.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/Kconfig.debug |   3 +
 lib/Makefile      |   1 +
 lib/test_printf.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 368 insertions(+)
 create mode 100644 lib/test_printf.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ab76b99adc85..c23fc42dc659 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
 config TEST_KSTRTOX
 	tristate "Test kstrto*() family of functions at runtime"
 
+config TEST_PRINTF
+	tristate "Test printf() family of functions at runtime"
+
 config TEST_RHASHTABLE
 	tristate "Perform selftest on resizable hash table"
 	default n
diff --git a/lib/Makefile b/lib/Makefile
index 13a7c6ae3fec..775de427ea92 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
+obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_printf.c b/lib/test_printf.c
new file mode 100644
index 000000000000..d9a2741c2909
--- /dev/null
+++ b/lib/test_printf.c
@@ -0,0 +1,364 @@
+/*
+ * Test cases for printf facility.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/random.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include <linux/socket.h>
+#include <linux/in.h>
+
+#define BUF_SIZE 256
+#define FILL_CHAR '$'
+
+#define PTR1 ((void*)0x01234567)
+#define PTR2 ((void*)(long)(int)0xfedcba98)
+
+#if BITS_PER_LONG == 64
+#define PTR1_ZEROES "000000000"
+#define PTR1_SPACES "         "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fffffffffedcba98"
+#define PTR_WIDTH 16
+#else
+#define PTR1_ZEROES "0"
+#define PTR1_SPACES " "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fedcba98"
+#define PTR_WIDTH 8
+#endif
+#define PTR_WIDTH_STR stringify(PTR_WIDTH)
+
+static unsigned total_tests __initdata;
+static unsigned failed_tests __initdata;
+static char *test_buffer __initdata;
+
+static int __printf(4, 0) __init
+do_test(int bufsize, const char *expect, int elen,
+	const char *fmt, va_list ap)
+{
+	va_list aq;
+	int ret, written;
+
+	total_tests++;
+
+	memset(test_buffer, FILL_CHAR, BUF_SIZE);
+	va_copy(aq, ap);
+	ret = vsnprintf(test_buffer, bufsize, fmt, aq);
+	va_end(aq);
+
+	if (ret != elen) {
+		pr_warn("bad return value, expected %d, got %d, format was '%s'\n",
+			elen, ret, fmt);
+		return 1;
+	}
+
+	if (!bufsize) {
+		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
+			pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
+				fmt);
+			return 1;
+		}
+		return 0;
+	}
+
+	written = min(bufsize-1, elen);
+	if (test_buffer[written]) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
+			bufsize, fmt);
+		return 1;
+	}
+
+	if (memcmp(test_buffer, expect, written)) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
+			bufsize, fmt, test_buffer, written, expect);
+		return 1;
+	}
+	return 0;
+}
+
+
+static void __printf(3, 4) __init
+__test(const char *expect, int elen, const char *fmt, ...)
+{
+	va_list ap;
+	int rand;
+	char *p;
+
+	BUG_ON(elen >= BUF_SIZE);
+
+	va_start(ap, fmt);
+
+	/*
+	 * Every fmt+args is subjected to four tests: Three where we
+	 * tell vsnprintf varying buffer sizes (plenty, not quite
+	 * enough and 0), and then we also test that kvasprintf would
+	 * be able to print it as expected.
+	 */
+	failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
+	rand = 1 + prandom_u32_max(elen+1);
+	/* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
+	failed_tests += do_test(rand, expect, elen, fmt, ap);
+	failed_tests += do_test(0, expect, elen, fmt, ap);
+
+	p = kvasprintf(GFP_KERNEL, fmt, ap);
+	if (p) {
+		if (memcmp(p, expect, elen+1)) {
+			pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
+				fmt, p, expect);
+			failed_tests++;
+		}
+		kfree(p);
+	}
+	va_end(ap);
+}
+
+#define test(expect, fmt, ...)					\
+	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
+
+
+static void __init
+test_basic(void)
+{
+	test("", "");
+	test("100%", "100%%");
+	test("xxx%yyy", "xxx%cyyy", '%');
+	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
+}
+
+static void __init
+test_number(void)
+{
+	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
+	test("  0x1234abcd", "%#12x", 0x1234abcd);
+	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
+}
+
+static void __init
+test_string(void)
+{
+	test("", "%s%.0s", "", "123");
+	test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
+	test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
+	/*
+	 * POSIX and C99 say that a missing precision should be
+	 * treated as a precision of 0. However, the kernel's printf
+	 * implementation treats this case as if the . wasn't
+	 * present. Let's add a test case documenting the current
+	 * behaviour; should anyone ever feel the need to follow the
+	 * standards more closely, this can be revisited.
+	 */
+	test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
+	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
+}
+
+
+static void __init
+plain(void)
+{
+	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
+	/*
+	 * The field width is overloaded for some %p extensions to
+	 * pass another piece of information. For plain pointers, the
+	 * behaviour is slightly odd: One cannot pass either the 0
+	 * flag nor a precition to %p without gcc complaining, and if
+	 * one explicitly gives a field width, the number is no longer
+	 * zero-padded.
+	 */
+	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
+	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
+	test("|" PTR2_STR "  |  " PTR2_STR "|",
+	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
+
+	/*
+	 * Unrecognized %p extensions are treated as plain %p, but the
+	 * alphanumeric suffix is ignored (that is, does not occur in
+	 * the output.)
+	 */
+	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
+	test("|"PTR2_STR"|", "|%p0y|", PTR2);
+}
+
+static void __init
+symbol_ptr(void)
+{
+}
+
+static void __init
+kernel_ptr(void)
+{
+}
+
+static void __init
+struct_resource(void)
+{
+}
+
+static void __init
+addr(void)
+{
+}
+
+static void __init
+escaped_str(void)
+{
+}
+
+static void __init
+hex_string(void)
+{
+	const char buf[3] = {0xc0, 0xff, 0xee};
+
+	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
+	     "%3ph|%3phC|%3phD|%3phN", buf, buf, buf, buf);
+	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
+	     "%*ph|%*phC|%*phD|%*phN", 3, buf, 3, buf, 3, buf, 3, buf);
+}
+
+static void __init
+mac(void)
+{
+	const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
+
+	test("2d:48:d6:fc:7a:05", "%pM", addr);
+	test("05:7a:fc:d6:48:2d", "%pMR", addr);
+	test("2d-48-d6-fc-7a-05", "%pMF", addr);
+	test("2d48d6fc7a05", "%pm", addr);
+	test("057afcd6482d", "%pmR", addr);
+}
+
+static void __init
+ip4(void)
+{
+	struct sockaddr_in sa;
+
+	sa.sin_family = AF_INET;
+	sa.sin_port = cpu_to_be16(12345);
+	sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
+
+	test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+	test("127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
+	sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
+	test("001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);
+}
+
+static void __init
+ip6(void)
+{
+}
+
+static void __init
+ip(void)
+{
+	ip4();
+	ip6();
+}
+
+static void __init
+uuid(void)
+{
+	const char uuid[16] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
+			       0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf};
+
+	test("00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid);
+	test("00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid);
+	test("03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid);
+	test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
+}
+
+static void __init
+dentry(void)
+{
+}
+
+static void __init
+struct_va_format(void)
+{
+}
+
+static void __init
+struct_clk(void)
+{
+}
+
+static void __init
+bitmap(void)
+{
+	DECLARE_BITMAP(bits, 20);
+	const int primes[] = {2,3,5,7,11,13,17,19};
+	int i;
+
+	bitmap_zero(bits, 20);
+	test("00000|00000", "%20pb|%*pb", bits, 20, bits);
+	test("|", "%20pbl|%*pbl", bits, 20, bits);
+
+	for (i = 0; i < ARRAY_SIZE(primes); ++i)
+		set_bit(primes[i], bits);
+	test("a28ac|a28ac", "%20pb|%*pb", bits, 20, bits);
+	test("2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", "%20pbl|%*pbl", bits, 20, bits);
+
+	bitmap_fill(bits, 20);
+	test("fffff|fffff", "%20pb|%*pb", bits, 20, bits);
+	test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
+}
+
+static void __init
+netdev_features(void)
+{
+}
+
+
+static void __init
+test_pointer(void)
+{
+	plain();
+	symbol_ptr();
+	kernel_ptr();
+	struct_resource();
+	addr();
+	escaped_str();
+	hex_string();
+	mac();
+	ip();
+	uuid();
+	dentry();
+	struct_va_format();
+	struct_clk();
+	bitmap();
+	netdev_features();
+}
+
+static int __init
+test_printf_init(void)
+{
+	test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!test_buffer)
+		return -ENOMEM;
+
+	test_basic();
+	test_number();
+	test_string();
+	test_pointer();
+
+	kfree(test_buffer);
+
+	if (failed_tests == 0)
+		pr_info("all %u tests passed\n", total_tests);
+	else
+		pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
+
+	return 0;
+}
+
+module_init(test_printf_init);
+
+
+MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
+MODULE_LICENSE("GPL");
-- 
2.1.3

Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly

[Andy Shevchenko]

On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> If we meet any invalid or unsupported format specifier, 'handling' it
> by just printing it as a literal string is not safe: Presumably the
> format string and the arguments passed gcc's type checking, but that
> means something like sprintf(buf, "%n %pd", &intvar, dentry) would 
> end
> up interpreting &intvar as a struct dentry*.
> 
> When the offending specifier was %n it used to be at the end of the
> format string, but we can't rely on that always being the case. Also,
> gcc doesn't complain about some more or less exotic qualifiers (or
> 'length modifiers' in posix-speak) such as 'j' or 'q', but being
> unrecognized by the kernel's printf implementation, they'd be
> interpreted as unknown specifiers, and the rest of arguments would be
> interpreted wrongly.
> 
> So let's complain about anything we don't understand, not just %n, 
> and
> stop pretending that we'd be able to make sense of the rest of the
> format/arguments. If the offending specifier is in a printk() call we
> unfortunately only get a "BUG: recent printk recursion!", but at 
> least
> direct users of the sprintf family will be caught.

I like the fix (I noticed the same issue after commented out Martin's
patch).

Few minor comments below.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 95cd63b43b99..f2590a80937f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1769,14 +1769,14 @@ qualifier:
>  
>  	case 'n':
>  		/*
> -		 * Since %n poses a greater security risk than 
> utility, treat
> -		 * it as an invalid format specifier. Warn about its 
> use so
> -		 * that new instances don't get added.
> +		 * Since %n poses a greater security risk than

Any reason to wrap first string?

> +		 * utility, treat it as any other invalid or
> +		 * unsupported format specifier.
>  		 */
> -		WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", 
> fmt);
>  		/* Fall-through */
>  
>  	default:
> +		WARN_ONCE(1, "Please remove unsupported %%%c in 
> format string\n", *fmt);
>  		spec->type = FORMAT_TYPE_INVALID;
>  		return fmt - start;
>  	}
> @@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const 
> char *fmt, va_list args)
>  			break;
>  
>  		case FORMAT_TYPE_INVALID:
> -			if (str < end)
> -				*str = '%';
> -			++str;
> -			break;
> +			/*
> +			 * Presumably the arguments passed gcc's 
> type
> +			 * checking, but there is no safe or sane 
> way
> +			 * for us to continue parsing the format and
> +			 * fetching from the va_list; the remaining
> +			 * specifiers and arguments would be out of
> +			 * sync.

Could we use wider strings in the commentary here?

> +			 */
> +			goto out;
>  
>  		default:
>  			switch (spec.type) {
> @@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const 
> char *fmt, va_list args)
>  		}
>  	}
>  
> +out:
>  	if (size > 0) {
>  		if (str < end)
>  			*str = '\0';
> @@ -2189,9 +2195,10 @@ do {						> 	> 	> 	> \
> 
>  
>  		switch (spec.type) {
>  		case FORMAT_TYPE_NONE:
> -		case FORMAT_TYPE_INVALID:
>  		case FORMAT_TYPE_PERCENT_CHAR:
>  			break;
> +		case FORMAT_TYPE_INVALID:
> +			goto out;
>  
>  		case FORMAT_TYPE_WIDTH:
>  		case FORMAT_TYPE_PRECISION:
> @@ -2253,6 +2260,7 @@ do {						> 	> 	> 	> \
> 
>  		}
>  	}
>  
> +out:
>  	return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf;
>  #undef save_arg
>  }
> @@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const 
> char *fmt, const u32 *bin_buf)
>  			break;
>  
>  		case FORMAT_TYPE_PERCENT_CHAR:
> -		case FORMAT_TYPE_INVALID:
>  			if (str < end)
>  				*str = '%';
>  			++str;
>  			break;
>  
> +		case FORMAT_TYPE_INVALID:
> +			goto out;
> +
>  		default: {
>  			unsigned long long num;
>  
> @@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const 
> char *fmt, const u32 *bin_buf)
>  		} /* switch(spec.type) */
>  	} /* while(*fmt) */
>  
> +out:
>  	if (size > 0) {
>  		if (str < end)
>  			*str = '\0';

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/4] lib/vsprintf.c: Remove SPECIAL handling in pointer()

[Andy Shevchenko]

On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> As a quick
> 
>    git grep -E '%[ +0#-]*#[ +0#-]*(\*|[0-9]+)?(\.(\*|[0-9]+)?)?p'
> 
> shows, nobody uses the # flag with %p. Moreover, I think users are
> unlikely to show up since gcc will complain with
> 
>   warning: `#' flag used with ‘%p’ gnu_printf format [-Wformat]

I would refer to POSIX here

"# Specifies that the value is to be converted to an alternative form.
...
For other conversion specifiers, the behavior is undefined."

> 
> Since default_width is effectively always 2*sizeof(void*), we can
> simplify the prologue of pointer() and save a few instructions.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 03fa10b4be96..98b0d7be3fb7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1457,7 +1457,7 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> -	int default_width = 2 * sizeof(void *) + (spec.flags & 
> SPECIAL ? 2 : 0);
> +	const int default_width = 2 * sizeof(void *);
>  
>  	if (!ptr && *fmt != 'K') {
>  		/*

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Andy Shevchenko]

On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> This adds a simple module for testing the kernel's printf
> facilities. Previously, some %p extensions have caused a wrong return
> value in case the entire output didn't fit and/or been unusable in
> kasprintf(). This should help catch such issues. Also, it should help
> ensure that changes to the formatting algorithms don't break 
> anything.
> 
> I'm not sure if we have a struct dentry or struct file lying around 
> at
> boot time or if we can fake one, but most %p extensions should be
> testable, as should the ordinary number and string formatting.
> 
> The nature of vararg functions means we can't use a more conventional
> table-driven approach.
> 
> For now, this is mostly a skeleton; contributions are very
> welcome. Some tests are/will be slightly annoying to write, since the
> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
> values etc.

Few comments below.

> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/Kconfig.debug |   3 +
>  lib/Makefile      |   1 +
>  lib/test_printf.c | 364 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 368 insertions(+)
>  create mode 100644 lib/test_printf.c
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab76b99adc85..c23fc42dc659 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
>  config TEST_KSTRTOX
>  	tristate "Test kstrto*() family of functions at runtime"
>  
> +config TEST_PRINTF
> +	tristate "Test printf() family of functions at runtime"
> +
>  config TEST_RHASHTABLE
>  	tristate "Perform selftest on resizable hash table"
>  	default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 13a7c6ae3fec..775de427ea92 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
>  obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> +obj-$(CONFIG_TEST_PRINTF) += test_printf.o
>  
>  ifeq ($(CONFIG_DEBUG_KOBJECT),y)
>  CFLAGS_kobject.o += -DDEBUG
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> new file mode 100644
> index 000000000000..d9a2741c2909
> --- /dev/null
> +++ b/lib/test_printf.c
> @@ -0,0 +1,364 @@
> +/*
> + * Test cases for printf facility.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/random.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include <linux/socket.h>
> +#include <linux/in.h>
> +
> +#define BUF_SIZE 256
> +#define FILL_CHAR '$'
> +
> +#define PTR1 ((void*)0x01234567)
> +#define PTR2 ((void*)(long)(int)0xfedcba98)
> +
> +#if BITS_PER_LONG == 64
> +#define PTR1_ZEROES "000000000"
> +#define PTR1_SPACES "         "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fffffffffedcba98"
> +#define PTR_WIDTH 16
> +#else
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 8
> +#endif
> +#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> +
> +static unsigned total_tests __initdata;
> +static unsigned failed_tests __initdata;
> +static char *test_buffer __initdata;
> +
> +static int __printf(4, 0) __init
> +do_test(int bufsize, const char *expect, int elen,
> +	const char *fmt, va_list ap)
> +{
> +	va_list aq;
> +	int ret, written;
> +
> +	total_tests++;
> +
> +	memset(test_buffer, FILL_CHAR, BUF_SIZE);
> +	va_copy(aq, ap);
> +	ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> +	va_end(aq);
> +
> +	if (ret != elen) {
> +		pr_warn("bad return value, expected %d, got %d, 
> format was '%s'\n",
> +			elen, ret, fmt);
> +		return 1;
> +	}
> +
> +	if (!bufsize) {
> +		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
> +			pr_warn("vsnprintf(buf, 0, \"%s\", ...) 
> wrote to buffer\n",
> +				fmt);
> +			return 1;
> +		}
> +		return 0;
> +	}
> +
> +	written = min(bufsize-1, elen);
> +	if (test_buffer[written]) {
> +		pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul
> -terminate buffer\n",
> +			bufsize, fmt);
> +		return 1;
> +	}
> +
> +	if (memcmp(test_buffer, expect, written)) {
> +		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', 
> expected '%.*s'\n",
> +			bufsize, fmt, test_buffer, written, expect);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +
> +static void __printf(3, 4) __init
> +__test(const char *expect, int elen, const char *fmt, ...)
> +{
> +	va_list ap;
> +	int rand;
> +	char *p;
> +
> +	BUG_ON(elen >= BUF_SIZE);
> +
> +	va_start(ap, fmt);
> +
> +	/*
> +	 * Every fmt+args is subjected to four tests: Three where we
> +	 * tell vsnprintf varying buffer sizes (plenty, not quite
> +	 * enough and 0), and then we also test that kvasprintf 
> would
> +	 * be able to print it as expected.
> +	 */
> +	failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
> +	rand = 1 + prandom_u32_max(elen+1);
> +	/* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
> +	failed_tests += do_test(rand, expect, elen, fmt, ap);
> +	failed_tests += do_test(0, expect, elen, fmt, ap);
> +
> +	p = kvasprintf(GFP_KERNEL, fmt, ap);
> +	if (p) {
> +		if (memcmp(p, expect, elen+1)) {
> +			pr_warn("kvasprintf(..., \"%s\", ...) 
> returned '%s', expected '%s'\n",
> +				fmt, p, expect);
> +			failed_tests++;
> +		}
> +		kfree(p);
> +	}
> +	va_end(ap);
> +}
> +
> +#define test(expect, fmt, ...)					\
> +	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)

Would be __test_m[em] / __test_s[tr] to distinguish them by name?

And might be inline function?

> +
> +
> +static void __init
> +test_basic(void)
> +{
> +	test("", "");
> +	test("100%", "100%%");
> +	test("xxx%yyy", "xxx%cyyy", '%');
> +	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');

And such pieces will be look better

__test_str("xxx%yyy", "xxx%cyyy", '%');
__test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0');

> +}
> +
> +static void __init
> +test_number(void)
> +{
> +	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
> +	test("  0x1234abcd", "%#12x", 0x1234abcd);
> +	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% 
> d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
> +}
> +
> +static void __init
> +test_string(void)
> +{
> +	test("", "%s%.0s", "", "123");
> +	test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, 
> "123456");
> +	test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", 
> "2", 3, "3", 3, "4", -3, "5");
> +	/*
> +	 * POSIX and C99 say that a missing precision should be
> +	 * treated as a precision of 0. However, the kernel's printf
> +	 * implementation treats this case as if the . wasn't
> +	 * present. Let's add a test case documenting the current
> +	 * behaviour; should anyone ever feel the need to follow the
> +	 * standards more closely, this can be revisited.
> +	 */
> +	test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
> +	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, 
> "c");
> +}
> +
> +
> +static void __init
> +plain(void)
> +{
> +	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, 
> PTR2);
> +	/*
> +	 * The field width is overloaded for some %p extensions to
> +	 * pass another piece of information. For plain pointers, 
> the
> +	 * behaviour is slightly odd: One cannot pass either the 0
> +	 * flag nor a precition to %p without gcc complaining, and 
> if
> +	 * one explicitly gives a field width, the number is no 
> longer
> +	 * zero-padded.
> +	 */
> +	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR 
> "|",
> +	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
> +	test("|" PTR2_STR "  |  " PTR2_STR "|",
> +	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
> +
> +	/*
> +	 * Unrecognized %p extensions are treated as plain %p, but 
> the
> +	 * alphanumeric suffix is ignored (that is, does not occur 
> in
> +	 * the output.)
> +	 */
> +	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
> +	test("|"PTR2_STR"|", "|%p0y|", PTR2);
> +}
> +
> +static void __init
> +symbol_ptr(void)
> +{
> +}
> +
> +static void __init
> +kernel_ptr(void)
> +{
> +}
> +
> +static void __init
> +struct_resource(void)
> +{
> +}
> +
> +static void __init
> +addr(void)
> +{
> +}
> +
> +static void __init
> +escaped_str(void)
> +{
> +}
> +
> +static void __init
> +hex_string(void)
> +{
> +	const char buf[3] = {0xc0, 0xff, 0xee};
> +
> +	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
> +	     "%3ph|%3phC|%3phD|%3phN", buf, buf, buf, buf);
> +	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
> +	     "%*ph|%*phC|%*phD|%*phN", 3, buf, 3, buf, 3, buf, 3, 
> buf);
> +}
> +
> +static void __init
> +mac(void)
> +{
> +	const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
> +
> +	test("2d:48:d6:fc:7a:05", "%pM", addr);
> +	test("05:7a:fc:d6:48:2d", "%pMR", addr);
> +	test("2d-48-d6-fc-7a-05", "%pMF", addr);
> +	test("2d48d6fc7a05", "%pm", addr);
> +	test("057afcd6482d", "%pmR", addr);
> +}
> +
> +static void __init
> +ip4(void)
> +{
> +	struct sockaddr_in sa;
> +
> +	sa.sin_family = AF_INET;
> +	sa.sin_port = cpu_to_be16(12345);
> +	sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
> +
> +	test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, 
> &sa.sin_addr);
> +	test("127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
> +	sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
> +	test("001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", 
> &sa, &sa);
> +}
> +
> +static void __init
> +ip6(void)
> +{
> +}
> +
> +static void __init
> +ip(void)
> +{
> +	ip4();
> +	ip6();
> +}
> +
> +static void __init
> +uuid(void)
> +{
> +	const char uuid[16] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 
> 0x7,
> +			       0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 
> 0xf};
> +
> +	test("00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid);
> +	test("00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid);
> +	test("03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid);
> +	test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
> +}
> +
> +static void __init
> +dentry(void)
> +{
> +}
> +
> +static void __init
> +struct_va_format(void)
> +{
> +}
> +
> +static void __init
> +struct_clk(void)
> +{
> +}
> +
> +static void __init
> +bitmap(void)
> +{
> +	DECLARE_BITMAP(bits, 20);
> +	const int primes[] = {2,3,5,7,11,13,17,19};
> +	int i;
> +
> +	bitmap_zero(bits, 20);
> +	test("00000|00000", "%20pb|%*pb", bits, 20, bits);
> +	test("|", "%20pbl|%*pbl", bits, 20, bits);
> +
> +	for (i = 0; i < ARRAY_SIZE(primes); ++i)
> +		set_bit(primes[i], bits);
> +	test("a28ac|a28ac", "%20pb|%*pb", bits, 20, bits);
> +	test("2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", 
> "%20pbl|%*pbl", bits, 20, bits);
> +
> +	bitmap_fill(bits, 20);
> +	test("fffff|fffff", "%20pb|%*pb", bits, 20, bits);
> +	test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
> +}
> +
> +static void __init
> +netdev_features(void)
> +{
> +}
> +
> +

Maybe commentary delimiter here and above where you have double empty
line.

> +static void __init
> +test_pointer(void)
> +{
> +	plain();
> +	symbol_ptr();
> +	kernel_ptr();
> +	struct_resource();
> +	addr();
> +	escaped_str();
> +	hex_string();
> +	mac();
> +	ip();
> +	uuid();
> +	dentry();
> +	struct_va_format();
> +	struct_clk();
> +	bitmap();
> +	netdev_features();
> +}
> +
> +static int __init
> +test_printf_init(void)
> +{
> +	test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> +	if (!test_buffer)
> +		return -ENOMEM;
> +
> +	test_basic();
> +	test_number();
> +	test_string();
> +	test_pointer();
> +
> +	kfree(test_buffer);
> +
> +	if (failed_tests == 0)
> +		pr_info("all %u tests passed\n", total_tests);
> +	else
> +		pr_warn("failed %u out of %u tests\n", failed_tests, 
> total_tests);
> +
> +	return 0;

Do we need this module in a memory?

> +}
> +
> +module_init(test_printf_init);
> +
> +
> +MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
> +MODULE_LICENSE("GPL");

GPL or ?..


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly

[Rasmus Villemoes]

On Mon, Sep 28 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

>>  		/*
>> -		 * Since %n poses a greater security risk than 
>> utility, treat
>> -		 * it as an invalid format specifier. Warn about its 
>> use so
>> -		 * that new instances don't get added.
>> +		 * Since %n poses a greater security risk than
>
> Any reason to wrap first string?

I just hit M-q in emacs and let that take care of somewhat sane
wrapping. I don't play diff golf.

>> +			/*
>> +			 * Presumably the arguments passed gcc's 
>> type
>> +			 * checking, but there is no safe or sane 
>> way
>> +			 * for us to continue parsing the format and
>> +			 * fetching from the va_list; the remaining
>> +			 * specifiers and arguments would be out of
>> +			 * sync.
>
> Could we use wider strings in the commentary here?

Ditto.

Rasmus

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

On Mon, Sep 28 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
>> This adds a simple module for testing the kernel's printf
>> facilities. Previously, some %p extensions have caused a wrong return
>> value in case the entire output didn't fit and/or been unusable in
>> kasprintf(). This should help catch such issues. Also, it should help
>> ensure that changes to the formatting algorithms don't break 
>> anything.
>> 
>> I'm not sure if we have a struct dentry or struct file lying around 
>> at
>> boot time or if we can fake one, but most %p extensions should be
>> testable, as should the ordinary number and string formatting.
>> 
>> The nature of vararg functions means we can't use a more conventional
>> table-driven approach.
>> 
>> For now, this is mostly a skeleton; contributions are very
>> welcome. Some tests are/will be slightly annoying to write, since the
>> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
>> values etc.
>
> Few comments below.
>
>> +
>> +#define test(expect, fmt, ...)					\
>> +	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
>
> Would be __test_m[em] / __test_s[tr] to distinguish them by name?

Erh, no. The 'mem' version will only be used in a very few cases, and I
really want the simple name "test" for the common case.

> And might be inline function?

That'd make the vararg handling more cumbersome.

>> +static void __init
>> +test_basic(void)
>> +{
>> +	test("", "");
>> +	test("100%", "100%%");
>> +	test("xxx%yyy", "xxx%cyyy", '%');
>> +	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
>
> And such pieces will be look better
>
> __test_str("xxx%yyy", "xxx%cyyy", '%');
> __test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0');

I don't agree. 

>> +
>> +static void __init
>> +netdev_features(void)
>> +{
>> +}
>> +
>> +
>
> Maybe commentary delimiter here and above where you have double empty
> line.

And say what? I can avoid double empty lines if they bother you.

>> +
>> +	return 0;
>
> Do we need this module in a memory?

I guess not. At first I thought it didn't really matter since all
functions and data are __init, but I suppose a little metadata would
stick around if loading is "successful". Will fix.

>> +
>> +MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
>> +MODULE_LICENSE("GPL");
>
> GPL or ?..

Honestly, I don't really care. Would you like BSD/GPL or what? I just
copied from the majority of MODULE_LICENSE() instances.

Rasmus

Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly

[Kees Cook]

On Fri, Sep 25, 2015 at 10:41 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> If we meet any invalid or unsupported format specifier, 'handling' it
> by just printing it as a literal string is not safe: Presumably the
> format string and the arguments passed gcc's type checking, but that
> means something like sprintf(buf, "%n %pd", &intvar, dentry) would end
> up interpreting &intvar as a struct dentry*.
>
> When the offending specifier was %n it used to be at the end of the
> format string, but we can't rely on that always being the case. Also,
> gcc doesn't complain about some more or less exotic qualifiers (or
> 'length modifiers' in posix-speak) such as 'j' or 'q', but being
> unrecognized by the kernel's printf implementation, they'd be
> interpreted as unknown specifiers, and the rest of arguments would be
> interpreted wrongly.
>
> So let's complain about anything we don't understand, not just %n, and
> stop pretending that we'd be able to make sense of the rest of the
> format/arguments. If the offending specifier is in a printk() call we
> unfortunately only get a "BUG: recent printk recursion!", but at least
> direct users of the sprintf family will be caught.

I like it! Thanks :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/4] lib/vsprintf.c: also improve sanity check in bstr_printf()

[Kees Cook]

On Fri, Sep 25, 2015 at 10:41 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Quoting from 2aa2f9e21e4e ("lib/vsprintf.c: improve sanity check in
> vsnprintf()"):
>
>     On 64 bit, size may very well be huge even if bit 31 happens to be 0.
>     Somehow it doesn't feel right that one can pass a 5 GiB buffer but not a
>     3 GiB one.  So cap at INT_MAX as was probably the intention all along.
>     This is also the made-up value passed by sprintf and vsprintf.
>
> I should have seen this copy-pasted instance back then, but let's just
> do it now.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Kees Cook]

On Fri, Sep 25, 2015 at 10:41 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> This adds a simple module for testing the kernel's printf
> facilities. Previously, some %p extensions have caused a wrong return
> value in case the entire output didn't fit and/or been unusable in
> kasprintf(). This should help catch such issues. Also, it should help
> ensure that changes to the formatting algorithms don't break anything.
>
> I'm not sure if we have a struct dentry or struct file lying around at
> boot time or if we can fake one, but most %p extensions should be
> testable, as should the ordinary number and string formatting.
>
> The nature of vararg functions means we can't use a more conventional
> table-driven approach.
>
> For now, this is mostly a skeleton; contributions are very
> welcome. Some tests are/will be slightly annoying to write, since the
> expected output depends on stuff like CONFIG_*, sizeof(long), runtime
> values etc.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/Kconfig.debug |   3 +
>  lib/Makefile      |   1 +
>  lib/test_printf.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 368 insertions(+)
>  create mode 100644 lib/test_printf.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ab76b99adc85..c23fc42dc659 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
>  config TEST_KSTRTOX
>         tristate "Test kstrto*() family of functions at runtime"
>
> +config TEST_PRINTF
> +       tristate "Test printf() family of functions at runtime"
> +
>  config TEST_RHASHTABLE
>         tristate "Perform selftest on resizable hash table"
>         default n
> diff --git a/lib/Makefile b/lib/Makefile
> index 13a7c6ae3fec..775de427ea92 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
>  obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
>  obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
> +obj-$(CONFIG_TEST_PRINTF) += test_printf.o
>
>  ifeq ($(CONFIG_DEBUG_KOBJECT),y)
>  CFLAGS_kobject.o += -DDEBUG
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> new file mode 100644
> index 000000000000..d9a2741c2909
> --- /dev/null
> +++ b/lib/test_printf.c
> @@ -0,0 +1,364 @@
> +/*
> + * Test cases for printf facility.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/random.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include <linux/socket.h>
> +#include <linux/in.h>
> +
> +#define BUF_SIZE 256
> +#define FILL_CHAR '$'
> +
> +#define PTR1 ((void*)0x01234567)
> +#define PTR2 ((void*)(long)(int)0xfedcba98)
> +
> +#if BITS_PER_LONG == 64
> +#define PTR1_ZEROES "000000000"
> +#define PTR1_SPACES "         "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fffffffffedcba98"
> +#define PTR_WIDTH 16
> +#else
> +#define PTR1_ZEROES "0"
> +#define PTR1_SPACES " "
> +#define PTR1_STR "1234567"
> +#define PTR2_STR "fedcba98"
> +#define PTR_WIDTH 8
> +#endif
> +#define PTR_WIDTH_STR stringify(PTR_WIDTH)
> +
> +static unsigned total_tests __initdata;
> +static unsigned failed_tests __initdata;
> +static char *test_buffer __initdata;
> +
> +static int __printf(4, 0) __init
> +do_test(int bufsize, const char *expect, int elen,
> +       const char *fmt, va_list ap)
> +{
> +       va_list aq;
> +       int ret, written;
> +
> +       total_tests++;
> +
> +       memset(test_buffer, FILL_CHAR, BUF_SIZE);
> +       va_copy(aq, ap);
> +       ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> +       va_end(aq);
> +
> +       if (ret != elen) {
> +               pr_warn("bad return value, expected %d, got %d, format was '%s'\n",
> +                       elen, ret, fmt);
> +               return 1;
> +       }
> +
> +       if (!bufsize) {
> +               if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
> +                       pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
> +                               fmt);
> +                       return 1;
> +               }
> +               return 0;
> +       }
> +
> +       written = min(bufsize-1, elen);
> +       if (test_buffer[written]) {
> +               pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
> +                       bufsize, fmt);
> +               return 1;
> +       }
> +
> +       if (memcmp(test_buffer, expect, written)) {
> +               pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
> +                       bufsize, fmt, test_buffer, written, expect);
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +
> +static void __printf(3, 4) __init
> +__test(const char *expect, int elen, const char *fmt, ...)
> +{
> +       va_list ap;
> +       int rand;
> +       char *p;
> +
> +       BUG_ON(elen >= BUF_SIZE);
> +
> +       va_start(ap, fmt);
> +
> +       /*
> +        * Every fmt+args is subjected to four tests: Three where we
> +        * tell vsnprintf varying buffer sizes (plenty, not quite
> +        * enough and 0), and then we also test that kvasprintf would
> +        * be able to print it as expected.
> +        */
> +       failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
> +       rand = 1 + prandom_u32_max(elen+1);
> +       /* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
> +       failed_tests += do_test(rand, expect, elen, fmt, ap);
> +       failed_tests += do_test(0, expect, elen, fmt, ap);
> +
> +       p = kvasprintf(GFP_KERNEL, fmt, ap);
> +       if (p) {
> +               if (memcmp(p, expect, elen+1)) {
> +                       pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
> +                               fmt, p, expect);
> +                       failed_tests++;
> +               }
> +               kfree(p);
> +       }
> +       va_end(ap);
> +}
> +
> +#define test(expect, fmt, ...)                                 \
> +       __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
> +
> +
> +static void __init
> +test_basic(void)
> +{
> +       test("", "");
> +       test("100%", "100%%");
> +       test("xxx%yyy", "xxx%cyyy", '%');
> +       __test("xxx\0yyy", 7, "xxx%cyyy", '\0');
> +}
> +
> +static void __init
> +test_number(void)
> +{
> +       test("0x1234abcd  ", "%#-12x", 0x1234abcd);
> +       test("  0x1234abcd", "%#12x", 0x1234abcd);
> +       test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
> +}
> +
> +static void __init
> +test_string(void)
> +{
> +       test("", "%s%.0s", "", "123");
> +       test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
> +       test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
> +       /*
> +        * POSIX and C99 say that a missing precision should be
> +        * treated as a precision of 0. However, the kernel's printf
> +        * implementation treats this case as if the . wasn't
> +        * present. Let's add a test case documenting the current
> +        * behaviour; should anyone ever feel the need to follow the
> +        * standards more closely, this can be revisited.
> +        */
> +       test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
> +       test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
> +}

Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
change) as well?

> +
> +
> +static void __init
> +plain(void)
> +{
> +       test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
> +       /*
> +        * The field width is overloaded for some %p extensions to
> +        * pass another piece of information. For plain pointers, the
> +        * behaviour is slightly odd: One cannot pass either the 0
> +        * flag nor a precition to %p without gcc complaining, and if
> +        * one explicitly gives a field width, the number is no longer
> +        * zero-padded.
> +        */
> +       test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
> +            "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
> +       test("|" PTR2_STR "  |  " PTR2_STR "|",
> +            "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
> +
> +       /*
> +        * Unrecognized %p extensions are treated as plain %p, but the
> +        * alphanumeric suffix is ignored (that is, does not occur in
> +        * the output.)
> +        */
> +       test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
> +       test("|"PTR2_STR"|", "|%p0y|", PTR2);
> +}
> +
> +static void __init
> +symbol_ptr(void)
> +{
> +}
> +
> +static void __init
> +kernel_ptr(void)
> +{
> +}
> +
> +static void __init
> +struct_resource(void)
> +{
> +}
> +
> +static void __init
> +addr(void)
> +{
> +}
> +
> +static void __init
> +escaped_str(void)
> +{
> +}
> +
> +static void __init
> +hex_string(void)
> +{
> +       const char buf[3] = {0xc0, 0xff, 0xee};
> +
> +       test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
> +            "%3ph|%3phC|%3phD|%3phN", buf, buf, buf, buf);
> +       test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
> +            "%*ph|%*phC|%*phD|%*phN", 3, buf, 3, buf, 3, buf, 3, buf);
> +}
> +
> +static void __init
> +mac(void)
> +{
> +       const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
> +
> +       test("2d:48:d6:fc:7a:05", "%pM", addr);
> +       test("05:7a:fc:d6:48:2d", "%pMR", addr);
> +       test("2d-48-d6-fc-7a-05", "%pMF", addr);
> +       test("2d48d6fc7a05", "%pm", addr);
> +       test("057afcd6482d", "%pmR", addr);
> +}
> +
> +static void __init
> +ip4(void)
> +{
> +       struct sockaddr_in sa;
> +
> +       sa.sin_family = AF_INET;
> +       sa.sin_port = cpu_to_be16(12345);
> +       sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
> +
> +       test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
> +       test("127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
> +       sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
> +       test("001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);
> +}
> +
> +static void __init
> +ip6(void)
> +{
> +}
> +
> +static void __init
> +ip(void)
> +{
> +       ip4();
> +       ip6();
> +}
> +
> +static void __init
> +uuid(void)
> +{
> +       const char uuid[16] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
> +                              0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf};
> +
> +       test("00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid);
> +       test("00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid);
> +       test("03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid);
> +       test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
> +}
> +
> +static void __init
> +dentry(void)
> +{
> +}
> +
> +static void __init
> +struct_va_format(void)
> +{
> +}
> +
> +static void __init
> +struct_clk(void)
> +{
> +}

For the empty functions, maybe just add a pr_info("TODO: struct_clk")
or something?

> +
> +static void __init
> +bitmap(void)
> +{
> +       DECLARE_BITMAP(bits, 20);
> +       const int primes[] = {2,3,5,7,11,13,17,19};
> +       int i;
> +
> +       bitmap_zero(bits, 20);
> +       test("00000|00000", "%20pb|%*pb", bits, 20, bits);
> +       test("|", "%20pbl|%*pbl", bits, 20, bits);
> +
> +       for (i = 0; i < ARRAY_SIZE(primes); ++i)
> +               set_bit(primes[i], bits);
> +       test("a28ac|a28ac", "%20pb|%*pb", bits, 20, bits);
> +       test("2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", "%20pbl|%*pbl", bits, 20, bits);
> +
> +       bitmap_fill(bits, 20);
> +       test("fffff|fffff", "%20pb|%*pb", bits, 20, bits);
> +       test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
> +}
> +
> +static void __init
> +netdev_features(void)
> +{
> +}
> +
> +
> +static void __init
> +test_pointer(void)
> +{
> +       plain();
> +       symbol_ptr();
> +       kernel_ptr();
> +       struct_resource();
> +       addr();
> +       escaped_str();
> +       hex_string();
> +       mac();
> +       ip();
> +       uuid();
> +       dentry();
> +       struct_va_format();
> +       struct_clk();
> +       bitmap();
> +       netdev_features();
> +}
> +
> +static int __init
> +test_printf_init(void)
> +{
> +       test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
> +       if (!test_buffer)
> +               return -ENOMEM;
> +
> +       test_basic();
> +       test_number();
> +       test_string();
> +       test_pointer();
> +
> +       kfree(test_buffer);
> +
> +       if (failed_tests == 0)
> +               pr_info("all %u tests passed\n", total_tests);
> +       else
> +               pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
> +
> +       return 0;
> +}

I actually have different feedback on leaving the module loaded: I
think it should succeed to load when the tests pass and fail when they
don't. This makes it a one-step test to check things ("what is
modprobe's return code?"), instead of needed to parse dmesg.

I love tests! Thank you. :) One suggestion would be to wire it up to
the tools/testing/selftests tree; it should be trivial once you change
the test_printf_init return code.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> +
> +module_init(test_printf_init);
> +
> +
> +MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
> +MODULE_LICENSE("GPL");
> --
> 2.1.3
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

On Tue, Sep 29 2015, Kees Cook <keescook@chromium.org> wrote:

>> +static void __init
>> +test_string(void)
>> +{
>> +       test("", "%s%.0s", "", "123");
>> +       test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
>> +       test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
>> +       /*
>> +        * POSIX and C99 say that a missing precision should be
>> +        * treated as a precision of 0. However, the kernel's printf
>> +        * implementation treats this case as if the . wasn't
>> +        * present. Let's add a test case documenting the current
>> +        * behaviour; should anyone ever feel the need to follow the
>> +        * standards more closely, this can be revisited.
>> +        */
>> +       test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
>> +       test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>> +}
>
> Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
> change) as well?

I suppose you'd also want checks for the somewhat more important
vsnprintf size check and unknown specifiers? I guess I could, but do we
really want to intentionally trigger WARN_ON_ONCEs? Say some distro
chooses to load this module at boot time, then we'd both spam the kernel
log with "false positives", and we'd have effectively disabled the
WARN_ON_ONCEs for the actual kernel code.

Maybe we can hide such things behind some module parameter, so that the
user explicitly has to ask for them. Also, we can't really probe the
"success" if these sanity checks from within the module (can we?) - the
user would have to check dmesg manually anyway.

>> +
>> +static void __init
>> +dentry(void)
>> +{
>> +}
>> +
>> +static void __init
>> +struct_va_format(void)
>> +{
>> +}
>> +
>> +static void __init
>> +struct_clk(void)
>> +{
>> +}
>
> For the empty functions, maybe just add a pr_info("TODO: struct_clk")
> or something?

I think that would be unnecessarily spammy. It should be obvious from
the code that it is just waiting for someone to fill in the blanks.

>> +static int __init
>> +test_printf_init(void)
>> +{
>> +       test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
>> +       if (!test_buffer)
>> +               return -ENOMEM;
>> +
>> +       test_basic();
>> +       test_number();
>> +       test_string();
>> +       test_pointer();
>> +
>> +       kfree(test_buffer);
>> +
>> +       if (failed_tests == 0)
>> +               pr_info("all %u tests passed\n", total_tests);
>> +       else
>> +               pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
>> +
>> +       return 0;
>> +}
>
> I actually have different feedback on leaving the module loaded: I
> think it should succeed to load when the tests pass and fail when they
> don't. This makes it a one-step test to check things ("what is
> modprobe's return code?"), instead of needed to parse dmesg.

Hm, I guess that makes sense. But, assuming we go with the module param
suggested above, would it be possible to (unload and) load with a
different set of parameters? 

> I love tests! Thank you. :) One suggestion would be to wire it up to
> the tools/testing/selftests tree; it should be trivial once you change
> the test_printf_init return code.

I'll look into that. Not sure I have too much time to work on this this
side of the merge window, and since these all seem to be things that can
be incrementally added, I'd prefer seeing something go into 4.4 instead
of waiting till it's "perfect". So unless I hear otherwise, I'll post a
v2 with the minor things addressed and ask Andrew to take that through
-mm.

Thanks,
Rasmus

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Kees Cook]

On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Tue, Sep 29 2015, Kees Cook <keescook@chromium.org> wrote:
>
>>> +static void __init
>>> +test_string(void)
>>> +{
>>> +       test("", "%s%.0s", "", "123");
>>> +       test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
>>> +       test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
>>> +       /*
>>> +        * POSIX and C99 say that a missing precision should be
>>> +        * treated as a precision of 0. However, the kernel's printf
>>> +        * implementation treats this case as if the . wasn't
>>> +        * present. Let's add a test case documenting the current
>>> +        * behaviour; should anyone ever feel the need to follow the
>>> +        * standards more closely, this can be revisited.
>>> +        */
>>> +       test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
>>> +       test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
>>> +}
>>
>> Could you add a test for your 2/4 patch (bstr_printf size > INT_MAX
>> change) as well?
>
> I suppose you'd also want checks for the somewhat more important
> vsnprintf size check and unknown specifiers? I guess I could, but do we
> really want to intentionally trigger WARN_ON_ONCEs? Say some distro
> chooses to load this module at boot time, then we'd both spam the kernel
> log with "false positives", and we'd have effectively disabled the
> WARN_ON_ONCEs for the actual kernel code.

Distros don't tend to run the test modules by default. The most common
case is that it's part of a selftests run, in which case the machine
has usually been freshly booted, etc. I think it's more important to
catch regressions.

> Maybe we can hide such things behind some module parameter, so that the
> user explicitly has to ask for them. Also, we can't really probe the
> "success" if these sanity checks from within the module (can we?) - the
> user would have to check dmesg manually anyway.

I think it's best that tests run with as few options as possible.
Surely we can test the behavior? The bstr returns 0, so the string
should be truncated? I haven't looked closely, but it seemed testable.

>
>>> +
>>> +static void __init
>>> +dentry(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_va_format(void)
>>> +{
>>> +}
>>> +
>>> +static void __init
>>> +struct_clk(void)
>>> +{
>>> +}
>>
>> For the empty functions, maybe just add a pr_info("TODO: struct_clk")
>> or something?
>
> I think that would be unnecessarily spammy. It should be obvious from
> the code that it is just waiting for someone to fill in the blanks.

Ok.

>
>>> +static int __init
>>> +test_printf_init(void)
>>> +{
>>> +       test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
>>> +       if (!test_buffer)
>>> +               return -ENOMEM;
>>> +
>>> +       test_basic();
>>> +       test_number();
>>> +       test_string();
>>> +       test_pointer();
>>> +
>>> +       kfree(test_buffer);
>>> +
>>> +       if (failed_tests == 0)
>>> +               pr_info("all %u tests passed\n", total_tests);
>>> +       else
>>> +               pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
>>> +
>>> +       return 0;
>>> +}
>>
>> I actually have different feedback on leaving the module loaded: I
>> think it should succeed to load when the tests pass and fail when they
>> don't. This makes it a one-step test to check things ("what is
>> modprobe's return code?"), instead of needed to parse dmesg.
>
> Hm, I guess that makes sense. But, assuming we go with the module param
> suggested above, would it be possible to (unload and) load with a
> different set of parameters?

Sure, you've freed memory already, it should be entirely safe to
unload (which is what the selftests script should do anyway). I still
don't think it should have options, though.

>> I love tests! Thank you. :) One suggestion would be to wire it up to
>> the tools/testing/selftests tree; it should be trivial once you change
>> the test_printf_init return code.
>
> I'll look into that. Not sure I have too much time to work on this this
> side of the merge window, and since these all seem to be things that can
> be incrementally added, I'd prefer seeing something go into 4.4 instead
> of waiting till it's "perfect". So unless I hear otherwise, I'll post a
> v2 with the minor things addressed and ask Andrew to take that through
> -mm.

I'll send the glue patch...

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Andy Shevchenko]

On Mon, 2015-09-28 at 22:55 +0200, Rasmus Villemoes wrote:
> On Mon, Sep 28 2015, Andy Shevchenko <
> andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> > > This adds a simple module for testing the kernel's printf
> > > facilities. Previously, some %p extensions have caused a wrong 
> > > return
> > > value in case the entire output didn't fit and/or been unusable 
> > > in
> > > kasprintf(). This should help catch such issues. Also, it should 
> > > help
> > > ensure that changes to the formatting algorithms don't break 
> > > anything.
> > > 
> > > I'm not sure if we have a struct dentry or struct file lying 
> > > around 
> > > at
> > > boot time or if we can fake one, but most %p extensions should be
> > > testable, as should the ordinary number and string formatting.
> > > 
> > > The nature of vararg functions means we can't use a more 
> > > conventional
> > > table-driven approach.
> > > 
> > > For now, this is mostly a skeleton; contributions are very
> > > welcome. Some tests are/will be slightly annoying to write, since 
> > > the
> > > expected output depends on stuff like CONFIG_*, sizeof(long), 
> > > runtime
> > > values etc.
> > 
> > Few comments below.


> > > +#define test(expect, fmt, ...)				
> > > \
> > > +	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
> > 
> > Would be __test_m[em] / __test_s[tr] to distinguish them by name?
> 
> Erh, no. The 'mem' version will only be used in a very few cases, and 
> I
> really want the simple name "test" for the common case.

> > > +static void __init
> > > +test_basic(void)
> > > +{
> > > +	test("", "");
> > > +	test("100%", "100%%");
> > > +	test("xxx%yyy", "xxx%cyyy", '%');
> > > +	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
> > 
> > And such pieces will be look better
> > 
> > __test_str("xxx%yyy", "xxx%cyyy", '%');
> > __test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0');
> 
> I don't agree. 

It's still better to distinguish what function does by names
test vs. __test confuses me.

But whatever, this is a test suite, not an actual code anyway.

> >Maybe commentary delimiter here and above where you have double
> >empty
> >line.
> 
> And say what? I can avoid double empty lines if they bother you.

So, what was the reason to add them in the first place?

> > > +
> > > +MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
> > > +MODULE_LICENSE("GPL");
> > 
> > GPL or ?..
> 
> Honestly, I don't really care. Would you like BSD/GPL or what? I just
> copied from the majority of MODULE_LICENSE() instances.

You are the author, your choice. I'm okay with any applicable type.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

On Wed, Sep 30 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> It's still better to distinguish what function does by names
> test vs. __test confuses me.

Think of test() as the ordinary "public" interface and __test() as
"private, but we expose it because you might need it, if you know what
you're doing". Having two related functions named like this is a
perfectly normal pattern in the kernel (even if one isn't necessarily
implemented in terms of the other); examples are
legio. krealloc/__krealloc (the latter is not called krealloc_nofree),
fls/__fls (not fls_I_know_word_is_nonzero, thank $deity), ...

>> >Maybe commentary delimiter here and above where you have double
>> >empty line.
>> 
>> And say what? I can avoid double empty lines if they bother you.
>
> So, what was the reason to add them in the first place?

Just so we could have this interesting little chat. I don't think it was
intentional, but don't worry, they're gone.

>> > > +MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
>> > > +MODULE_LICENSE("GPL");
>> > 
>> > GPL or ?..
>> 
>> Honestly, I don't really care. Would you like BSD/GPL or what? I just
>> copied from the majority of MODULE_LICENSE() instances.
>
> You are the author, your choice. I'm okay with any applicable type.

Then I'll stick to GPL, even if that's more cargo-culting than
deliberate informed choice.

Rasmus

Re: [PATCH 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

On Tue, Sep 29 2015, Kees Cook <keescook@chromium.org> wrote:

> On Tue, Sep 29, 2015 at 12:10 AM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>
>> I guess I could, but do we really want to intentionally trigger
>> WARN_ON_ONCEs? Say some distro chooses to load this module at boot
>> time, then we'd both spam the kernel log with "false positives", and
>> we'd have effectively disabled the WARN_ON_ONCEs for the actual
>> kernel code.
>
> Distros don't tend to run the test modules by default. The most common
> case is that it's part of a selftests run, in which case the machine
> has usually been freshly booted, etc. I think it's more important to
> catch regressions.
>
>> Maybe we can hide such things behind some module parameter, so that the
>> user explicitly has to ask for them. Also, we can't really probe the
>> "success" if these sanity checks from within the module (can we?) - the
>> user would have to check dmesg manually anyway.
>
> I think it's best that tests run with as few options as possible.
> Surely we can test the behavior? The bstr returns 0, so the string
> should be truncated? I haven't looked closely, but it seemed testable.

Well, yes, obviously we can check that part, but I also think it would
be nice to check that it actually resulted in a warning, which is what I
think would require manual inspection.

I'm still not convinced intentionally triggering a WARN on module load
is a good idea, even if the module wouldn't be loaded by normal
distros. Especially because of the _ONCE part, so that actual bugs might
not be warned about for the rest of that boot's lifetime. I'd certainly
like to hear what others think about this.

>>> I love tests! Thank you. :) One suggestion would be to wire it up to
>>> the tools/testing/selftests tree; it should be trivial once you change
>>> the test_printf_init return code.
>>
>> I'll look into that. Not sure I have too much time to work on this this
>> side of the merge window, and since these all seem to be things that can
>> be incrementally added, I'd prefer seeing something go into 4.4 instead
>> of waiting till it's "perfect". So unless I hear otherwise, I'll post a
>> v2 with the minor things addressed and ask Andrew to take that through
>> -mm.
>
> I'll send the glue patch...

Thanks. v2 coming up. For now I'll just change the return code; we can
always add tests for the sanity checks later, when we figure out the
best way to do them.

Rasmus

[PATCH v2 0/4] printf stuff

[Rasmus Villemoes]

A few printf-related patches I've been sitting on. I also have some
documentation updates, but I'll wait until I see Martin's patch [1] in
-next. There's also the %pb issue [2], but I'm not sure there's consensus
on the best fix for that.

[1] https://lkml.org/lkml/2015/9/24/256
[2] https://lkml.org/lkml/2015/9/16/769

Rasmus Villemoes (4):
  lib/vsprintf.c: handle invalid format specifiers more robustly
  lib/vsprintf.c: also improve sanity check in bstr_printf()
  lib/vsprintf.c: Remove SPECIAL handling in pointer()
  test_printf: test printf family at runtime

 lib/Kconfig.debug |   3 +
 lib/Makefile      |   1 +
 lib/test_printf.c | 362 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c    |  35 ++++--
 4 files changed, 389 insertions(+), 12 deletions(-)
 create mode 100644 lib/test_printf.c

-- 
2.1.3

[PATCH v2 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly

[Rasmus Villemoes]

If we meet any invalid or unsupported format specifier, 'handling' it
by just printing it as a literal string is not safe: Presumably the
format string and the arguments passed gcc's type checking, but that
means something like sprintf(buf, "%n %pd", &intvar, dentry) would end
up interpreting &intvar as a struct dentry*.

When the offending specifier was %n it used to be at the end of the
format string, but we can't rely on that always being the case. Also,
gcc doesn't complain about some more or less exotic qualifiers (or
'length modifiers' in posix-speak) such as 'j' or 'q', but being
unrecognized by the kernel's printf implementation, they'd be
interpreted as unknown specifiers, and the rest of arguments would be
interpreted wrongly.

So let's complain about anything we don't understand, not just %n, and
stop pretending that we'd be able to make sense of the rest of the
format/arguments. If the offending specifier is in a printk() call we
unfortunately only get a "BUG: recent printk recursion!", but at least
direct users of the sprintf family will be caught.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2: add acked/reviewed-bys.

 lib/vsprintf.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b43b99..f2590a80937f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1769,14 +1769,14 @@ qualifier:
 
 	case 'n':
 		/*
-		 * Since %n poses a greater security risk than utility, treat
-		 * it as an invalid format specifier. Warn about its use so
-		 * that new instances don't get added.
+		 * Since %n poses a greater security risk than
+		 * utility, treat it as any other invalid or
+		 * unsupported format specifier.
 		 */
-		WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
 		/* Fall-through */
 
 	default:
+		WARN_ONCE(1, "Please remove unsupported %%%c in format string\n", *fmt);
 		spec->type = FORMAT_TYPE_INVALID;
 		return fmt - start;
 	}
@@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_INVALID:
-			if (str < end)
-				*str = '%';
-			++str;
-			break;
+			/*
+			 * Presumably the arguments passed gcc's type
+			 * checking, but there is no safe or sane way
+			 * for us to continue parsing the format and
+			 * fetching from the va_list; the remaining
+			 * specifiers and arguments would be out of
+			 * sync.
+			 */
+			goto out;
 
 		default:
 			switch (spec.type) {
@@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		}
 	}
 
+out:
 	if (size > 0) {
 		if (str < end)
 			*str = '\0';
@@ -2189,9 +2195,10 @@ do {									\
 
 		switch (spec.type) {
 		case FORMAT_TYPE_NONE:
-		case FORMAT_TYPE_INVALID:
 		case FORMAT_TYPE_PERCENT_CHAR:
 			break;
+		case FORMAT_TYPE_INVALID:
+			goto out;
 
 		case FORMAT_TYPE_WIDTH:
 		case FORMAT_TYPE_PRECISION:
@@ -2253,6 +2260,7 @@ do {									\
 		}
 	}
 
+out:
 	return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf;
 #undef save_arg
 }
@@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-		case FORMAT_TYPE_INVALID:
 			if (str < end)
 				*str = '%';
 			++str;
 			break;
 
+		case FORMAT_TYPE_INVALID:
+			goto out;
+
 		default: {
 			unsigned long long num;
 
@@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 		} /* switch(spec.type) */
 	} /* while(*fmt) */
 
+out:
 	if (size > 0) {
 		if (str < end)
 			*str = '\0';
-- 
2.1.3

[PATCH v2 2/4] lib/vsprintf.c: also improve sanity check in bstr_printf()

[Rasmus Villemoes]

Quoting from 2aa2f9e21e4e ("lib/vsprintf.c: improve sanity check in
vsnprintf()"):

    On 64 bit, size may very well be huge even if bit 31 happens to be 0.
    Somehow it doesn't feel right that one can pass a 5 GiB buffer but not a
    3 GiB one.  So cap at INT_MAX as was probably the intention all along.
    This is also the made-up value passed by sprintf and vsprintf.

I should have seen this copy-pasted instance back then, but let's just
do it now.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2: add ack.

 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f2590a80937f..03fa10b4be96 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2294,7 +2294,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 	char *str, *end;
 	const char *args = (const char *)bin_buf;
 
-	if (WARN_ON_ONCE((int) size < 0))
+	if (WARN_ON_ONCE(size > INT_MAX))
 		return 0;
 
 	str = buf;
-- 
2.1.3

[PATCH v2 3/4] lib/vsprintf.c: Remove SPECIAL handling in pointer()

[Rasmus Villemoes]

As a quick

   git grep -E '%[ +0#-]*#[ +0#-]*(\*|[0-9]+)?(\.(\*|[0-9]+)?)?p'

shows, nobody uses the # flag with %p. Should one try to do so, one
will be met with

  warning: `#' flag used with ‘%p’ gnu_printf format [-Wformat]

(POSIX and C99 both say "... For other conversion specifiers, the
behavior is undefined.". Obviously, the kernel can choose to define
the behaviour however it wants, but as long as gcc issues that
warning, users are unlikely to show up.)

Since default_width is effectively always 2*sizeof(void*), we can
simplify the prologue of pointer() and save a few instructions.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2: reword slightly, refer to POSIX/C99.

 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 03fa10b4be96..98b0d7be3fb7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1457,7 +1457,7 @@ static noinline_for_stack
 char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
-	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
+	const int default_width = 2 * sizeof(void *);
 
 	if (!ptr && *fmt != 'K') {
 		/*
-- 
2.1.3

[PATCH v2 4/4] test_printf: test printf family at runtime

[Rasmus Villemoes]

This adds a simple module for testing the kernel's printf
facilities. Previously, some %p extensions have caused a wrong return
value in case the entire output didn't fit and/or been unusable in
kasprintf(). This should help catch such issues. Also, it should help
ensure that changes to the formatting algorithms don't break anything.

I'm not sure if we have a struct dentry or struct file lying around at
boot time or if we can fake one, but most %p extensions should be
testable, as should the ordinary number and string formatting.

The nature of vararg functions means we can't use a more conventional
table-driven approach.

For now, this is mostly a skeleton; contributions are very
welcome. Some tests are/will be slightly annoying to write, since the
expected output depends on stuff like CONFIG_*, sizeof(long), runtime
values etc.

Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2: fix some cosmetic issues, avoid annoying gcc warning for empty
format string, return success if and only if all tests passed, add
reviewed-by.

 lib/Kconfig.debug |   3 +
 lib/Makefile      |   1 +
 lib/test_printf.c | 362 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 lib/test_printf.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ab76b99adc85..c23fc42dc659 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1685,6 +1685,9 @@ config TEST_STRING_HELPERS
 config TEST_KSTRTOX
 	tristate "Test kstrto*() family of functions at runtime"
 
+config TEST_PRINTF
+	tristate "Test printf() family of functions at runtime"
+
 config TEST_RHASHTABLE
 	tristate "Perform selftest on resizable hash table"
 	default n
diff --git a/lib/Makefile b/lib/Makefile
index 13a7c6ae3fec..775de427ea92 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
+obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_printf.c b/lib/test_printf.c
new file mode 100644
index 000000000000..c5a666af9ba5
--- /dev/null
+++ b/lib/test_printf.c
@@ -0,0 +1,362 @@
+/*
+ * Test cases for printf facility.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/random.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include <linux/socket.h>
+#include <linux/in.h>
+
+#define BUF_SIZE 256
+#define FILL_CHAR '$'
+
+#define PTR1 ((void*)0x01234567)
+#define PTR2 ((void*)(long)(int)0xfedcba98)
+
+#if BITS_PER_LONG == 64
+#define PTR1_ZEROES "000000000"
+#define PTR1_SPACES "         "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fffffffffedcba98"
+#define PTR_WIDTH 16
+#else
+#define PTR1_ZEROES "0"
+#define PTR1_SPACES " "
+#define PTR1_STR "1234567"
+#define PTR2_STR "fedcba98"
+#define PTR_WIDTH 8
+#endif
+#define PTR_WIDTH_STR stringify(PTR_WIDTH)
+
+static unsigned total_tests __initdata;
+static unsigned failed_tests __initdata;
+static char *test_buffer __initdata;
+
+static int __printf(4, 0) __init
+do_test(int bufsize, const char *expect, int elen,
+	const char *fmt, va_list ap)
+{
+	va_list aq;
+	int ret, written;
+
+	total_tests++;
+
+	memset(test_buffer, FILL_CHAR, BUF_SIZE);
+	va_copy(aq, ap);
+	ret = vsnprintf(test_buffer, bufsize, fmt, aq);
+	va_end(aq);
+
+	if (ret != elen) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
+			bufsize, fmt, ret, elen);
+		return 1;
+	}
+
+	if (!bufsize) {
+		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
+			pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
+				fmt);
+			return 1;
+		}
+		return 0;
+	}
+
+	written = min(bufsize-1, elen);
+	if (test_buffer[written]) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
+			bufsize, fmt);
+		return 1;
+	}
+
+	if (memcmp(test_buffer, expect, written)) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
+			bufsize, fmt, test_buffer, written, expect);
+		return 1;
+	}
+	return 0;
+}
+
+static void __printf(3, 4) __init
+__test(const char *expect, int elen, const char *fmt, ...)
+{
+	va_list ap;
+	int rand;
+	char *p;
+
+	BUG_ON(elen >= BUF_SIZE);
+
+	va_start(ap, fmt);
+
+	/*
+	 * Every fmt+args is subjected to four tests: Three where we
+	 * tell vsnprintf varying buffer sizes (plenty, not quite
+	 * enough and 0), and then we also test that kvasprintf would
+	 * be able to print it as expected.
+	 */
+	failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
+	rand = 1 + prandom_u32_max(elen+1);
+	/* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
+	failed_tests += do_test(rand, expect, elen, fmt, ap);
+	failed_tests += do_test(0, expect, elen, fmt, ap);
+
+	p = kvasprintf(GFP_KERNEL, fmt, ap);
+	if (p) {
+		if (memcmp(p, expect, elen+1)) {
+			pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
+				fmt, p, expect);
+			failed_tests++;
+		}
+		kfree(p);
+	}
+	va_end(ap);
+}
+
+#define test(expect, fmt, ...)					\
+	__test(expect, strlen(expect), fmt, ##__VA_ARGS__)
+
+static void __init
+test_basic(void)
+{
+	/* Work around annoying "warning: zero-length gnu_printf format string". */
+	char nul = '\0';
+
+	test("", &nul);
+	test("100%", "100%%");
+	test("xxx%yyy", "xxx%cyyy", '%');
+	__test("xxx\0yyy", 7, "xxx%cyyy", '\0');
+}
+
+static void __init
+test_number(void)
+{
+	test("0x1234abcd  ", "%#-12x", 0x1234abcd);
+	test("  0x1234abcd", "%#12x", 0x1234abcd);
+	test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234);
+}
+
+static void __init
+test_string(void)
+{
+	test("", "%s%.0s", "", "123");
+	test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456");
+	test("1  |  2|3  |  4|5  ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5");
+	/*
+	 * POSIX and C99 say that a missing precision should be
+	 * treated as a precision of 0. However, the kernel's printf
+	 * implementation treats this case as if the . wasn't
+	 * present. Let's add a test case documenting the current
+	 * behaviour; should anyone ever feel the need to follow the
+	 * standards more closely, this can be revisited.
+	 */
+	test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
+	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
+}
+
+static void __init
+plain(void)
+{
+	test(PTR1_ZEROES PTR1_STR " " PTR2_STR, "%p %p", PTR1, PTR2);
+	/*
+	 * The field width is overloaded for some %p extensions to
+	 * pass another piece of information. For plain pointers, the
+	 * behaviour is slightly odd: One cannot pass either the 0
+	 * flag nor a precision to %p without gcc complaining, and if
+	 * one explicitly gives a field width, the number is no longer
+	 * zero-padded.
+	 */
+	test("|" PTR1_STR PTR1_SPACES "  |  " PTR1_SPACES PTR1_STR "|",
+	     "|%-*p|%*p|", PTR_WIDTH+2, PTR1, PTR_WIDTH+2, PTR1);
+	test("|" PTR2_STR "  |  " PTR2_STR "|",
+	     "|%-*p|%*p|", PTR_WIDTH+2, PTR2, PTR_WIDTH+2, PTR2);
+
+	/*
+	 * Unrecognized %p extensions are treated as plain %p, but the
+	 * alphanumeric suffix is ignored (that is, does not occur in
+	 * the output.)
+	 */
+	test("|"PTR1_ZEROES PTR1_STR"|", "|%p0y|", PTR1);
+	test("|"PTR2_STR"|", "|%p0y|", PTR2);
+}
+
+static void __init
+symbol_ptr(void)
+{
+}
+
+static void __init
+kernel_ptr(void)
+{
+}
+
+static void __init
+struct_resource(void)
+{
+}
+
+static void __init
+addr(void)
+{
+}
+
+static void __init
+escaped_str(void)
+{
+}
+
+static void __init
+hex_string(void)
+{
+	const char buf[3] = {0xc0, 0xff, 0xee};
+
+	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
+	     "%3ph|%3phC|%3phD|%3phN", buf, buf, buf, buf);
+	test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee",
+	     "%*ph|%*phC|%*phD|%*phN", 3, buf, 3, buf, 3, buf, 3, buf);
+}
+
+static void __init
+mac(void)
+{
+	const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
+
+	test("2d:48:d6:fc:7a:05", "%pM", addr);
+	test("05:7a:fc:d6:48:2d", "%pMR", addr);
+	test("2d-48-d6-fc-7a-05", "%pMF", addr);
+	test("2d48d6fc7a05", "%pm", addr);
+	test("057afcd6482d", "%pmR", addr);
+}
+
+static void __init
+ip4(void)
+{
+	struct sockaddr_in sa;
+
+	sa.sin_family = AF_INET;
+	sa.sin_port = cpu_to_be16(12345);
+	sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
+
+	test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+	test("127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
+	sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
+	test("001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);
+}
+
+static void __init
+ip6(void)
+{
+}
+
+static void __init
+ip(void)
+{
+	ip4();
+	ip6();
+}
+
+static void __init
+uuid(void)
+{
+	const char uuid[16] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
+			       0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf};
+
+	test("00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid);
+	test("00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid);
+	test("03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid);
+	test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
+}
+
+static void __init
+dentry(void)
+{
+}
+
+static void __init
+struct_va_format(void)
+{
+}
+
+static void __init
+struct_clk(void)
+{
+}
+
+static void __init
+bitmap(void)
+{
+	DECLARE_BITMAP(bits, 20);
+	const int primes[] = {2,3,5,7,11,13,17,19};
+	int i;
+
+	bitmap_zero(bits, 20);
+	test("00000|00000", "%20pb|%*pb", bits, 20, bits);
+	test("|", "%20pbl|%*pbl", bits, 20, bits);
+
+	for (i = 0; i < ARRAY_SIZE(primes); ++i)
+		set_bit(primes[i], bits);
+	test("a28ac|a28ac", "%20pb|%*pb", bits, 20, bits);
+	test("2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", "%20pbl|%*pbl", bits, 20, bits);
+
+	bitmap_fill(bits, 20);
+	test("fffff|fffff", "%20pb|%*pb", bits, 20, bits);
+	test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
+}
+
+static void __init
+netdev_features(void)
+{
+}
+
+static void __init
+test_pointer(void)
+{
+	plain();
+	symbol_ptr();
+	kernel_ptr();
+	struct_resource();
+	addr();
+	escaped_str();
+	hex_string();
+	mac();
+	ip();
+	uuid();
+	dentry();
+	struct_va_format();
+	struct_clk();
+	bitmap();
+	netdev_features();
+}
+
+static int __init
+test_printf_init(void)
+{
+	test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
+	if (!test_buffer)
+		return -ENOMEM;
+
+	test_basic();
+	test_number();
+	test_string();
+	test_pointer();
+
+	kfree(test_buffer);
+
+	if (failed_tests == 0)
+		pr_info("all %u tests passed\n", total_tests);
+	else
+		pr_warn("failed %u out of %u tests\n", failed_tests, total_tests);
+
+	return failed_tests ? -EINVAL : 0;
+}
+
+module_init(test_printf_init);
+
+MODULE_AUTHOR("Rasmus Villemoes <linux@rasmusvillemoes.dk>");
+MODULE_LICENSE("GPL");
-- 
2.1.3

Re: [PATCH v2 3/4] lib/vsprintf.c: Remove SPECIAL handling in pointer()

[Andy Shevchenko]

On Wed, 2015-09-30 at 17:30 +0200, Rasmus Villemoes wrote:
> As a quick
> 
>    git grep -E '%[ +0#-]*#[ +0#-]*(\*|[0-9]+)?(\.(\*|[0-9]+)?)?p'
> 
> shows, nobody uses the # flag with %p. Should one try to do so, one
> will be met with
> 
>   warning: `#' flag used with ‘%p’ gnu_printf format [-Wformat]
> 
> (POSIX and C99 both say "... For other conversion specifiers, the
> behavior is undefined.". Obviously, the kernel can choose to define
> the behaviour however it wants, but as long as gcc issues that
> warning, users are unlikely to show up.)
> 
> Since default_width is effectively always 2*sizeof(void*), we can
> simplify the prologue of pointer() and save a few instructions.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> v2: reword slightly, refer to POSIX/C99.
> 
>  lib/vsprintf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 03fa10b4be96..98b0d7be3fb7 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1457,7 +1457,7 @@ static noinline_for_stack
>  char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	      struct printf_spec spec)
>  {
> -	int default_width = 2 * sizeof(void *) + (spec.flags & 
> SPECIAL ? 2 : 0);
> +	const int default_width = 2 * sizeof(void *);
>  
>  	if (!ptr && *fmt != 'K') {
>  		/*

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy