[PATCH 1/4] Rip out simple_strtoll()

[PATCH 1/4] Rip out simple_strtoll()

[Demi Marie Obenour]

It is not used anywhere but its own unit tests.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 Documentation/dev-tools/checkpatch.rst             |  9 ++++-----
 Documentation/process/deprecated.rst               |  5 ++---
 .../translations/it_IT/process/deprecated.rst      |  9 ++++-----
 .../translations/sp_SP/process/deprecated.rst      | 14 +++++++-------
 include/linux/kstrtox.h                            |  1 -
 lib/kstrtox.c                                      |  2 +-
 lib/test_scanf.c                                   | 10 ----------
 lib/vsprintf.c                                     | 14 --------------
 8 files changed, 18 insertions(+), 46 deletions(-)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index c3389c6f38381f038ed5d9a884f2d333a749f8a2..0ae0ca80beb0c0171e8c04306cb5b9ccbc9fa713 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -290,11 +290,10 @@ API usage
     See: https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
 
   **CONSIDER_KSTRTO**
-    The simple_strtol(), simple_strtoll(), simple_strtoul(), and
-    simple_strtoull() functions explicitly ignore overflows, which
-    may lead to unexpected results in callers.  The respective kstrtol(),
-    kstrtoll(), kstrtoul(), and kstrtoull() functions tend to be the
-    correct replacements.
+    The simple_strtol(), simple_strtoul(), and simple_strtoull() functions
+    explicitly ignore overflows, which may lead to unexpected results in
+    callers.  The respective kstrtol(), kstrtoll(), kstrtoul(), and kstrtoull()
+    functions tend to be the correct replacements.
 
     See: https://www.kernel.org/doc/html/latest/process/deprecated.html#simple-strtol-simple-strtoll-simple-strtoul-simple-strtoull
 
diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index f91b8441f2ef70576c5bad079e631e4077eabed6..b2f677eb6d6b12df63003b00960ef27b1656e4c3 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -109,10 +109,9 @@ For more details, also see array3_size() and flex_array_size(),
 as well as the related check_mul_overflow(), check_add_overflow(),
 check_sub_overflow(), and check_shl_overflow() family of functions.
 
-simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull()
+simple_strtol(), simple_strtoul(), simple_strtoull()
 ----------------------------------------------------------------------
-The simple_strtol(), simple_strtoll(),
-simple_strtoul(), and simple_strtoull() functions
+The simple_strtol(), simple_strtoul(), and simple_strtoull() functions
 explicitly ignore overflows, which may lead to unexpected results
 in callers. The respective kstrtol(), kstrtoll(),
 kstrtoul(), and kstrtoull() functions tend to be the
diff --git a/Documentation/translations/it_IT/process/deprecated.rst b/Documentation/translations/it_IT/process/deprecated.rst
index ba0ed7dc154c95e0e973cb51cc36b95e786079a8..cdc0aa107bce7f4018ad241a02b4f701974e52b4 100644
--- a/Documentation/translations/it_IT/process/deprecated.rst
+++ b/Documentation/translations/it_IT/process/deprecated.rst
@@ -118,12 +118,11 @@ Per maggiori dettagli fate riferimento a array3_size() e flex_array_size(), ma
 anche le funzioni della famiglia check_mul_overflow(), check_add_overflow(),
 check_sub_overflow(), e check_shl_overflow().
 
-simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull()
+simple_strtol(), simple_strtoul(), simple_strtoull()
 ----------------------------------------------------------------------
-Le funzioni simple_strtol(), simple_strtoll(),
-simple_strtoul(), e simple_strtoull() ignorano volutamente
-i possibili overflow, e questo può portare il chiamante a generare risultati
-inaspettati. Le rispettive funzioni kstrtol(), kstrtoll(),
+Le funzioni simple_strtol(), simple_strtoul(), e simple_strtoull() ignorano
+volutamente i possibili overflow, e questo può portare il chiamante a generare
+risultati inaspettati. Le rispettive funzioni kstrtol(), kstrtoll(),
 kstrtoul(), e kstrtoull() sono da considerarsi le corrette
 sostitute; tuttavia va notato che queste richiedono che la stringa sia
 terminata con il carattere NUL o quello di nuova riga.
diff --git a/Documentation/translations/sp_SP/process/deprecated.rst b/Documentation/translations/sp_SP/process/deprecated.rst
index d52120e0d75354d0d32c33d631f9f364eba32f82..f0ba93e7188f02ae98a8533e2dfee4c82cf78c5c 100644
--- a/Documentation/translations/sp_SP/process/deprecated.rst
+++ b/Documentation/translations/sp_SP/process/deprecated.rst
@@ -117,14 +117,14 @@ como también la familia de funciones relacionadas check_mul_overflow(),
 check_add_overflow(), check_sub_overflow(), y check_shl_overflow().
 
 
-simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull()
+simple_strtol(), simple_strtoul(), simple_strtoull()
 ----------------------------------------------------------------------
-Las funciones: simple_strtol(), simple_strtoll(), simple_strtoul(), y
-simple_strtoull() explícitamente ignoran los desbordamientos, lo que puede
-llevar a resultados inesperados por las funciones que las llaman. Las
-funciones respectivas kstrtol(), kstrtoll(), kstrtoul(), y kstrtoull()
-tienden a ser reemplazos correctos, aunque nótese que necesitarán que la
-cadena de caracteres termine en NUL o en el carácter de línea nueva.
+Las funciones: simple_strtol(), simple_strtoul(), y simple_strtoull()
+explícitamente ignoran los desbordamientos, lo que puede llevar a
+resultados inesperados por las funciones que las llaman. Las funciones
+respectivas kstrtol(), kstrtoll(), kstrtoul(), y kstrtoull() tienden a
+ser reemplazos correctos, aunque nótese que necesitarán que la cadena de
+caracteres termine en NUL o en el carácter de línea nueva.
 
 
 strcpy()
diff --git a/include/linux/kstrtox.h b/include/linux/kstrtox.h
index 529974e22ea799adf5c07b48d803ac9bfc1f85a4..c3ed0ce20be49fba185937e3fd0f949e7be6f9a4 100644
--- a/include/linux/kstrtox.h
+++ b/include/linux/kstrtox.h
@@ -145,7 +145,6 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
-extern long long simple_strtoll(const char *,char **,unsigned int);
 
 static inline int strtobool(const char *s, bool *res)
 {
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 08c14019841af9df6d1c354b4503ecee4c7a9da0..3397a91e07dbad814bb0f2c93d19761a375bedc5 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -150,7 +150,7 @@ EXPORT_SYMBOL(kstrtoull);
  * @res: Where to write the result of the conversion on success.
  *
  * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Preferred over simple_strtoll(). Return code must be checked.
+ * Return code must be checked.
  */
 noinline
 int kstrtoll(const char *s, unsigned int base, long long *res)
diff --git a/lib/test_scanf.c b/lib/test_scanf.c
index b620cf7de503542a98f38c45c76e9043b242746e..73e6e912f39c7746b1aaaf35eeb562c9c2d3fac8 100644
--- a/lib/test_scanf.c
+++ b/lib/test_scanf.c
@@ -727,15 +727,6 @@ static void __init test_simple_strtoull(void)
 	test_simple_strtoxx(unsigned long long, simple_strtoull, "0x%llx", 0);
 }
 
