linux-kernel - [PATCH 2/2] vsprintf: Stop using obsolete simple

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20181211152113.8523-3-thomas.preston@codethink.co.uk>
Date:   Tue, 11 Dec 2018 15:21:13 +0000
From:   Thomas Preston <thomas.preston@...ethink.co.uk>
To:     akpm@...ux-foundation.org, pmladek@...e.com,
        andriy.shevchenko@...ux.intel.com, rostedt@...dmis.org,
        geert+renesas@...der.be, corbet@....net, me@...in.cc,
        sergey.senozhatsky@...il.com, linux-kernel@...r.kernel.org
Cc:     Thomas Preston <thomas.preston@...ethink.co.uk>,
        Ben Dooks <ben.dooks@...ethink.co.uk>
Subject: [PATCH 2/2] vsprintf: Stop using obsolete simple_strtoul()

Stop using the obsolete functions simple_strtoul() and
simple_strtoull(). Instead, we should use the improved kstrtol() and
kstrtoll() functions. To do this, we must copy the current field into a
null-terminated tmpstr and advance the variable `next` manually.

The width of tmpstr has been chosen because no integer field can be
larger than 22 characters (ULLONG_MAX in octal), plus sign, radix,
new-line, null-terminator and some extra for alignment.

This patch fixes a bug with vsscanf. If passing sscan a 16-digit
hex-string like so:

	sscanf("fafafafa0b0b0b0b", "%8x%8x", &hi, &lo)

then the result comes out with hi always being 0.

The issue is that the code calls simple_strtoul() which consumes up
to 16-digits but only returns an unsigned long (8 hex digits on ARM).
The vsscanf() code then checks and finds that the field_width of 8 was
greater than the 16 consumed characters and tries to fix this by:

while (next - str > field_width) {
    if (is_sign)
	    val.s = div_s64(val.s, base);
    else
	    val.u = div_u64(val.u, base);
    --next;
}

However val.{s,u} is already trunacted from the simple_strtoul call
and all that happens is the value gets divided down to zero.

Signed-off-by: Thomas Preston <thomas.preston@...ethink.co.uk>
Signed-off-by: Ben Dooks <ben.dooks@...ethink.co.uk>
---
 lib/vsprintf.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bbf2ac734711..ec23e18e8cc6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -47,6 +47,8 @@
 #include <linux/string_helpers.h>
 #include "kstrtox.h"
 
+#define INT_BUF_LEN 28
+
 /**
  * simple_strtoull - convert a string to an unsigned long long
  * @cp: The start of the string
@@ -2914,7 +2916,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 	u8 qualifier;
 	unsigned int base;
 	union {
+		long sl;
 		long long sll;
+		unsigned long ul;
 		unsigned long long ull;
 	} val;
 	s16 field_width;
@@ -3119,14 +3123,30 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		    || (base == 0 && !isdigit(digit)))
 			break;
 
-		if (is_sign)
-			val.sll = qualifier != 'L' ?
-				simple_strtol(str, &next, base) :
-				simple_strtoll(str, &next, base);
-		else
-			val.ull = qualifier != 'L' ?
-				simple_strtoul(str, &next, base) :
-				simple_strtoull(str, &next, base);
+		if (unlikely((field_width+1) > INT_BUF_LEN))
+			return num;
+
+		if (field_width > 0) {
+			char tmpstr[INT_BUF_LEN];
+			int ret;
+
+			strscpy(tmpstr, str, field_width+1);
+
+			if (is_sign)
+				if (qualifier != 'L')
+					ret = kstrtol(tmpstr, base, &val.sl);
+				else
+					ret = kstrtoll(tmpstr, base, &val.sll);
+			else
+				if (qualifier != 'L')
+					ret = kstrtoul(tmpstr, base, &val.ul);
+				else
+					ret = kstrtoull(tmpstr, base, &val.ull);
+			if (ret < 0)
+				return num;
+
+		}
+		next = (char *)str + field_width;
 
 		if (field_width > 0 && next - str > field_width) {
 			if (base == 0)
-- 
2.11.0