[PATCH v1 0/6] Make sscanf safer

[PATCH v1 0/6] Make sscanf safer

[Konstantin Khlebnikov]

Standard sscanf isn't well designed for input validation:

 * no way to detect interger overflow
 * unmached tail text is ignored
 * no mandatory buffer overflow checks

All these problems were found in the wild in cgroup interfaces:
https://patchwork.kernel.org/patch/10831387/
https://lore.kernel.org/patchwork/patch/1046130/

This patchset has patches for first two issues.
Unbounded "%s" is out of scope for now.

Second patch handles integer overlow as parse error:
sscanf exits without touching related argument.

Third patch adds convenient way for checking that
whole text was matched and nothing left.

Fourth adds missing standard features for %[...],
this might be useful for input validation.

Fifth adds __must_check and will generate some new warnings.

The last is a simple test module for chechking some basic and
new corner-cases added by this patchset.


Code haven't chaneged much:

$ ./scripts/bloat-o-meter lib/vsprintf.o.old lib/vsprintf.o.new
add/remove: 0/0 grow/shrink: 1/0 up/down: 60/0 (60)
Function                                     old     new   delta
vsscanf                                     2270    2330     +60
Total: Before=16904, After=16964, chg +0.35%


Unbounded "%s" could be fixed only by making field width mandatory.
There are only few users and in most cases sscanf could be eliminated.


---

Konstantin Khlebnikov (6):
      lib: scanf: document features of scanf format string
      lib: scanf: handle integer overflows in vsscanf
      lib: scanf: add vsscanf feature for matching end of text
      lib: scanf: handle character ranges in %[...]
      lib: scanf: mark sscanf and vsscanf as __must_check
      lib: scanf: add test module


 include/linux/kernel.h |   10 ++
 lib/Kconfig.debug      |    3 +
 lib/Makefile           |    1 
 lib/test_scanf.c       |  252 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c         |  153 +++++++++++++++++++++--------
 5 files changed, 376 insertions(+), 43 deletions(-)
 create mode 100644 lib/test_scanf.c

--
Signature

[PATCH v1 1/6] lib: scanf: document features of scanf format string

[Konstantin Khlebnikov]

Kernel implementation of vsscanf() has some limitations.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 lib/vsprintf.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 791b6fa36905..276a0bc3b019 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3004,6 +3004,28 @@ EXPORT_SYMBOL_GPL(bprintf);
  * @buf:	input buffer
  * @fmt:	format of buffer
  * @args:	arguments