-static void __init test_simple_strtoll(void)
-{
-	test_simple_strtoxx(long long, simple_strtoll, "%lld",	 10);
-	test_simple_strtoxx(long long, simple_strtoll, "%lld",	 0);
-	test_simple_strtoxx(long long, simple_strtoll, "%llx",	 16);
-	test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 16);
-	test_simple_strtoxx(long long, simple_strtoll, "0x%llx", 0);
-}
-
 static void __init test_simple_strtoul(void)
 {
 	test_simple_strtoxx(unsigned long, simple_strtoul, "%lu",   10);
@@ -800,7 +791,6 @@ static void __init selftest(void)
 	test_numbers();
 
 	test_simple_strtoull();
-	test_simple_strtoll();
 	test_simple_strtoul();
 	test_simple_strtol();
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 40f560959b169b4c4ac6154d658cfe76cfd0c5a6..a60d348efb276d66ca07fe464883408df7fdab97 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -144,20 +144,6 @@ static long long simple_strntoll(const char *cp, size_t max_chars, char **endp,
 	return simple_strntoull(cp, max_chars, endp, base);
 }
 
-/**
- * simple_strtoll - convert a string to a signed long long
- * @cp: The start of the string
- * @endp: A pointer to the end of the parsed string will be placed here
- * @base: The number base to use
- *
- * This function has caveats. Please use kstrtoll instead.
- */
-long long simple_strtoll(const char *cp, char **endp, unsigned int base)
-{
-	return simple_strntoll(cp, INT_MAX, endp, base);
-}
-EXPORT_SYMBOL(simple_strtoll);
-
 static noinline_for_stack
 int skip_atoi(const char **s)
 {
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 2/4] vsscanf(): Return -ERANGE on integer overflow

[Demi Marie Obenour]

Userspace sets errno to ERANGE, but the kernel can't do that.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 include/linux/limits.h          |  1 +
 include/linux/mfd/wl1273-core.h |  3 --
 include/vdso/limits.h           |  3 ++
 lib/vsprintf.c                  | 80 ++++++++++++++++++++++++---------
 4 files changed, 63 insertions(+), 24 deletions(-)

diff --git a/include/linux/limits.h b/include/linux/limits.h
index f6bcc936901071f496e3e85bb6e1d93905b12e32..8f7fd85b41fb46e6992d9e5912da00424119227a 100644
--- a/include/linux/limits.h
+++ b/include/linux/limits.h
@@ -8,6 +8,7 @@
 
 #define SIZE_MAX	(~(size_t)0)
 #define SSIZE_MAX	((ssize_t)(SIZE_MAX >> 1))
+#define SSIZE_MIN	(-SSIZE_MAX - 1)
 #define PHYS_ADDR_MAX	(~(phys_addr_t)0)
 
 #define U8_MAX		((u8)~0U)
diff --git a/include/linux/mfd/wl1273-core.h b/include/linux/mfd/wl1273-core.h
index c28cf76d5c31ee1c94a9319a2e2d318bf00283a6..b81a229135ed9f756c749122a8341816031c8311 100644
--- a/include/linux/mfd/wl1273-core.h
+++ b/include/linux/mfd/wl1273-core.h
@@ -204,9 +204,6 @@
 				 WL1273_IS2_TRI_OPT | \
 				 WL1273_IS2_RATE_48K)
 
-#define SCHAR_MIN (-128)
-#define SCHAR_MAX 127
-
 #define WL1273_FR_EVENT			BIT(0)
 #define WL1273_BL_EVENT			BIT(1)
 #define WL1273_RDS_EVENT		BIT(2)
diff --git a/include/vdso/limits.h b/include/vdso/limits.h
index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
--- a/include/vdso/limits.h
+++ b/include/vdso/limits.h
@@ -2,6 +2,9 @@
 #ifndef __VDSO_LIMITS_H
 #define __VDSO_LIMITS_H
 
+#define UCHAR_MAX	((unsigned char)~0U)
+#define SCHAR_MAX	((signed char)(UCHAR_MAX >> 1))
+#define SCHAR_MIN	((signed char)(-SCHAR_MAX - 1))
 #define USHRT_MAX	((unsigned short)~0U)
 #define SHRT_MAX	((short)(USHRT_MAX >> 1))
 #define SHRT_MIN	((short)(-SHRT_MAX - 1))
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a60d348efb276d66ca07fe464883408df7fdab97..9846d2385f5b9e8f3945a5664d81047e97cf10d5 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -59,7 +59,7 @@
 bool no_hash_pointers __ro_after_init;
 EXPORT_SYMBOL_GPL(no_hash_pointers);
 
-static noinline unsigned long long simple_strntoull(const char *startp, size_t max_chars, char **endp, unsigned int base)
+static noinline unsigned long long simple_strntoull(const char *startp, size_t max_chars, char **endp, unsigned int base, bool *overflow)
 {
 	const char *cp;
 	unsigned long long result = 0ULL;
@@ -71,6 +71,8 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m
 	if (prefix_chars < max_chars) {
 		rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars);
 		/* FIXME */
+		if (overflow)
+			*overflow = !!(rv & KSTRTOX_OVERFLOW);
 		cp += (rv & ~KSTRTOX_OVERFLOW);
 	} else {
 		/* Field too short for prefix + digit, skip over without converting */
@@ -94,7 +96,7 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m
 noinline
 unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
 {
-	return simple_strntoull(cp, INT_MAX, endp, base);
+	return simple_strntoull(cp, INT_MAX, endp, base, NULL);
 }
 EXPORT_SYMBOL(simple_strtoull);
 
@@ -130,18 +132,22 @@ long simple_strtol(const char *cp, char **endp, unsigned int base)
 EXPORT_SYMBOL(simple_strtol);
 
 static long long simple_strntoll(const char *cp, size_t max_chars, char **endp,
-				 unsigned int base)
+				 unsigned int base, bool *overflow)
 {
+	unsigned long long minand;
+	bool negate;
+
 	/*
 	 * simple_strntoull() safely handles receiving max_chars==0 in the
 	 * case cp[0] == '-' && max_chars == 1.
 	 * If max_chars == 0 we can drop through and pass it to simple_strntoull()
 	 * and the content of *cp is irrelevant.
 	 */
-	if (*cp == '-' && max_chars > 0)
-		return -simple_strntoull(cp + 1, max_chars - 1, endp, base);
-
-	return simple_strntoull(cp, max_chars, endp, base);
+	negate = *cp == '-' && max_chars > 0;
+	minand = simple_strntoull(cp + negate, max_chars - negate, endp, base, overflow);
+	if (minand > (unsigned long long)LONG_MAX + negate)
+		*overflow = true;
+	return negate ? -minand : minand;
 }
 
 static noinline_for_stack
