Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1275878 > unrolled thread

[PATCH 00/14] printf stuff for 4.5

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/14] printf stuff for 4.5 Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
    [PATCH 14/14] lib/test_printf.c: test dentry printing Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
    [PATCH 02/14] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
    [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
      Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-24 00:00 +0100
        Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-26 22:40 +0100
    [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
    [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
      Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and  field widths Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-23 23:40 +0100
        Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-26 22:20 +0100
    [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
      Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-23 23:40 +0100
        Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-26 22:30 +0100
    [PATCH 11/14] lib/test_printf.c: test precision quirks Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100
    [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-11-23 22:40 +0100

#1275878 — [PATCH 00/14] printf stuff for 4.5

I was too late for 4.4, so here's hoping to get this into 4.5.

1-3 fix a theoretical race in printing strings via %s - since we're
reusing existing code from the dentry printer, we actually also win on
code size. Ingo, can I perhaps get you to turn your "looks good to"
into an ack?

4 fixes a problem introduced by converting all bitmap formatting to go
via %pb[l] - it turns out that bitmaps with >= 1<<15 bits are actually
printed sometimes. This isn't necessarily the best fix, but the
discussion died out, so I'm proposing this for now.

5 is just a minor optimization (maybe not so much on
register-challenged arches).

6 I'm not too sure about, but maybe the bitmap problem had been
discovered sooner (it took a little over half a year to be reported)
if these had been in place.

7 is another microoptimization.

Paranoid-me wanted me to include patch 8, but I don't have strong
feelings for it.

9-14 are minor additions to the test module (some with acks from
Kees).

Rasmus Villemoes (14):
  lib/vsprintf.c: pull out padding code from dentry_name()
  lib/vsprintf.c: move string() below widen_string()
  lib/vsprintf.c: eliminate potential race in string()
  lib/vsprintf.c: expand field_width to 24 bits
  lib/vsprintf.c: help gcc make number() smaller
  lib/vsprintf.c: warn about too large precisions and field widths
  lib/vsprintf.c: slightly refactor vscnprintf()
  lib/kasprintf.c: add sanity check to kvasprintf
  lib/test_printf.c: don't BUG
  lib/test_printf.c: check for out-of-bound writes
  lib/test_printf.c: test precision quirks
  lib/test_printf.c: account for kvasprintf tests
  lib/test_printf.c: add test for large bitmaps
  lib/test_printf.c: test dentry printing

 lib/kasprintf.c   |  10 +--
 lib/test_printf.c |  96 ++++++++++++++++++++++----
 lib/vsprintf.c    | 196 +++++++++++++++++++++++++++++++-----------------------
 3 files changed, 203 insertions(+), 99 deletions(-)

-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1275879 — [PATCH 14/14] lib/test_printf.c: test dentry printing

Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 705907aec901..6b7c4a1ec141 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -13,6 +13,7 @@
 #include <linux/string.h>
 
 #include <linux/bitmap.h>
+#include <linux/dcache.h>
 #include <linux/socket.h>
 #include <linux/in.h>
 
@@ -301,9 +302,35 @@ uuid(void)
 	test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid);
 }
 
+static struct dentry test_dentry[4] __initdata = {
+	{ .d_parent = &test_dentry[0],
+	  .d_name = { .len = 3, .name = test_dentry[0].d_iname },
+	  .d_iname = "foo" },
+	{ .d_parent = &test_dentry[0],
+	  .d_name = { .len = 5, .name = test_dentry[1].d_iname },
+	  .d_iname = "bravo" },
+	{ .d_parent = &test_dentry[1],
+	  .d_name = { .len = 4, .name = test_dentry[2].d_iname },
+	  .d_iname = "alfa" },
+	{ .d_parent = &test_dentry[2],
+	  .d_name = { .len = 5, .name = test_dentry[3].d_iname },
+	  .d_iname = "romeo" },
+};
+
 static void __init
 dentry(void)
 {
+	test("foo", "%pd", &test_dentry[0]);
+	test("foo", "%pd2", &test_dentry[0]);
+
+	test("romeo", "%pd", &test_dentry[3]);
+	test("alfa/romeo", "%pd2", &test_dentry[3]);
+	test("bravo/alfa/romeo", "%pd3", &test_dentry[3]);
+	test("/bravo/alfa/romeo", "%pd4", &test_dentry[3]);
+	test("/bravo/alfa", "%pd4", &test_dentry[2]);
+
+	test("bravo/alfa  |bravo/alfa  ", "%-12pd2|%*pd2", &test_dentry[2], -12, &test_dentry[2]);
+	test("  bravo/alfa|  bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]);
 }
 
 static void __init
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275881 — [PATCH 02/14] lib/vsprintf.c: move string() below widen_string()

This is pure code movement, making sure the widen_string() helper is
defined before the string() function.

Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 62 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7452563a6a6..a021e6380404 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -507,37 +507,6 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
-static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
-{
-	int len, i;
-
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
-
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
-
-	return buf;
-}
-
 static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
@@ -586,6 +555,37 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 }
 
 static noinline_for_stack
+char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "(null)";
+
+	len = strnlen(s, spec.precision);
+
+	if (!(spec.flags & LEFT)) {
+		while (len < spec.field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < spec.field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
 {
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275882 — [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

If the string corresponding to a %s specifier can change under us, we
might end up copying a \0 byte to the output buffer. There might be
callers who expect the output buffer to contain a genuine C string
whose length is exactly the snprintf return value (assuming truncation
hasn't happened or has been checked for).

We can avoid this by only passing over the source string once,
stopping the first time we meet a nul byte (or when we reach the given
precision), and then letting widen_string() handle left/right space
padding. As a small bonus, this code reuse also makes the generated
code slightly smaller.

Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a021e6380404..63ca52366049 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 static noinline_for_stack
 char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
-	int len, i;
+	int len = 0;
+	size_t lim = spec.precision;
 
 	if ((unsigned long)s < PAGE_SIZE)
 		s = "(null)";
 
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
+	while (lim--) {
+		char c = *s++;
+		if (!c)
+			break;
 		if (buf < end)
-			*buf = ' ';
+			*buf = c;
 		++buf;
+		++len;
 	}
-
-	return buf;
+	return widen_string(buf, len, end, spec);
 }
 
 static noinline_for_stack
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275941 — Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> If the string corresponding to a %s specifier can change under us, we
> might end up copying a \0 byte to the output buffer. There might be
> callers who expect the output buffer to contain a genuine C string
> whose length is exactly the snprintf return value (assuming truncation
> hasn't happened or has been checked for).
>
> We can avoid this by only passing over the source string once,
> stopping the first time we meet a nul byte (or when we reach the given
> precision), and then letting widen_string() handle left/right space
> padding. As a small bonus, this code reuse also makes the generated
> code slightly smaller.
>

Could it be pair of patches: a) re-use, b) optimize for fuzzy strings?

> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a021e6380404..63ca52366049 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>  static noinline_for_stack
>  char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>  {
> -       int len, i;
> +       int len = 0;
> +       size_t lim = spec.precision;

Just a nitpick: maybe longer first?

>
>         if ((unsigned long)s < PAGE_SIZE)
>                 s = "(null)";
>
> -       len = strnlen(s, spec.precision);
> -
> -       if (!(spec.flags & LEFT)) {
> -               while (len < spec.field_width--) {
> -                       if (buf < end)
> -                               *buf = ' ';
> -                       ++buf;
> -               }
> -       }
> -       for (i = 0; i < len; ++i) {
> -               if (buf < end)
> -                       *buf = *s;
> -               ++buf; ++s;
> -       }
> -       while (len < spec.field_width--) {
> +       while (lim--) {
> +               char c = *s++;
> +               if (!c)
> +                       break;
>                 if (buf < end)
> -                       *buf = ' ';
> +                       *buf = c;
>                 ++buf;
> +               ++len;
>         }
> -
> -       return buf;
> +       return widen_string(buf, len, end, spec);
>  }
>
>  static noinline_for_stack
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278459 — Re: [PATCH 03/14] lib/vsprintf.c: eliminate potential race in string()

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> If the string corresponding to a %s specifier can change under us, we
>> might end up copying a \0 byte to the output buffer. There might be
>> callers who expect the output buffer to contain a genuine C string
>> whose length is exactly the snprintf return value (assuming truncation
>> hasn't happened or has been checked for).
>>
>> We can avoid this by only passing over the source string once,
>> stopping the first time we meet a nul byte (or when we reach the given
>> precision), and then letting widen_string() handle left/right space
>> padding. As a small bonus, this code reuse also makes the generated
>> code slightly smaller.
>>
>
> Could it be pair of patches: a) re-use, b) optimize for fuzzy strings?

I'm afraid I have no idea what you mean. The patch is already broken
into three (pull out from dentry(), move helper, do the actual thing to
string()). What's a 'fuzzy string', and what optimization do you think of?

This patch already gives us the bonus of only passing over the source
once instead of twice. (Well, at the expense of a little complicated
logic in case we have a larger field width and not the LEFT flag set,
but these are so extremely rare compared to plain %s that it's not worth
caring about. And in any case, the logic already existed.)

>> Cc: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  lib/vsprintf.c | 28 +++++++++-------------------
>>  1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index a021e6380404..63ca52366049 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
>>  static noinline_for_stack
>>  char *string(char *buf, char *end, const char *s, struct printf_spec spec)
>>  {
>> -       int len, i;
>> +       int len = 0;
>> +       size_t lim = spec.precision;
>
> Just a nitpick: maybe longer first?

Why?

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275883 — [PATCH 12/14] lib/test_printf.c: account for kvasprintf tests

These should also count as performed tests.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5f742b99cfdc..fc9169d360ba 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -127,6 +127,7 @@ __test(const char *expect, int elen, const char *fmt, ...)
 
 	p = kvasprintf(GFP_KERNEL, fmt, ap);
 	if (p) {
+		total_tests++;
 		if (memcmp(p, expect, elen+1)) {
 			pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
 				fmt, p, expect);
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275884 — [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

The field width is overloaded to pass some extra information for
some %p extensions (e.g. #bits for %pb). But we might silently
truncate the passed value when we stash it in struct printf_spec (see
e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
value bits should now be enough for everybody, but if not, let's make
some noise.

Do the same for the precision. In both cases, clamping seems more
sensible than truncating. While, according to POSIX, "A negative
precision is taken as if the precision were omitted.", the kernel's
printf has always treated that case as if the precision was 0, so we
use that as lower bound. For the field width, the smallest
representable value is actually -(1<<23), but a negative field width
means 'set the LEFT flag and use the absolute value', so we want the
absolute value to fit.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d7e27c54fa00..8af5535fd738 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -386,6 +386,8 @@ struct printf_spec {
 	unsigned int	base:8;		/* number base, 8, 10 or 16 only */
 	signed int	precision:16;	/* # of digits/chars */
 } __packed;
+#define FIELD_WIDTH_MAX ((1 << 23) - 1)
+#define PRECISION_MAX ((1 << 15) - 1)
 extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
 
 static noinline_for_stack
@@ -1815,6 +1817,24 @@ qualifier:
 	return ++fmt - start;
 }
 
+static inline void
+set_field_width(struct printf_spec *spec, int width)
+{
+	spec->field_width = width;
+	if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) {
+		spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
+	}
+}
+
+static inline void
+set_precision(struct printf_spec *spec, int prec)
+{
+	spec->precision = prec;
+	if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) {
+		spec->precision = clamp(prec, 0, PRECISION_MAX);
+	}
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -1882,11 +1902,11 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		}
 
 		case FORMAT_TYPE_WIDTH:
-			spec.field_width = va_arg(args, int);
+			set_field_width(&spec, va_arg(args, int));
 			break;
 
 		case FORMAT_TYPE_PRECISION:
-			spec.precision = va_arg(args, int);
+			set_precision(&spec, va_arg(args, int));
 			break;
 
 		case FORMAT_TYPE_CHAR: {
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275937 — Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> The field width is overloaded to pass some extra information for
> some %p extensions (e.g. #bits for %pb). But we might silently
> truncate the passed value when we stash it in struct printf_spec (see
> e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
> value bits should now be enough for everybody, but if not, let's make
> some noise.
>
> Do the same for the precision. In both cases, clamping seems more
> sensible than truncating. While, according to POSIX, "A negative
> precision is taken as if the precision were omitted.", the kernel's
> printf has always treated that case as if the precision was 0, so we
> use that as lower bound. For the field width, the smallest
> representable value is actually -(1<<23), but a negative field width
> means 'set the LEFT flag and use the absolute value', so we want the
> absolute value to fit.
>

Do we need to do the same for bstr_printf() ?

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index d7e27c54fa00..8af5535fd738 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -386,6 +386,8 @@ struct printf_spec {
>         unsigned int    base:8;         /* number base, 8, 10 or 16 only */
>         signed int      precision:16;   /* # of digits/chars */
>  } __packed;
> +#define FIELD_WIDTH_MAX ((1 << 23) - 1)
> +#define PRECISION_MAX ((1 << 15) - 1)
>  extern char __check_printf_spec[1-2*(sizeof(struct printf_spec) != 8)];
>
>  static noinline_for_stack
> @@ -1815,6 +1817,24 @@ qualifier:
>         return ++fmt - start;
>  }
>
> +static inline void
> +set_field_width(struct printf_spec *spec, int width)
> +{
> +       spec->field_width = width;
> +       if (WARN_ONCE(spec->field_width != width, "field width %d too large", width)) {
> +               spec->field_width = clamp(width, -FIELD_WIDTH_MAX, FIELD_WIDTH_MAX);
> +       }
> +}
> +
> +static inline void
> +set_precision(struct printf_spec *spec, int prec)
> +{
> +       spec->precision = prec;
> +       if (WARN_ONCE(spec->precision != prec, "precision %d too large", prec)) {
> +               spec->precision = clamp(prec, 0, PRECISION_MAX);
> +       }
> +}
> +
>  /**
>   * vsnprintf - Format a string and place it in a buffer
>   * @buf: The buffer to place the result into
> @@ -1882,11 +1902,11 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>                 }
>
>                 case FORMAT_TYPE_WIDTH:
> -                       spec.field_width = va_arg(args, int);
> +                       set_field_width(&spec, va_arg(args, int));
>                         break;
>
>                 case FORMAT_TYPE_PRECISION:
> -                       spec.precision = va_arg(args, int);
> +                       set_precision(&spec, va_arg(args, int));
>                         break;
>
>                 case FORMAT_TYPE_CHAR: {
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278453 — Re: [PATCH 06/14] lib/vsprintf.c: warn about too large precisions and field widths

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> The field width is overloaded to pass some extra information for
>> some %p extensions (e.g. #bits for %pb). But we might silently
>> truncate the passed value when we stash it in struct printf_spec (see
>> e.g. "lib/vsprintf.c: expand field_width to 24 bits").  Hopefully 23
>> value bits should now be enough for everybody, but if not, let's make
>> some noise.
>>
>> Do the same for the precision. In both cases, clamping seems more
>> sensible than truncating. While, according to POSIX, "A negative
>> precision is taken as if the precision were omitted.", the kernel's
>> printf has always treated that case as if the precision was 0, so we
>> use that as lower bound. For the field width, the smallest
>> representable value is actually -(1<<23), but a negative field width
>> means 'set the LEFT flag and use the absolute value', so we want the
>> absolute value to fit.
>>
>
> Do we need to do the same for bstr_printf() ?
>

Heh, apparently I didn't learn anything from 762abb51. Thanks, will fix
in next spin.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275885 — [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

If we're given a size of 0, the vsnprintf() won't have any side
effects, and neither "i < size" or "size != 0" will trigger. So we
might as well return 0 immediately.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8af5535fd738..e22a6189548f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
 	int i;
 
+	if (unlikely(!size))
+		return 0;
+
 	i = vsnprintf(buf, size, fmt, args);
 
 	if (likely(i < size))
 		return i;
-	if (size != 0)
-		return size - 1;
-	return 0;
+	return size - 1;
 }
 EXPORT_SYMBOL(vscnprintf);
 
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275936 — Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> If we're given a size of 0, the vsnprintf() won't have any side
> effects, and neither "i < size" or "size != 0" will trigger. So we
> might as well return 0 immediately.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  lib/vsprintf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8af5535fd738..e22a6189548f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  {
>         int i;
>
> +       if (unlikely(!size))
> +               return 0;
> +

Might it potentially shadow any issue when run vsnprintf(buf, 0, fmt,
args); with certain arguments?
I can imagine something like %pV with unstable pointer.

>         i = vsnprintf(buf, size, fmt, args);
>
>         if (likely(i < size))
>                 return i;
> -       if (size != 0)
> -               return size - 1;
> -       return 0;
> +       return size - 1;
>  }
>  EXPORT_SYMBOL(vscnprintf);
>
> --
> 2.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278456 — Re: [PATCH 07/14] lib/vsprintf.c: slightly refactor vscnprintf()

On Mon, Nov 23 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Nov 23, 2015 at 11:29 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> If we're given a size of 0, the vsnprintf() won't have any side
>> effects, and neither "i < size" or "size != 0" will trigger. So we
>> might as well return 0 immediately.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  lib/vsprintf.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 8af5535fd738..e22a6189548f 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -2036,13 +2036,14 @@ int vscnprintf(char *buf, size_t size, const char *fmt, va_list args)
>>  {
>>         int i;
>>
>> +       if (unlikely(!size))
>> +               return 0;
>> +
>
> Might it potentially shadow any issue when run vsnprintf(buf, 0, fmt,
> args); with certain arguments?

Only if we ever come up with a %p extension with side effects, but then
people couldn't rely on them happening exactly once anyway (kasprintf
would make them happen twice). printf-like calls are also often compiled
out or disabled (dyndebug, ratelimit, ...) without it being obvious at
the call site whether they'll run or not, so I think such a hypothetical
%p extension would meet some resistance.

> I can imagine something like %pV with unstable pointer.

I don't see how %pV is different than any other current %p
extensions. 

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275887 — [PATCH 11/14] lib/test_printf.c: test precision quirks

The kernel's printf doesn't follow the standards in a few corner cases
(which are probably mostly irrelevant). Add tests that document the
current behaviour.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 1ce1a1dd8faf..5f742b99cfdc 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -166,14 +166,22 @@ 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");
+	test("1234      ", "%-10.4s", "123456");
+	test("      1234", "%10.4s", "123456");
 	/*
-	 * 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.
+	 * POSIX and C99 say that a negative precision (which is only
+	 * possible to pass via a * argument) should be treated as if
+	 * the precision wasn't present, and that if the precision is
+	 * omitted (as in %.s), the precision should be taken to be
+	 * 0. However, the kernel's printf behave exactly opposite,
+	 * treating a negative precision as 0 and treating an omitted
+	 * precision specifier as if no precision was given.
+	 *
+	 * These test cases document the current behaviour; should
+	 * anyone ever feel the need to follow the standards more
+	 * closely, this can be revisited.
 	 */
+	test("    ", "%4.*s", -5, "123456");
 	test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c");
 	test("a  |   |   ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c");
 }
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275889 — [PATCH 10/14] lib/test_printf.c: check for out-of-bound writes

Add a few padding bytes on either side of the test buffer, and check
that these (and the part of the buffer not used) are untouched by
vsnprintf.

Acked-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/test_printf.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 9232a2add28c..1ce1a1dd8faf 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -16,6 +16,7 @@
 #include <linux/in.h>
 
 #define BUF_SIZE 256
+#define PAD_SIZE 16
 #define FILL_CHAR '$'
 
 #define PTR1 ((void*)0x01234567)
@@ -39,6 +40,7 @@
 static unsigned total_tests __initdata;
 static unsigned failed_tests __initdata;
 static char *test_buffer __initdata;
+static char *alloced_buffer __initdata;
 
 static int __printf(4, 0) __init
 do_test(int bufsize, const char *expect, int elen,
@@ -49,7 +51,7 @@ do_test(int bufsize, const char *expect, int elen,
 
 	total_tests++;
 
-	memset(test_buffer, FILL_CHAR, BUF_SIZE);
+	memset(alloced_buffer, FILL_CHAR, BUF_SIZE + 2*PAD_SIZE);
 	va_copy(aq, ap);
 	ret = vsnprintf(test_buffer, bufsize, fmt, aq);
 	va_end(aq);
@@ -60,8 +62,13 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
+	if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
+		return 1;
+	}
+
 	if (!bufsize) {
-		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE)) {
+		if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) {
 			pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
 				fmt);
 			return 1;
@@ -76,6 +83,12 @@ do_test(int bufsize, const char *expect, int elen,
 		return 1;
 	}
 
+	if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
+		pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\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);
@@ -342,16 +355,17 @@ test_pointer(void)
 static int __init
 test_printf_init(void)
 {
-	test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL);
-	if (!test_buffer)
+	alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
+	if (!alloced_buffer)
 		return -ENOMEM;
+	test_buffer = alloced_buffer + PAD_SIZE;
 
 	test_basic();
 	test_number();
 	test_string();
 	test_pointer();
 
-	kfree(test_buffer);
+	kfree(alloced_buffer);
 
 	if (failed_tests == 0)
 		pr_info("all %u tests passed\n", total_tests);
-- 
2.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web