+ *
+ * Returns number of successfully matched arguments.
+ *
+ * This function implements only basic vsscanf features:
+ *
+ * - conversion specifiers: d, i, o, u, x, X, s, c, [, n
+ * - type modifiers: hh, h, l, L, ll, z
+ *
+ * Not implemented features:
+ *
+ * - floating-point numbers
+ * - memory-allocation "%m..."
+ * - pointer "%p", ptrdiff_t "%t...", intmax_t "%j..."
+ * - discaring of matching input "%*[...]"
+ * - ranges or matching ']' in "%[...]"
+ * - positional assignment "%n$"
+ *
+ * Non-standatd features:
+ *
+ * - "%[...]" requires field width
+ * - %s without field width limited with SHRT_MAX
+ * - "%*..." simply skips non white-space characters without conversion
  */
 int vsscanf(const char *buf, const char *fmt, va_list args)
 {
@@ -3291,6 +3313,12 @@ EXPORT_SYMBOL(vsscanf);
  * @buf:	input buffer
  * @fmt:	formatting of buffer
  * @...:	resulting arguments
+ *
+ * Returns number of successfully matched arguments.
+ *
+ * See format description in comment for vsscanf() above.
+ *
+ * For single integer conversion it is better to use kstrto*().
  */
 int sscanf(const char *buf, const char *fmt, ...)
 {

[PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

[Konstantin Khlebnikov]

Traditional scanf implementations ignore integer overflows because
C language standard allows here undefined behavior (§7.21.6.2 #10).

So, sane and safe behavior wouldn't harm anything.

This patch carefully checks integer overflows and stops matching if result
does not fit into appropriate type before assigning it into argument.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 lib/vsprintf.c |   86 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 30 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 276a0bc3b019..ada0501f1525 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3026,12 +3026,13 @@ EXPORT_SYMBOL_GPL(bprintf);
  * - "%[...]" requires field width
  * - %s without field width limited with SHRT_MAX
  * - "%*..." simply skips non white-space characters without conversion
+ * - integer overflows are handled as matching failure
  */
 int vsscanf(const char *buf, const char *fmt, va_list args)
 {
 	const char *str = buf;
-	char *next;
-	char digit;
+	const char *next;
+	unsigned int rv;
 	int num = 0;
 	u8 qualifier;
 	unsigned int base;
@@ -3230,29 +3231,31 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		 */
 		str = skip_spaces(str);
 
-		digit = *str;
-		if (is_sign && digit == '-')
-			digit = *(str + 1);
+		next = str;
 
-		if (!digit
-		    || (base == 16 && !isxdigit(digit))
-		    || (base == 10 && !isdigit(digit))
-		    || (base == 8 && (!isdigit(digit) || digit > '7'))
-		    || (base == 0 && !isdigit(digit)))
-			break;
+		/* skip leading sign */
+		if (is_sign && (*str == '+' || *str == '-'))
+			next++;
 
-		if (is_sign)
-			val.s = qualifier != 'L' ?
-				simple_strtol(str, &next, base) :
-				simple_strtoll(str, &next, base);
-		else
-			val.u = qualifier != 'L' ?
-				simple_strtoul(str, &next, base) :
-				simple_strtoull(str, &next, base);
+		/* 64-bit integer conversion, similar to _kstrtoull() */
+		next = _parse_integer_fixup_radix(next, &base);
+		rv = _parse_integer(next, base, &val.u);
+		if (rv == 0)
+			break;
+		if (rv & KSTRTOX_OVERFLOW)
+			goto overflow;
+		next += rv;
+
+		if (is_sign) {
+			if (*str == '-') {
+				val.s = -val.u;
+				if (val.s > 0)
+					goto overflow;
+			} else if (val.s < 0)
+				goto overflow;
+		}
 
-		if (field_width > 0 && next - str > field_width) {
-			if (base == 0)
-				_parse_integer_fixup_radix(str, &base);
+		if (field_width > 0) {
 			while (next - str > field_width) {
 				if (is_sign)
 					val.s = div_s64(val.s, base);
@@ -3264,22 +3267,37 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 
 		switch (qualifier) {
 		case 'H':	/* that's 'hh' in format */
-			if (is_sign)
+			if (is_sign) {
+				if ((signed char)val.s != val.s)
+					goto overflow;
 				*va_arg(args, signed char *) = val.s;
-			else
+			} else {
+				if ((unsigned char)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned char *) = val.u;
+			}
 			break;
 		case 'h':
-			if (is_sign)
+			if (is_sign) {
+				if ((short)val.s != val.s)
+					goto overflow;
 				*va_arg(args, short *) = val.s;
-			else
+			} else {
+				if ((unsigned short)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned short *) = val.u;
+			}
 			break;
 		case 'l':
-			if (is_sign)
+			if (is_sign) {
+				if ((long)val.s != val.s)
+					goto overflow;
 				*va_arg(args, long *) = val.s;
-			else
+			} else {
+				if ((unsigned long)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned long *) = val.u;
+			}
 			break;
 		case 'L':
 			if (is_sign)
@@ -3288,13 +3306,20 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 				*va_arg(args, unsigned long long *) = val.u;
 			break;
 		case 'z':
+			if ((size_t)val.u != val.u)
+				goto overflow;
 			*va_arg(args, size_t *) = val.u;
 			break;
 		default:
-			if (is_sign)
+			if (is_sign) {
+				if ((int)val.s != val.s)
+					goto overflow;
 				*va_arg(args, int *) = val.s;
-			else
+			} else {
+				if ((unsigned int)val.u != val.u)
+					goto overflow;
 				*va_arg(args, unsigned int *) = val.u;
+			}
 			break;
 		}
 		num++;
@@ -3304,6 +3329,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		str = next;
 	}
 
+overflow:
 	return num;
 }
 EXPORT_SYMBOL(vsscanf);

[PATCH v1 3/6] lib: scanf: add vsscanf feature for matching end of text

[Konstantin Khlebnikov]

Traditional sscanf ignores trailing unmatched characters,
because it derives from design of streaming fscanf().

Without extra care this leads to confusing parsing results:
"%d" successfully parses "0x1" as 0 and "1M" as 1.

Silent ignore of trailing input also could hide bugs and
unexpectedly break backward compatibility at any change.

This patch adds end of text matching into sscanf format by
ASCII End-Of-Transfer character '\004' (EOT aka ^D).

EOT stops matching and if input text haven't ended and adds
flag SCANF_MORE to the result value. Result stays positive.

EOT allows to rephrase common end of text checks like:

int end;
if (sscanf(buf, fmt "%n", ..., &end) == NR && !buf[end])

char dummy;
if (sscanf(buf, fmt "%c", ..., &dummy) == NR - 1)

purely in format string without extra code or arguments:

if (sscanf(buf, fmt KERN_EOT, ...) == NR)

This checks that text ends strictly after previous matched character.
For skipping trailing white spaces simply add space before EOT.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 include/linux/kernel.h |    6 ++++++
 lib/vsprintf.c         |   11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 34a5036debd3..32726b25eaa5 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -476,6 +476,12 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
 extern __printf(2, 0)
 const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
+#define KERN_EOT	"\004"	/* ASCII End Of Transfer */
+#define KERN_EOT_ASCII	'\004'
+
+/* sscanf adds this to the result if EOT does not match end of text */
+#define SCANF_MORE	(1 << 30)
+
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ada0501f1525..66debf42387a 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3005,7 +3005,8 @@ EXPORT_SYMBOL_GPL(bprintf);
  * @fmt:	format of buffer
  * @args:	arguments
  *
- * Returns number of successfully matched arguments.
+ * Returns number of successfully matched arguments,
+ * plus SCANF_MORE bit flag if EOT does not matched end of text.
  *
  * This function implements only basic vsscanf features:
  *
@@ -3027,6 +3028,8 @@ EXPORT_SYMBOL_GPL(bprintf);
  * - %s without field width limited with SHRT_MAX
  * - "%*..." simply skips non white-space characters without conversion
  * - integer overflows are handled as matching failure
+ * - KERN_EOT ("\004") matches end of string otherwise stops parsing and
+ *   returns count matched arguments plus SCANF_MORE bit flag,
  */
 int vsscanf(const char *buf, const char *fmt, va_list args)
 {
@@ -3055,6 +3058,12 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 
 		/* anything that is not a conversion must match exactly */
 		if (*fmt != '%' && *fmt) {
+			/* EOT in format string: text must end here */
+			if (*fmt == KERN_EOT_ASCII) {
+				if (*str)
+					num |= SCANF_MORE;
+				break;
+			}
 			if (*fmt++ != *str++)
 				break;
 			continue;

[PATCH v1 4/6] lib: scanf: handle character ranges in %[...]

[Konstantin Khlebnikov]

Complete implementation isn't much bigger than present one.

This patch adds missing standard features:

  %[a-z] - matches 'a'..'z'
  %[][] - matches '[' and ']'   (']' must be first)
  %[_-] - matches '_' and '-'   ('-' must be last)

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 lib/vsprintf.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 66debf42387a..5c27c5d18d4e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3019,7 +3019,6 @@ EXPORT_SYMBOL_GPL(bprintf);
  * - memory-allocation "%m..."
  * - pointer "%p", ptrdiff_t "%t...", intmax_t "%j..."
  * - discaring of matching input "%*[...]"
- * - ranges or matching ']' in "%[...]"
  * - positional assignment "%n$"
  *
  * Non-standatd features:
@@ -3160,11 +3159,8 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		/*
 		 * Warning: This implementation of the '[' conversion specifier
 		 * deviates from its glibc counterpart in the following ways:
-		 * (1) It does NOT support ranges i.e. '-' is NOT a special
-		 *     character
-		 * (2) It cannot match the closing bracket ']' itself
-		 * (3) A field width is required
-		 * (4) '%*[' (discard matching input) is currently not supported
+		 * (1) A field width is required
+		 * (2) '%*[' (discard matching input) is currently not supported
 		 *
 		 * Example usage:
 		 * ret = sscanf("00:0a:95","%2[^:]:%2[^:]:%2[^:]",
@@ -3176,7 +3172,6 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		{
 			char *s = (char *)va_arg(args, char *);
 			DECLARE_BITMAP(set, 256) = {0};
-			unsigned int len = 0;
 			bool negate = (*fmt == '^');
 
 			/* field width is required */
@@ -3186,12 +3181,25 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			if (negate)
 				++fmt;
 
-			for ( ; *fmt && *fmt != ']'; ++fmt, ++len)
-				set_bit((u8)*fmt, set);
+			do {
+				/* no ']' found */
+				if (!*fmt)
+					return num;
 
-			/* no ']' or no character set found */
-			if (!*fmt || !len)
-				return num;
+				/* '-' at last position is non-special */
+				if (*fmt == '-' && *(fmt + 1) != ']') {
+					u8 a = *(fmt - 1);
+					u8 b = *(fmt + 1);
+
+					if (a <= b)
+						bitmap_set(set, a, b - a + 1);
+				} else
+					set_bit((u8)*fmt, set);
+
+				/* ']' at first position is non-special */
+			} while (*++fmt != ']');
+
+			/* skip ']' */
 			++fmt;
 
 			if (negate) {

[PATCH v1 5/6] lib: scanf: mark sscanf and vsscanf as __must_check

[Konstantin Khlebnikov]

Caller must check returned count of matched arguments.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 include/linux/kernel.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 32726b25eaa5..3142a456f00b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -483,9 +483,9 @@ const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 #define SCANF_MORE	(1 << 30)
 
 extern __scanf(2, 3)
-int sscanf(const char *, const char *, ...);
+int __must_check sscanf(const char *, const char *, ...);
 extern __scanf(2, 0)
-int vsscanf(const char *, const char *, va_list);
+int __must_check vsscanf(const char *, const char *, va_list);
 
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);

[PATCH v1 6/6] lib: scanf: add test module

[Konstantin Khlebnikov]

Module is under config option CONFIG_TEST_SCANF. This is just basic cases,
complete coverage isn't that easy for such complicated function.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 lib/Kconfig.debug |    3 +
 lib/Makefile      |    1 
 lib/test_scanf.c  |  252 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 lib/test_scanf.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 91ed81250fb3..b50ac3c81961 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1806,6 +1806,9 @@ config TEST_STRING_HELPERS
 config TEST_KSTRTOX
 	tristate "Test kstrto*() family of functions at runtime"
 
+config TEST_SCANF
+	tristate "Test scanf() family of functions at runtime"
+
 config TEST_PRINTF
 	tristate "Test printf() family of functions at runtime"
 
diff --git a/lib/Makefile b/lib/Makefile
index 647517940b29..b1d538d7180e 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -68,6 +68,7 @@ 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
+obj-$(CONFIG_TEST_SCANF) += test_scanf.o
 obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
 obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o
 obj-$(CONFIG_TEST_UUID) += test_uuid.o
diff --git a/lib/test_scanf.c b/lib/test_scanf.c
new file mode 100644
index 000000000000..1a4f8f145a46
--- /dev/null
+++ b/lib/test_scanf.c
@@ -0,0 +1,252 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#ifndef KERN_EOT
+#define KERN_EOT ""
+#define SCANF_MORE 0
+#endif
+
+#define TEST_ONE(str, fmt, type, type_fmt, ret, val)	\
+	do {						\
+		type _neg = (type)~(type)(val);		\
+		type _val = _neg;			\
+		int _ret = sscanf(str, fmt, &_val);	\
+		WARN(_ret != (ret), "scanf(\"%s\", \"%s\", ...) returned %d, expected %d", str, fmt, _ret, ret);	\
+		WARN(ret && _ret && _val != (type)(val), "scanf(\"%s\", \"%s\", ...) matched value " type_fmt ", expected " type_fmt, str, fmt, _val, (type)(val));	\
+		WARN(!_ret && _val != _neg, "scanf(\"%s\", \"%s\", ...) unmached value overwritten with " type_fmt, str, fmt, _val);	\
+	} while (0);
+
+#define TEST_STR(str, fmt, len, ret, val)		\
+	do {						\
+		char _val[len + 1] = {0,};		\
+		int _ret = sscanf(str, fmt, _val);	\
+		WARN(_ret != (ret), "scanf(\"%s\", \"%s\", ...) returned %d, expected %d", str, fmt, _ret, ret);	\
+		WARN(ret && _ret && strncmp(_val, val, len), "scanf(\"%s\", \"%s\", ...) matched value \"%.*s\", expected \"%.*s\"", str, fmt, len + 1, _val, len + 1, val);	\
+		WARN(!_ret && _val[0], "scanf(\"%s\", \"%s\", ...) unmached value overwritten with \"%.*s\"", str, fmt, len + 1, _val);	\
+	} while (0);
+
+#define TEST_VA(str, fmt, ret, ...)			\
+	do {						\
+		int _ret = sscanf(str, fmt, ##__VA_ARGS__);	\
+		WARN(_ret != (ret), "scanf(\"%s\", \"%s\", ...) returned %d, expected %d", str, fmt, _ret, ret);	\
+	} while (0);
+
+#define TEST_SC(str, fmt, ret, val)	TEST_ONE(str, fmt, signed char, "%d", ret, val)
+#define TEST_UC(str, fmt, ret, val)	TEST_ONE(str, fmt, unsigned char, "%u", ret, val)
+#define TEST_SS(str, fmt, ret, val)	TEST_ONE(str, fmt, short, "%d", ret, val)
+#define TEST_US(str, fmt, ret, val)	TEST_ONE(str, fmt, unsigned short, "%u", ret, val)
+#define TEST_SI(str, fmt, ret, val)	TEST_ONE(str, fmt, int, "%d", ret, val)
+#define TEST_UI(str, fmt, ret, val)	TEST_ONE(str, fmt, unsigned int, "%u", ret, val)
+#define TEST_SL(str, fmt, ret, val)	TEST_ONE(str, fmt, long, "%ld", ret, val)
+#define TEST_UL(str, fmt, ret, val)	TEST_ONE(str, fmt, unsigned long, "%lu", ret, val)
+#define TEST_SQ(str, fmt, ret, val)	TEST_ONE(str, fmt, long long, "%llu", ret, val)
+#define TEST_UQ(str, fmt, ret, val)	TEST_ONE(str, fmt, unsigned long long, "%llu", ret, val)
+
+static void __init test_base(void)
+{
+	TEST_UI("10", "%u", 1, 10);
+	TEST_SI("10", "%d", 1, 10);
+	TEST_UI("10", "%o", 1, 8);
+	TEST_UI("10", "%x", 1, 16);
+	TEST_SI("10", "%i", 1, 10);
+	TEST_SI("010", "%i", 1, 8);
+	TEST_SI("0x10", "%i", 1, 16);
+
+	TEST_UI("9a", "%u", 1, 9);
+	TEST_SI("9a", "%d", 1, 9);
+	TEST_UI("78", "%o", 1, 7);
+	TEST_UI("fg", "%x", 1, 15);
+	TEST_SI("9a", "%i", 1, 9);
+	TEST_SI("078", "%i", 1, 7);
+	TEST_SI("0xfg", "%i", 1, 15);
+
+	TEST_UI("a", "%u", 0, 0);
+	TEST_SI("a", "%d", 0, 0);
+	TEST_UI("8", "%o", 0, 0);
+	TEST_UI("g", "%x", 0, 0);
+	TEST_SI("a", "%i", 0, 0);
+}
+
+static void __init test_sign(void)
+{
+	TEST_UI("+0", "%u", 0, 0);
+	TEST_UI("-0", "%u", 0, 0);
+	TEST_UI("+1", "%u", 0, 0);
+	TEST_UI("-1", "%u", 0, 0);
+	TEST_UI("++1", "%u", 0, 0);
+	TEST_UI("--1", "%u", 0, 0);
+	TEST_UI("+-1", "%u", 0, 0);
+	TEST_UI("-+1", "%u", 0, 0);
+	TEST_UI("+", "%u", 0, 0);
+	TEST_UI("-", "%u", 0, 0);
+
+	TEST_SI("+0", "%d", 1, 0);
+	TEST_SI("-0", "%d", 1, 0);
+	TEST_SI("+1", "%d", 1, 1);
+	TEST_SI("-1", "%d", 1, -1);
+	TEST_SI("++1", "%d", 0, 0);
+	TEST_SI("--1", "%d", 0, 0);
+	TEST_SI("+-1", "%d", 0, 0);
+	TEST_SI("-+1", "%d", 0, 0);
+	TEST_SI("+", "%d", 0, 0);
+	TEST_SI("-", "%d", 0, 0);
+}
+
+static void __init test_range(void)
+{
+	TEST_UC("255", "%hhu", 1, 255);
+	TEST_US("65535", "%hu", 1, 65535);
+	TEST_UI("4294967295", "%u", 1, 4294967295);
+	TEST_UL("4294967295", "%lu", 1, 4294967295);
+#if BITS_PER_LONG == 32
+	TEST_UL("18446744073709551615", "%lu", 0, 0);
+#else
+	TEST_UL("18446744073709551615", "%lu", 1, 18446744073709551615ull);
+#endif
+	TEST_UQ("18446744073709551615", "%llu", 1, 18446744073709551615ull);
+
+
+	TEST_SC("-128", "%hhd", 1, -128);
+	TEST_SC("127", "%hhd", 1, 127);
+	TEST_SS("-32768", "%hd", 1, -32768);
+	TEST_SS("32767", "%hd", 1, 32767);
+	TEST_SI("-2147483648", "%d", 1, -2147483648);
+	TEST_SI("2147483647", "%d", 1, 2147483647);
+	TEST_SL("-2147483648", "%ld", 1, -2147483648);
+	TEST_SL("2147483647", "%ld", 1, 2147483647);
+#if BITS_PER_LONG == 32
+	TEST_SL("-9223372036854775808", "%ld", 0, 0);
+	TEST_SL("9223372036854775807", "%ld", 0, 0);
+#else
+	TEST_SL("-9223372036854775808", "%ld", 1, -9223372036854775808ull);
+	TEST_SL("9223372036854775807", "%ld", 1, 9223372036854775807ll);
+#endif
+	TEST_SQ("-9223372036854775808", "%lld", 1, -9223372036854775808ull);
+	TEST_SQ("9223372036854775807", "%lld", 1, 9223372036854775807ll);
+
+
+	TEST_UC("256", "%hhu", 0, 0);
+	TEST_US("65536", "%hu", 0, 0);
+	TEST_UI("4294967296", "%u", 0, 0);
+#if BITS_PER_LONG == 32
+	TEST_UL("4294967296", "%lu", 0, 0);
+#else
+	TEST_UL("4294967296", "%lu", 1, 4294967296);
+#endif
+	TEST_UL("18446744073709551616", "%lu", 0, 0);
+	TEST_UQ("18446744073709551616", "%llu", 0, 0);
+
+	TEST_SC("-129", "%hhd", 0, 0);
+	TEST_SC("128", "%hhd", 0, 0);
+	TEST_SS("-32769", "%hd", 0, 0);
+	TEST_SS("32768", "%hd", 0, 0);
+	TEST_SI("-2147483649", "%d", 0, 0);
+	TEST_SI("2147483648", "%d", 0, 0);
+#if BITS_PER_LONG == 32
+	TEST_SL("-2147483649", "%ld", 0, 0);
+	TEST_SL("2147483648", "%ld", 0, 0);
+#else
+	TEST_SL("-2147483649", "%ld", 1, -2147483649);
+	TEST_SL("2147483648", "%ld", 1, 2147483648);
+#endif
+	TEST_SL("-9223372036854775809", "%ld", 0, 0);
+	TEST_SL("9223372036854775808", "%ld", 0, 0);
+	TEST_SQ("-9223372036854775809", "%lld", 0, 0);
+	TEST_SQ("9223372036854775808", "%lld", 0, 0);
+}
+
+static void __init test_eot(void)
+{
+	TEST_UI("123", "%u" KERN_EOT, 1, 123);
+	TEST_UI("123 ", "%u" KERN_EOT, 1 | SCANF_MORE, 123);
+	TEST_UI("123 ", "%u " KERN_EOT, 1, 123);
+	TEST_UI("123 \t\n", "%u " KERN_EOT, 1, 123);
+	TEST_UI("123z ", "%u " KERN_EOT, 1 | SCANF_MORE, 123);
+
+	TEST_SI("1M", "%d", 1, 1);
+	TEST_SI("1M", "%d" KERN_EOT, 1 | SCANF_MORE, 1);
+
+	TEST_SI("1\n", "%d" KERN_EOT, 1 | SCANF_MORE, 1);
+	TEST_SI("1\n", "%d " KERN_EOT, 1, 1);
+}
+
+static void __init test_set(void)
+{
+	// TODO it would be nice to make field width mandatory here
+	TEST_STR("abc", "%s", 3, 1, "abc");
+
+	TEST_STR("abc", "%3s", 3, 1, "abc");
+	TEST_STR("abc", "%2s", 3, 1, "ab");
+	TEST_STR("abc", "%1s", 3, 1, "a");
+
+	TEST_STR(" a\n", "%3s", 3, 1, "a");
+
+	TEST_STR("abc", "%3c", 3, 1, "abc");
+	TEST_STR(" a\n", "%3c", 3, 1, " a\n");
+
+	TEST_STR("abc", "%[a-z]", 3, 0, "");	// no width
+
+	TEST_STR("abc", "%3[abc]", 3, 1, "abc");
+	TEST_STR("abc", "%3[^abc]", 3, 0, "");
+
+	TEST_STR("abc", "%3[xyz]", 3, 0, "");
+	TEST_STR("abc", "%3[^xyz]", 3, 1, "abc");
+
+	TEST_STR(" a", "%3[a]", 3, 0, "");
+	TEST_STR(" a", "%3[ a]", 3, 1, " a");
+
+	TEST_STR("abcd", "%5[a-c]", 5, 1, "abc");
+	TEST_STR("abc", "%3[^a-b]", 3, 0, "");
+
+	TEST_STR("bd", "%3[a-c]", 3, 1, "b");
+	TEST_STR("db", "%3[^a-c]", 3, 1, "d");
+
+	TEST_STR("q1w2e3", "%8[a-z0-9]", 8, 1, "q1w2e3");
+	TEST_STR("q1w2e3", "%8[^a-z0-9]", 8, 0, "");
+
+	TEST_STR("a-", "%3[a-b]", 3, 1, "a");
+	TEST_STR("a-", "%3[a-]", 3, 1, "a-");
+	TEST_STR("a-", "%3[^a-]", 3, 0, "");
+
+	TEST_STR("-", "%3[-]", 3, 1, "-");
+	TEST_STR("-", "%3[^-]", 3, 0, "");
+
+	TEST_STR("]ab", "%3[]]", 3, 1, "]");
+	TEST_STR("a]b", "%3[^]]", 3, 1, "a");
+
+	TEST_STR("]-abc", "%3[]-]", 3, 1, "]-");
+	TEST_STR("abc]-", "%3[^]-]", 3, 1, "abc");
+}
+
+static void __init test_va(void)
+{
+	int x, y, z;
+	char a, b, c;
+
+	TEST_VA(" 1 ", "%d%d", 1, &x, &y);
+	TEST_VA(" 1 2 ", "%d%d", 2, &x, &y);
+	TEST_VA(" 1 2 3 ", "%d%d%d", 3, &x, &y, &z);
+	TEST_VA(" 1 2 3 ", "%d%d", 2, &x, &y);
+
+	TEST_VA(" 1x 2 ", "%d%d", 1, &x, &y);
+
+	TEST_VA("1", "%d%c", 1, &x, &a);
+	TEST_VA("1 ", "%d%c", 2, &x, &a);
+	TEST_VA("1 ", "%d %c", 1, &x, &a);
+
+	TEST_VA(" \t\n", "%c%c%c", 3, &a, &b, &c);
+}
+
+static int __init test_scanf_init(void)
+{
+	test_base();
+	test_sign();
+	test_range();
+	test_eot();
+	test_set();
+	test_va();
+	return -EINVAL;
+}
+module_init(test_scanf_init);
+MODULE_LICENSE("GPL");

Re: [PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

[Rasmus Villemoes]

On 10/03/2019 17.56, Konstantin Khlebnikov wrote:
> Traditional scanf implementations ignore integer overflows because
> C language standard allows here undefined behavior (§7.21.6.2 #10).
> 
> So, sane and safe behavior wouldn't harm anything.
> 
> This patch carefully checks integer overflows and stops matching if result
> does not fit into appropriate type before assigning it into argument.

IIRC, this has been attempted before, causing a userspace regression
because some sysfs/procfs file matched with %u or %x, and somebody wrote
-1 to get 0xffffffff .

I can't remember or find a reference right now, making the above a
rather weak argument. However, please start the series with your test
cases, before any refactoring. That makes it easier to see what
behaviour you're changing (i.e., what used to be allowed is now treated
as non-match, etc.).

Rasmus

Re: [PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

[Linus Torvalds]

On Sun, Mar 10, 2019 at 2:06 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> IIRC, this has been attempted before, causing a userspace regression
> because some sysfs/procfs file matched with %u or %x, and somebody wrote
> -1 to get 0xffffffff .

.. which is correct anyway. That's how scanf is supposed to work.

If somebody needs overflow checking, they shouldn't be using scanf.

                  Linus

Re: [PATCH v1 2/6] lib: scanf: handle integer overflows in vsscanf

[Konstantin Khlebnikov]

On 11.03.2019 0:52, Linus Torvalds wrote:
> On Sun, Mar 10, 2019 at 2:06 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> IIRC, this has been attempted before, causing a userspace regression
>> because some sysfs/procfs file matched with %u or %x, and somebody wrote
>> -1 to get 0xffffffff .
> 
> .. which is correct anyway. That's how scanf is supposed to work.

Well. In this case '%u' sscanf in kernel is already broken.
%u does not accept minus sign and looks like never does.

'is_sign' is set only for %d and %i,
without it string couldn't pass initial check of first "digit".

More over simple_strtoul never accepted '-'.

Glibc version indeed accepts '-1' as %u and returns 0xFFFFFFFF
which isn't in manpage:' optionally signed' mentioned only for %d and %i.
But this explicitly mentioned in manpage for 'stroul'.

> 
> If somebody needs overflow checking, they shouldn't be using scanf.
> 
>                    Linus
>