@@ -3427,7 +3433,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		unsigned long long u;
 	} val;
 	s16 field_width;
-	bool is_sign;
+	bool is_sign, overflow;
 
 	while (*fmt) {
 		/* skip any white space in format */
@@ -3635,45 +3641,77 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		if (is_sign)
 			val.s = simple_strntoll(str,
 						field_width >= 0 ? field_width : INT_MAX,
-						&next, base);
+						&next, base, &overflow);
 		else
 			val.u = simple_strntoull(str,
 						 field_width >= 0 ? field_width : INT_MAX,
-						 &next, base);
+						 &next, base, &overflow);
+		if (unlikely(overflow))
+			return -ERANGE;
 
 		switch (qualifier) {
 		case 'H':	/* that's 'hh' in format */
-			if (is_sign)
+			if (is_sign) {
+				if (unlikely(val.s < SCHAR_MIN || val.s > SCHAR_MAX))
+					return -ERANGE;
 				*va_arg(args, signed char *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > UCHAR_MAX))
+					return -ERANGE;
 				*va_arg(args, unsigned char *) = val.u;
+			}
 			break;
 		case 'h':
-			if (is_sign)
+			if (is_sign) {
+				if (unlikely(val.s < SHRT_MIN || val.s > SHRT_MAX))
+					return -ERANGE;
 				*va_arg(args, short *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > USHRT_MAX))
+					return -ERANGE;
 				*va_arg(args, unsigned short *) = val.u;
+			}
 			break;
 		case 'l':
-			if (is_sign)
+			if (is_sign) {
+				if (unlikely(val.s < LONG_MIN || val.s > LONG_MAX))
+					return -ERANGE;
 				*va_arg(args, long *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > ULONG_MAX))
+					return -ERANGE;
 				*va_arg(args, unsigned long *) = val.u;
+			}
 			break;
 		case 'L':
-			if (is_sign)
+			/* No overflow check needed */
+			if (is_sign) {
 				*va_arg(args, long long *) = val.s;
-			else
+			} else {
 				*va_arg(args, unsigned long long *) = val.u;
+			}
 			break;
 		case 'z':
-			*va_arg(args, size_t *) = val.u;
+			if (is_sign) {
+				if (unlikely(val.s < SSIZE_MIN || val.s > SSIZE_MAX))
+					return -ERANGE;
+				*va_arg(args, ssize_t *) = val.s;
+			} else {
+				if (unlikely(val.u > SIZE_MAX))
+					return -ERANGE;
+				*va_arg(args, size_t *) = val.u;
+			}
 			break;
 		default:
-			if (is_sign)
+			if (is_sign) {
+				if (unlikely(val.s < INT_MIN || val.s > INT_MAX))
+					return -ERANGE;
 				*va_arg(args, int *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > UINT_MAX))
+					return -ERANGE;
 				*va_arg(args, unsigned int *) = val.u;
+			}
 			break;
 		}
 		num++;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 3/4] Add strict version of vsscanf()

[Demi Marie Obenour]

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 include/linux/kernel.h |  4 ++++
 lib/vsprintf.c         | 43 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0d91e0af01250c1d82f4a2ea562d2619b9cc6e9c..b348b84ce9c4e95031f67e0cbac5de8deca69aac 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -227,8 +227,12 @@ const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
+extern __scanf(2, 3)
+int sscanf_strict(const char *, const char *, ...);
 extern __scanf(2, 0)
 int vsscanf(const char *, const char *, va_list);
+extern __scanf(2, 0)
+int vsscanf_strict(const char *, const char *, va_list);
 
 extern int no_hash_pointers_enable(char *str);
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9846d2385f5b9e8f3945a5664d81047e97cf10d5..2dae357b367e1da8b1004ed6e85e051a045ca36b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -3414,6 +3414,8 @@ EXPORT_SYMBOL_GPL(bprintf);
 
 #endif /* CONFIG_BINARY_PRINTF */
 
+static int vsscanf_internal(const char *buf, const char *fmt, va_list args, bool strict);
+
 /**
  * vsscanf - Unformat a buffer into a list of arguments
  * @buf:	input buffer
@@ -3421,6 +3423,23 @@ EXPORT_SYMBOL_GPL(bprintf);
  * @args:	arguments
  */
 int vsscanf(const char *buf, const char *fmt, va_list args)
+{
+	return vsscanf_internal(buf, fmt, args, false);
+}
+
+/**
+ * vsscanf_strict - Unformat a buffer into a list of arguments, but
+ *                  do not skip spaces.
+ * @buf:	input buffer
+ * @fmt:	format of buffer
+ * @args:	arguments
+ */
+int vsscanf_strict(const char *buf, const char *fmt, va_list args)
+{
+	return vsscanf_internal(buf, fmt, args, true);
+}
+
+static int vsscanf_internal(const char *buf, const char *fmt, va_list args, bool strict)
 {
 	const char *str = buf;
 	char *next;
@@ -3530,8 +3549,10 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			char *s = (char *)va_arg(args, char *);
 			if (field_width == -1)
 				field_width = SHRT_MAX;
-			/* first, skip leading white space in buffer */
-			str = skip_spaces(str);
+			if (!strict) {
+				/* first, skip leading white space in buffer */
+				str = skip_spaces(str);
+			}
 
 			/* now copy until next white space */
 			while (*str && !isspace(*str) && field_width--)
@@ -3621,7 +3642,8 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		/* have some sort of integer conversion.
 		 * first, skip white space in buffer.
 		 */
-		str = skip_spaces(str);
+		if (!strict)
+			str = skip_spaces(str);
 
 		digit = *str;
 		if (is_sign && digit == '-') {
@@ -3721,6 +3743,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		str = next;
 	}
 
+	if (strict && *str)
+		return -EINVAL;
+
 	return num;
 }
 EXPORT_SYMBOL(vsscanf);
@@ -3743,3 +3768,15 @@ int sscanf(const char *buf, const char *fmt, ...)
 	return i;
 }
 EXPORT_SYMBOL(sscanf);
+int sscanf_strict(const char *buf, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = vsscanf_strict(buf, fmt, args);
+	va_end(args);
+
+	return i;
+}
+EXPORT_SYMBOL(sscanf_strict);
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[PATCH 4/4] Strict XenStore entry parsing

[Demi Marie Obenour]

This uses the newly-introduced strict version of sscanf().

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/xen/xenbus/xenbus_xs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 12e02eb01f5991b31db451cc57037205359b347f..88e94269c9221d16d1a97e59399058e870675729 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -569,16 +569,20 @@ int xenbus_scanf(struct xenbus_transaction t,
 		 const char *dir, const char *node, const char *fmt, ...)
 {
 	va_list ap;
-	int ret;
+	int ret = 0;
+	unsigned int len;
 	char *val;
 
-	val = xenbus_read(t, dir, node, NULL);
+	val = xenbus_read(t, dir, node, &len);
 	if (IS_ERR(val))
 		return PTR_ERR(val);
+	if (strlen(val) != len)
+		goto bad;
 
 	va_start(ap, fmt);
-	ret = vsscanf(val, fmt, ap);
+	ret = vsscanf_strict(val, fmt, ap);
 	va_end(ap);
+bad:
 	kfree(val);
 	/* Distinctive errno. */
 	if (ret == 0)
@@ -636,15 +640,18 @@ int xenbus_gather(struct xenbus_transaction t, const char *dir, ...)
 	while (ret == 0 && (name = va_arg(ap, char *)) != NULL) {
 		const char *fmt = va_arg(ap, char *);
 		void *result = va_arg(ap, void *);
+		unsigned len;
 		char *p;
 
-		p = xenbus_read(t, dir, name, NULL);
+		p = xenbus_read(t, dir, name, &len);
 		if (IS_ERR(p)) {
 			ret = PTR_ERR(p);
 			break;
 		}
-		if (fmt) {
-			if (sscanf(p, fmt, result) == 0)
+		if (strlen(p) != len)
+			ret = -EINVAL;
+		else if (fmt) {
+			if (sscanf_strict(p, fmt, result) <= 0)
 				ret = -EINVAL;
 			kfree(p);
 		} else
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH 4/4] Strict XenStore entry parsing

[Matthew Wilcox]

On Fri, Jun 09, 2023 at 10:57:59PM -0400, Demi Marie Obenour wrote:
> This uses the newly-introduced strict version of sscanf().

I can see that.  Why does it do that?

Documentation/process/5.Posting.rst

(in general, there is a lack of detail across all four of these patches
justifying why any of this work is being done.  it isn't obvious to me
why skipping leading whitespace is bad in this context)

Re: [PATCH 1/4] Rip out simple_strtoll()

[kernel test robot]

Hi Demi,

kernel test robot noticed the following build errors:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on lee-leds/for-leds-next linus/master v6.4-rc5 next-20230609]
[cannot apply to xen-tip/linux-next lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Demi-Marie-Obenour/vsscanf-Return-ERANGE-on-integer-overflow/20230610-110026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20230610025759.1813-1-demi%40invisiblethingslab.com
patch subject: [PATCH 1/4] Rip out simple_strtoll()
config: arm-randconfig-r046-20230610 (https://download.01.org/0day-ci/archive/20230610/202306101334.lDZERsKo-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        git remote add lee-mfd https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
        git fetch lee-mfd for-mfd-next
        git checkout lee-mfd/for-mfd-next
        b4 shazam https://lore.kernel.org/r/20230610025759.1813-1-demi@invisiblethingslab.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/md/bcache/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306101334.lDZERsKo-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/md/bcache/util.c:82:1: error: call to undeclared function 'simple_strtoll'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
   STRTO_H(strtoll, long long)
   ^
   drivers/md/bcache/util.c:28:11: note: expanded from macro 'STRTO_H'
           type i = simple_ ## name(cp, &e, 10);                   \
                    ^
   <scratch space>:22:1: note: expanded from here
   simple_strtoll
   ^
   1 error generated.


vim +/simple_strtoll +82 drivers/md/bcache/util.c

cafe563591446c Kent Overstreet 2013-03-23  79  
cafe563591446c Kent Overstreet 2013-03-23  80  STRTO_H(strtoint, int)
cafe563591446c Kent Overstreet 2013-03-23  81  STRTO_H(strtouint, unsigned int)
cafe563591446c Kent Overstreet 2013-03-23 @82  STRTO_H(strtoll, long long)
cafe563591446c Kent Overstreet 2013-03-23  83  STRTO_H(strtoull, unsigned long long)
cafe563591446c Kent Overstreet 2013-03-23  84  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] Rip out simple_strtoll()

[kernel test robot]

Hi Demi,

kernel test robot noticed the following build errors:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on lee-leds/for-leds-next linus/master v6.4-rc5 next-20230609]
[cannot apply to xen-tip/linux-next lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Demi-Marie-Obenour/vsscanf-Return-ERANGE-on-integer-overflow/20230610-110026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20230610025759.1813-1-demi%40invisiblethingslab.com
patch subject: [PATCH 1/4] Rip out simple_strtoll()
config: csky-randconfig-r011-20230610 (https://download.01.org/0day-ci/archive/20230610/202306101317.YiBrl6OZ-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add lee-mfd https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
        git fetch lee-mfd for-mfd-next
        git checkout lee-mfd/for-mfd-next
        b4 shazam https://lore.kernel.org/r/20230610025759.1813-1-demi@invisiblethingslab.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash drivers/md/bcache/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306101317.YiBrl6OZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/md/bcache/util.c: In function 'bch_strtoll_h':
>> drivers/md/bcache/util.c:28:18: error: implicit declaration of function 'simple_strtoll'; did you mean 'simple_strtoull'? [-Werror=implicit-function-declaration]
      28 |         type i = simple_ ## name(cp, &e, 10);                   \
         |                  ^~~~~~~
   drivers/md/bcache/util.c:82:1: note: in expansion of macro 'STRTO_H'
      82 | STRTO_H(strtoll, long long)
         | ^~~~~~~
   cc1: some warnings being treated as errors


vim +28 drivers/md/bcache/util.c

cafe563591446c Kent Overstreet     2013-03-23  22  
cafe563591446c Kent Overstreet     2013-03-23  23  #define STRTO_H(name, type)					\
169ef1cf6171d3 Kent Overstreet     2013-03-28  24  int bch_ ## name ## _h(const char *cp, type *res)		\
cafe563591446c Kent Overstreet     2013-03-23  25  {								\
cafe563591446c Kent Overstreet     2013-03-23  26  	int u = 0;						\
cafe563591446c Kent Overstreet     2013-03-23  27  	char *e;						\
cafe563591446c Kent Overstreet     2013-03-23 @28  	type i = simple_ ## name(cp, &e, 10);			\
cafe563591446c Kent Overstreet     2013-03-23  29  								\
cafe563591446c Kent Overstreet     2013-03-23  30  	switch (tolower(*e)) {					\
cafe563591446c Kent Overstreet     2013-03-23  31  	default:						\
cafe563591446c Kent Overstreet     2013-03-23  32  		return -EINVAL;					\
cafe563591446c Kent Overstreet     2013-03-23  33  	case 'y':						\
cafe563591446c Kent Overstreet     2013-03-23  34  	case 'z':						\
cafe563591446c Kent Overstreet     2013-03-23  35  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  36  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  37  	case 'e':						\
cafe563591446c Kent Overstreet     2013-03-23  38  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  39  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  40  	case 'p':						\
cafe563591446c Kent Overstreet     2013-03-23  41  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  42  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  43  	case 't':						\
cafe563591446c Kent Overstreet     2013-03-23  44  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  45  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  46  	case 'g':						\
cafe563591446c Kent Overstreet     2013-03-23  47  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  48  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  49  	case 'm':						\
cafe563591446c Kent Overstreet     2013-03-23  50  		u++;						\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  51  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  52  	case 'k':						\
cafe563591446c Kent Overstreet     2013-03-23  53  		u++;						\
cafe563591446c Kent Overstreet     2013-03-23  54  		if (e++ == cp)					\
cafe563591446c Kent Overstreet     2013-03-23  55  			return -EINVAL;				\
df561f6688fef7 Gustavo A. R. Silva 2020-08-23  56  		fallthrough;					\
cafe563591446c Kent Overstreet     2013-03-23  57  	case '\n':						\
cafe563591446c Kent Overstreet     2013-03-23  58  	case '\0':						\
cafe563591446c Kent Overstreet     2013-03-23  59  		if (*e == '\n')					\
cafe563591446c Kent Overstreet     2013-03-23  60  			e++;					\
cafe563591446c Kent Overstreet     2013-03-23  61  	}							\
cafe563591446c Kent Overstreet     2013-03-23  62  								\
cafe563591446c Kent Overstreet     2013-03-23  63  	if (*e)							\
cafe563591446c Kent Overstreet     2013-03-23  64  		return -EINVAL;					\
cafe563591446c Kent Overstreet     2013-03-23  65  								\
cafe563591446c Kent Overstreet     2013-03-23  66  	while (u--) {						\
cafe563591446c Kent Overstreet     2013-03-23  67  		if ((type) ~0 > 0 &&				\
cafe563591446c Kent Overstreet     2013-03-23  68  		    (type) ~0 / 1024 <= i)			\
cafe563591446c Kent Overstreet     2013-03-23  69  			return -EINVAL;				\
cafe563591446c Kent Overstreet     2013-03-23  70  		if ((i > 0 && ANYSINT_MAX(type) / 1024 < i) ||	\
cafe563591446c Kent Overstreet     2013-03-23  71  		    (i < 0 && -ANYSINT_MAX(type) / 1024 > i))	\
cafe563591446c Kent Overstreet     2013-03-23  72  			return -EINVAL;				\
cafe563591446c Kent Overstreet     2013-03-23  73  		i *= 1024;					\
cafe563591446c Kent Overstreet     2013-03-23  74  	}							\
cafe563591446c Kent Overstreet     2013-03-23  75  								\
cafe563591446c Kent Overstreet     2013-03-23  76  	*res = i;						\
cafe563591446c Kent Overstreet     2013-03-23  77  	return 0;						\
cafe563591446c Kent Overstreet     2013-03-23  78  }								\
cafe563591446c Kent Overstreet     2013-03-23  79  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] Rip out simple_strtoll()

[Christoph Hellwig]

I'd maybe say remove instead of "rip out", but otherwise this looks
good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/4] vsscanf(): Return -ERANGE on integer overflow

[Christoph Hellwig]

[Adding Richard and Linus as they're having another overflow checking
discussion and we should probably merge those]

On Fri, Jun 09, 2023 at 10:57:57PM -0400, Demi Marie Obenour wrote:
> Userspace sets errno to ERANGE, but the kernel can't do that.

That seems like a very parse commit log, and also kinda besides
the point - the kernel always returns error in-line and not through
errno.  I think you need to document here why we want to do the
overflow checking (not that I doubt it, but it really needs to be
in the commit message).

Leaving the rest of the quote here for the new arrivals.

> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  include/linux/limits.h          |  1 +
>  include/linux/mfd/wl1273-core.h |  3 --
>  include/vdso/limits.h           |  3 ++
>  lib/vsprintf.c                  | 80 ++++++++++++++++++++++++---------
>  4 files changed, 63 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/limits.h b/include/linux/limits.h
> index f6bcc936901071f496e3e85bb6e1d93905b12e32..8f7fd85b41fb46e6992d9e5912da00424119227a 100644
> --- a/include/linux/limits.h
> +++ b/include/linux/limits.h
> @@ -8,6 +8,7 @@
>  
>  #define SIZE_MAX	(~(size_t)0)
>  #define SSIZE_MAX	((ssize_t)(SIZE_MAX >> 1))
> +#define SSIZE_MIN	(-SSIZE_MAX - 1)
>  #define PHYS_ADDR_MAX	(~(phys_addr_t)0)
>  
>  #define U8_MAX		((u8)~0U)
> diff --git a/include/linux/mfd/wl1273-core.h b/include/linux/mfd/wl1273-core.h
> index c28cf76d5c31ee1c94a9319a2e2d318bf00283a6..b81a229135ed9f756c749122a8341816031c8311 100644
> --- a/include/linux/mfd/wl1273-core.h
> +++ b/include/linux/mfd/wl1273-core.h
> @@ -204,9 +204,6 @@
>  				 WL1273_IS2_TRI_OPT | \
>  				 WL1273_IS2_RATE_48K)
>  
> -#define SCHAR_MIN (-128)
> -#define SCHAR_MAX 127
> -
>  #define WL1273_FR_EVENT			BIT(0)
>  #define WL1273_BL_EVENT			BIT(1)
>  #define WL1273_RDS_EVENT		BIT(2)
> diff --git a/include/vdso/limits.h b/include/vdso/limits.h
> index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
> --- a/include/vdso/limits.h
> +++ b/include/vdso/limits.h
> @@ -2,6 +2,9 @@
>  #ifndef __VDSO_LIMITS_H
>  #define __VDSO_LIMITS_H
>  
> +#define UCHAR_MAX	((unsigned char)~0U)
> +#define SCHAR_MAX	((signed char)(UCHAR_MAX >> 1))
> +#define SCHAR_MIN	((signed char)(-SCHAR_MAX - 1))
>  #define USHRT_MAX	((unsigned short)~0U)
>  #define SHRT_MAX	((short)(USHRT_MAX >> 1))
>  #define SHRT_MIN	((short)(-SHRT_MAX - 1))
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a60d348efb276d66ca07fe464883408df7fdab97..9846d2385f5b9e8f3945a5664d81047e97cf10d5 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -59,7 +59,7 @@
>  bool no_hash_pointers __ro_after_init;
>  EXPORT_SYMBOL_GPL(no_hash_pointers);
>  
> -static noinline unsigned long long simple_strntoull(const char *startp, size_t max_chars, char **endp, unsigned int base)
> +static noinline unsigned long long simple_strntoull(const char *startp, size_t max_chars, char **endp, unsigned int base, bool *overflow)
>  {
>  	const char *cp;
>  	unsigned long long result = 0ULL;
> @@ -71,6 +71,8 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m
>  	if (prefix_chars < max_chars) {
>  		rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars);
>  		/* FIXME */
> +		if (overflow)
> +			*overflow = !!(rv & KSTRTOX_OVERFLOW);
>  		cp += (rv & ~KSTRTOX_OVERFLOW);
>  	} else {
>  		/* Field too short for prefix + digit, skip over without converting */
> @@ -94,7 +96,7 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m
>  noinline
>  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
>  {
> -	return simple_strntoull(cp, INT_MAX, endp, base);
> +	return simple_strntoull(cp, INT_MAX, endp, base, NULL);
>  }
>  EXPORT_SYMBOL(simple_strtoull);
>  
> @@ -130,18 +132,22 @@ long simple_strtol(const char *cp, char **endp, unsigned int base)
>  EXPORT_SYMBOL(simple_strtol);
>  
>  static long long simple_strntoll(const char *cp, size_t max_chars, char **endp,
> -				 unsigned int base)
> +				 unsigned int base, bool *overflow)
>  {
> +	unsigned long long minand;
> +	bool negate;
> +
>  	/*
>  	 * simple_strntoull() safely handles receiving max_chars==0 in the
>  	 * case cp[0] == '-' && max_chars == 1.
>  	 * If max_chars == 0 we can drop through and pass it to simple_strntoull()
>  	 * and the content of *cp is irrelevant.
>  	 */
> -	if (*cp == '-' && max_chars > 0)
> -		return -simple_strntoull(cp + 1, max_chars - 1, endp, base);
> -
> -	return simple_strntoull(cp, max_chars, endp, base);
> +	negate = *cp == '-' && max_chars > 0;
> +	minand = simple_strntoull(cp + negate, max_chars - negate, endp, base, overflow);
> +	if (minand > (unsigned long long)LONG_MAX + negate)
> +		*overflow = true;
> +	return negate ? -minand : minand;
>  }
>  
>  static noinline_for_stack
> @@ -3427,7 +3433,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
>  		unsigned long long u;
>  	} val;
>  	s16 field_width;
> -	bool is_sign;
> +	bool is_sign, overflow;
>  
>  	while (*fmt) {
>  		/* skip any white space in format */
> @@ -3635,45 +3641,77 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
>  		if (is_sign)
>  			val.s = simple_strntoll(str,
>  						field_width >= 0 ? field_width : INT_MAX,
> -						&next, base);
> +						&next, base, &overflow);
>  		else
>  			val.u = simple_strntoull(str,
>  						 field_width >= 0 ? field_width : INT_MAX,
> -						 &next, base);
> +						 &next, base, &overflow);
> +		if (unlikely(overflow))
> +			return -ERANGE;
>  
>  		switch (qualifier) {
>  		case 'H':	/* that's 'hh' in format */
> -			if (is_sign)
> +			if (is_sign) {
> +				if (unlikely(val.s < SCHAR_MIN || val.s > SCHAR_MAX))
> +					return -ERANGE;
>  				*va_arg(args, signed char *) = val.s;
> -			else
> +			} else {
> +				if (unlikely(val.u > UCHAR_MAX))
> +					return -ERANGE;
>  				*va_arg(args, unsigned char *) = val.u;
> +			}
>  			break;
>  		case 'h':
> -			if (is_sign)
> +			if (is_sign) {
> +				if (unlikely(val.s < SHRT_MIN || val.s > SHRT_MAX))
> +					return -ERANGE;
>  				*va_arg(args, short *) = val.s;
> -			else
> +			} else {
> +				if (unlikely(val.u > USHRT_MAX))
> +					return -ERANGE;
>  				*va_arg(args, unsigned short *) = val.u;
> +			}
>  			break;
>  		case 'l':
> -			if (is_sign)
> +			if (is_sign) {
> +				if (unlikely(val.s < LONG_MIN || val.s > LONG_MAX))
> +					return -ERANGE;
>  				*va_arg(args, long *) = val.s;
> -			else
> +			} else {
> +				if (unlikely(val.u > ULONG_MAX))
> +					return -ERANGE;
>  				*va_arg(args, unsigned long *) = val.u;
> +			}
>  			break;
>  		case 'L':
> -			if (is_sign)
> +			/* No overflow check needed */
> +			if (is_sign) {
>  				*va_arg(args, long long *) = val.s;
> -			else
> +			} else {
>  				*va_arg(args, unsigned long long *) = val.u;
> +			}
>  			break;
>  		case 'z':
> -			*va_arg(args, size_t *) = val.u;
> +			if (is_sign) {
> +				if (unlikely(val.s < SSIZE_MIN || val.s > SSIZE_MAX))
> +					return -ERANGE;
> +				*va_arg(args, ssize_t *) = val.s;
> +			} else {
> +				if (unlikely(val.u > SIZE_MAX))
> +					return -ERANGE;
> +				*va_arg(args, size_t *) = val.u;
> +			}
>  			break;
>  		default:
> -			if (is_sign)
> +			if (is_sign) {
> +				if (unlikely(val.s < INT_MIN || val.s > INT_MAX))
> +					return -ERANGE;
>  				*va_arg(args, int *) = val.s;
> -			else
> +			} else {
> +				if (unlikely(val.u > UINT_MAX))
> +					return -ERANGE;
>  				*va_arg(args, unsigned int *) = val.u;
> +			}
>  			break;
>  		}
>  		num++;
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
> 
---end quoted text---

Re: [PATCH 3/4] Add strict version of vsscanf()

[Christoph Hellwig]

This needs a real commit message explaining what the strict version
does and why, and why we can't just make the normal version more
strict.

Re: [PATCH 2/4] vsscanf(): Return -ERANGE on integer overflow

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christoph Hellwig" <hch@infradead.org>
> [Adding Richard and Linus as they're having another overflow checking
> discussion and we should probably merge those]

Thx for letting me know!
 
> On Fri, Jun 09, 2023 at 10:57:57PM -0400, Demi Marie Obenour wrote:
>> Userspace sets errno to ERANGE, but the kernel can't do that.
> 
> That seems like a very parse commit log, and also kinda besides
> the point - the kernel always returns error in-line and not through
> errno.  I think you need to document here why we want to do the
> overflow checking (not that I doubt it, but it really needs to be
> in the commit message).
> 
> Leaving the rest of the quote here for the new arrivals.
> 
>> 
>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>> ---
>>  include/linux/limits.h          |  1 +
>>  include/linux/mfd/wl1273-core.h |  3 --
>>  include/vdso/limits.h           |  3 ++
>>  lib/vsprintf.c                  | 80 ++++++++++++++++++++++++---------
>>  4 files changed, 63 insertions(+), 24 deletions(-)
>> 
>> diff --git a/include/linux/limits.h b/include/linux/limits.h
>> index
>> f6bcc936901071f496e3e85bb6e1d93905b12e32..8f7fd85b41fb46e6992d9e5912da00424119227a
>> 100644
>> --- a/include/linux/limits.h
>> +++ b/include/linux/limits.h
>> @@ -8,6 +8,7 @@
>>  
>>  #define SIZE_MAX	(~(size_t)0)
>>  #define SSIZE_MAX	((ssize_t)(SIZE_MAX >> 1))
>> +#define SSIZE_MIN	(-SSIZE_MAX - 1)
>>  #define PHYS_ADDR_MAX	(~(phys_addr_t)0)
>>  
>>  #define U8_MAX		((u8)~0U)
>> diff --git a/include/linux/mfd/wl1273-core.h b/include/linux/mfd/wl1273-core.h
>> index
>> c28cf76d5c31ee1c94a9319a2e2d318bf00283a6..b81a229135ed9f756c749122a8341816031c8311
>> 100644
>> --- a/include/linux/mfd/wl1273-core.h
>> +++ b/include/linux/mfd/wl1273-core.h
>> @@ -204,9 +204,6 @@
>>  				 WL1273_IS2_TRI_OPT | \
>>  				 WL1273_IS2_RATE_48K)
>>  
>> -#define SCHAR_MIN (-128)
>> -#define SCHAR_MAX 127
>> -
>>  #define WL1273_FR_EVENT			BIT(0)
>>  #define WL1273_BL_EVENT			BIT(1)
>>  #define WL1273_RDS_EVENT		BIT(2)
>> diff --git a/include/vdso/limits.h b/include/vdso/limits.h
>> index
>> 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4
>> 100644
>> --- a/include/vdso/limits.h
>> +++ b/include/vdso/limits.h
>> @@ -2,6 +2,9 @@
>>  #ifndef __VDSO_LIMITS_H
>>  #define __VDSO_LIMITS_H
>>  
>> +#define UCHAR_MAX	((unsigned char)~0U)
>> +#define SCHAR_MAX	((signed char)(UCHAR_MAX >> 1))
>> +#define SCHAR_MIN	((signed char)(-SCHAR_MAX - 1))
>>  #define USHRT_MAX	((unsigned short)~0U)
>>  #define SHRT_MAX	((short)(USHRT_MAX >> 1))
>>  #define SHRT_MIN	((short)(-SHRT_MAX - 1))
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index
>> a60d348efb276d66ca07fe464883408df7fdab97..9846d2385f5b9e8f3945a5664d81047e97cf10d5
>> 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -59,7 +59,7 @@
>>  bool no_hash_pointers __ro_after_init;
>>  EXPORT_SYMBOL_GPL(no_hash_pointers);
>>  
>> -static noinline unsigned long long simple_strntoull(const char *startp, size_t
>> max_chars, char **endp, unsigned int base)
>> +static noinline unsigned long long simple_strntoull(const char *startp, size_t
>> max_chars, char **endp, unsigned int base, bool *overflow)
>>  {
>>  	const char *cp;
>>  	unsigned long long result = 0ULL;
>> @@ -71,6 +71,8 @@ static noinline unsigned long long simple_strntoull(const char
>> *startp, size_t m
>>  	if (prefix_chars < max_chars) {
>>  		rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars);
>>  		/* FIXME */
>> +		if (overflow)
>> +			*overflow = !!(rv & KSTRTOX_OVERFLOW);
>>  		cp += (rv & ~KSTRTOX_OVERFLOW);
>>  	} else {
>>  		/* Field too short for prefix + digit, skip over without converting */
>> @@ -94,7 +96,7 @@ static noinline unsigned long long simple_strntoull(const char
>> *startp, size_t m
>>  noinline
>>  unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int
>>  base)
>>  {
>> -	return simple_strntoull(cp, INT_MAX, endp, base);
>> +	return simple_strntoull(cp, INT_MAX, endp, base, NULL);
>>  }
>>  EXPORT_SYMBOL(simple_strtoull);
>>  
>> @@ -130,18 +132,22 @@ long simple_strtol(const char *cp, char **endp, unsigned
>> int base)
>>  EXPORT_SYMBOL(simple_strtol);
>>  
>>  static long long simple_strntoll(const char *cp, size_t max_chars, char **endp,
>> -				 unsigned int base)
>> +				 unsigned int base, bool *overflow)
>>  {
>> +	unsigned long long minand;
>> +	bool negate;
>> +
>>  	/*
>>  	 * simple_strntoull() safely handles receiving max_chars==0 in the
>>  	 * case cp[0] == '-' && max_chars == 1.
>>  	 * If max_chars == 0 we can drop through and pass it to simple_strntoull()
>>  	 * and the content of *cp is irrelevant.
>>  	 */
>> -	if (*cp == '-' && max_chars > 0)
>> -		return -simple_strntoull(cp + 1, max_chars - 1, endp, base);
>> -
>> -	return simple_strntoull(cp, max_chars, endp, base);
>> +	negate = *cp == '-' && max_chars > 0;
>> +	minand = simple_strntoull(cp + negate, max_chars - negate, endp, base,
>> overflow);
>> +	if (minand > (unsigned long long)LONG_MAX + negate)
>> +		*overflow = true;
>> +	return negate ? -minand : minand;
>>  }
>>  
>>  static noinline_for_stack
>> @@ -3427,7 +3433,7 @@ int vsscanf(const char *buf, const char *fmt, va_list
>> args)
>>  		unsigned long long u;
>>  	} val;
>>  	s16 field_width;
>> -	bool is_sign;
>> +	bool is_sign, overflow;
>>  
>>  	while (*fmt) {
>>  		/* skip any white space in format */
>> @@ -3635,45 +3641,77 @@ int vsscanf(const char *buf, const char *fmt, va_list
>> args)
>>  		if (is_sign)
>>  			val.s = simple_strntoll(str,
>>  						field_width >= 0 ? field_width : INT_MAX,
>> -						&next, base);
>> +						&next, base, &overflow);
>>  		else
>>  			val.u = simple_strntoull(str,
>>  						 field_width >= 0 ? field_width : INT_MAX,
>> -						 &next, base);
>> +						 &next, base, &overflow);
>> +		if (unlikely(overflow))
>> +			return -ERANGE;
>>  
>>  		switch (qualifier) {
>>  		case 'H':	/* that's 'hh' in format */
>> -			if (is_sign)
>> +			if (is_sign) {
>> +				if (unlikely(val.s < SCHAR_MIN || val.s > SCHAR_MAX))
>> +					return -ERANGE;
>>  				*va_arg(args, signed char *) = val.s;
>> -			else
>> +			} else {
>> +				if (unlikely(val.u > UCHAR_MAX))
>> +					return -ERANGE;
>>  				*va_arg(args, unsigned char *) = val.u;
>> +			}
>>  			break;
>>  		case 'h':
>> -			if (is_sign)
>> +			if (is_sign) {
>> +				if (unlikely(val.s < SHRT_MIN || val.s > SHRT_MAX))
>> +					return -ERANGE;

Returning a negative value here will break many existing in-kernel users.
Most users just check for the number of matched elements.

Linus' idea was returning 0 upon overflow (nothing matched) and allowing
overflows (if really needed) by adding a new format string qualifier "!".
e.g. "%!d".

Thanks,
//richard

Re: [PATCH 2/4] vsscanf(): Return -ERANGE on integer overflow

[kernel test robot]

Hi Demi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on lee-mfd/for-mfd-next]
[also build test WARNING on lee-leds/for-leds-next linus/master v6.4-rc5 next-20230609]
[cannot apply to xen-tip/linux-next lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Demi-Marie-Obenour/vsscanf-Return-ERANGE-on-integer-overflow/20230610-110026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
patch link:    https://lore.kernel.org/r/20230610025759.1813-2-demi%40invisiblethingslab.com
patch subject: [PATCH 2/4] vsscanf(): Return -ERANGE on integer overflow
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230610/202306102055.ZSJK8Xsj-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
        git remote add lee-mfd https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git
        git fetch lee-mfd for-mfd-next
        git checkout lee-mfd/for-mfd-next
        b4 shazam https://lore.kernel.org/r/20230610025759.1813-2-demi@invisiblethingslab.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 olddefconfig
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/staging/media/atomisp/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306102055.ZSJK8Xsj-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/staging/media/atomisp//pci/ia_css_acc_types.h:25,
                    from drivers/staging/media/atomisp//pci/ia_css_pipe_public.h:29,
                    from drivers/staging/media/atomisp//pci/ia_css_stream_public.h:25,
                    from drivers/staging/media/atomisp//pci/runtime/binary/interface/ia_css_binary.h:23,
                    from drivers/staging/media/atomisp//pci/runtime/debug/interface/ia_css_debug.h:25,
                    from drivers/staging/media/atomisp/pci/isp/kernels/macc/macc_1.0/ia_css_macc.host.c:18:
>> drivers/staging/media/atomisp//pci/hive_isp_css_include/platform_support.h:30: warning: "UCHAR_MAX" redefined
      30 | #define UCHAR_MAX  (255)
         | 
   In file included from include/linux/limits.h:7,
                    from drivers/staging/media/atomisp//pci/hive_isp_css_include/type_support.h:37,
                    from drivers/staging/media/atomisp//pci/ia_css_types.h:27,
                    from drivers/staging/media/atomisp/pci/isp/kernels/macc/macc_1.0/ia_css_macc.host.c:16:
   include/vdso/limits.h:5: note: this is the location of the previous definition
       5 | #define UCHAR_MAX       ((unsigned char)~0U)
         | 


vim +/UCHAR_MAX +30 drivers/staging/media/atomisp//pci/hive_isp_css_include/platform_support.h

ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/platform_support.h Mauro Carvalho Chehab 2020-04-19  27  
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/platform_support.h Mauro Carvalho Chehab 2020-04-19  28  #define UINT16_MAX USHRT_MAX
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/platform_support.h Mauro Carvalho Chehab 2020-04-19  29  #define UINT32_MAX UINT_MAX
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/platform_support.h Mauro Carvalho Chehab 2020-04-19 @30  #define UCHAR_MAX  (255)
ad85094b293e40 drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/platform_support.h Mauro Carvalho Chehab 2020-04-19  31  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] Rip out simple_strtoll()

[Federico Vaga]

On 2023-06-10 04:57, Demi Marie Obenour wrote:
> It is not used anywhere but its own unit tests.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
>  Documentation/dev-tools/checkpatch.rst             |  9 ++++-----
>  Documentation/process/deprecated.rst               |  5 ++---
>  .../translations/it_IT/process/deprecated.rst      |  9 ++++-----
>  .../translations/sp_SP/process/deprecated.rst      | 14 +++++++-------
>  include/linux/kstrtox.h                            |  1 -
>  lib/kstrtox.c                                      |  2 +-
>  lib/test_scanf.c                                   | 10 ----------
>  lib/vsprintf.c                                     | 14 --------------
>  8 files changed, 18 insertions(+), 46 deletions(-)

> --- a/Documentation/translations/it_IT/process/deprecated.rst
> +++ b/Documentation/translations/it_IT/process/deprecated.rst
> @@ -118,12 +118,11 @@ Per maggiori dettagli fate riferimento a
> array3_size() e flex_array_size(), ma
>  anche le funzioni della famiglia check_mul_overflow(), 
> check_add_overflow(),
>  check_sub_overflow(), e check_shl_overflow().
> 
> -simple_strtol(), simple_strtoll(), simple_strtoul(), simple_strtoull()
> +simple_strtol(), simple_strtoul(), simple_strtoull()
>  ----------------------------------------------------------------------
> -Le funzioni simple_strtol(), simple_strtoll(),
> -simple_strtoul(), e simple_strtoull() ignorano volutamente
> -i possibili overflow, e questo può portare il chiamante a generare 
> risultati
> -inaspettati. Le rispettive funzioni kstrtol(), kstrtoll(),
> +Le funzioni simple_strtol(), simple_strtoul(), e simple_strtoull() 
> ignorano
> +volutamente i possibili overflow, e questo può portare il chiamante a 
> generare
> +risultati inaspettati. Le rispettive funzioni kstrtol(), kstrtoll(),
>  kstrtoul(), e kstrtoull() sono da considerarsi le corrette
>  sostitute; tuttavia va notato che queste richiedono che la stringa sia
>  terminata con il carattere NUL o quello di nuova riga.

This is fine

-- 
Federico Vaga