Make sscanf() stricter

PATCH In review Demi Marie Obenour 6 participants 13 messages Jun 10, 2023 – Jun 13, 2023

Subsystems (9)

Media (V4L/DVB) Staging Library code Multifunction devices (MFD) +5 more subsystems

lore

Share & tools

lore

Demi Marie Obenour

author

2023-06-10 20:40 UTC #0 lore raw

v3 cover Make sscanf() stricter

Roger Pau Monné suggested making xenbus_scanf() stricter instead of using a custom parser. Christoph Hellwig asked why the normal vsscanf() cannot be made stricter. Richard Weinberger mentioned Linus Torvalds’s suggestion of using ! to allow overflow.

Changes since v2:

- Better commit messages.
- Fix a compile error in simple_strtoll() (found by 0day bot).
- Fix an uninitialized variable (found by Dan Carpenter).

Changes since v1:

- Better commit messages.
- Use ! to explicitly opt-in to allowing overflow.
- Treat overflow as a conversion failure instead of returning ERANGE.
- Drop the first patch (removal of simple_strtoll()) as it breaks bcache.
- Stop skipping spaces in vsscanf() instead of adding a separate vsscanf_strict() function.

Demi Marie Obenour (4):
limits.h: add UCHAR_MAX, SCHAR_MAX, and SCHAR_MIN vsscanf(): Integer overflow is a conversion failure vsscanf(): do not skip spaces
Reject NUL bytes in xenstore nodes

 .../hive_isp_css_include/platform_support.h   |  1 -
 drivers/xen/xenbus/xenbus_xs.c                | 17 +++-
 include/linux/limits.h                        |  1 +
 include/linux/mfd/wl1273-core.h               |  3 -
 include/vdso/limits.h                         |  3 +
 lib/vsprintf.c                                | 98 +++++++++++++------
 6 files changed, 86 insertions(+), 37 deletions(-)

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Demi Marie Obenour

author

2023-06-10 20:40 UTC #1 lore raw

PATCH v3 1/4 limits.h: add UCHAR_MAX, SCHAR_MAX, and SCHAR_MIN

Some drivers already defined these, and they will be used by sscanf() for overflow checks later. Also add SSIZE_MIN to limits.h, which will also be needed later.

Signed-off-by Demi Marie Obenour <[email protected]>

---
 .../media/atomisp/pci/hive_isp_css_include/platform_support.h  | 1 -
 include/linux/limits.h                                         | 1 +
 include/linux/mfd/wl1273-core.h                                | 3 ---
 include/vdso/limits.h                                          | 3 +++
 4 files changed, 4 insertions(+), 4 deletions(-)

drivers/staging/media/atomisp/pci/hive_isp_css_include/platform_support.h +0 -1

index 0cdef4a5e8b1bed9884133f1a0b9d853d59d43a4..e29b96d8bebf14839f6dd48fdc6c0f8b029ef31d 100644
@@ -27,7 +27,6 @@
 
 #define UINT16_MAX USHRT_MAX
 #define UINT32_MAX UINT_MAX
-#define UCHAR_MAX  (255)
 
 #define CSS_ALIGN(d, a) d __attribute__((aligned(a)))

include/linux/limits.h +1 -0

index f6bcc936901071f496e3e85bb6e1d93905b12e32..8f7fd85b41fb46e6992d9e5912da00424119227a 100644
@@ -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)

include/linux/mfd/wl1273-core.h +0 -3

index c28cf76d5c31ee1c94a9319a2e2d318bf00283a6..b81a229135ed9f756c749122a8341816031c8311 100644
@@ -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)

include/vdso/limits.h +3 -0

index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
@@ -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))

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Lee Jones

maintainer Acked-by on v3 1/4

2023-06-12 11:20 UTC #2 lore raw

Re: [PATCH v3 1/4] limits.h: add UCHAR_MAX, SCHAR_MAX, and SCHAR_MIN

Quoted 9 lines · Demi Marie Obenour

Some drivers already defined these, and they will be used by sscanf() for overflow checks later. Also add SSIZE_MIN to limits.h, which will also be needed later.
Signed-off-by Demi Marie Obenour <[email protected]>
---
 .../media/atomisp/pci/hive_isp_css_include/platform_support.h  | 1 -
 include/linux/limits.h                                         | 1 +
 include/linux/mfd/wl1273-core.h                                | 3 ---

Acked-by Lee Jones <[email protected]>

Quoted 60 lines

 include/vdso/limits.h                                          | 3 +++
 4 files changed, 4 insertions(+), 4 deletions(-)

drivers/staging/media/atomisp/pci/hive_isp_css_include/platform_support.h +0 -1

index 0cdef4a5e8b1bed9884133f1a0b9d853d59d43a4..e29b96d8bebf14839f6dd48fdc6c0f8b029ef31d 100644
@@ -27,7 +27,6 @@
 
 #define UINT16_MAX USHRT_MAX
 #define UINT32_MAX UINT_MAX
-#define UCHAR_MAX  (255)
 
 #define CSS_ALIGN(d, a) d __attribute__((aligned(a)))

include/linux/limits.h +1 -0

index f6bcc936901071f496e3e85bb6e1d93905b12e32..8f7fd85b41fb46e6992d9e5912da00424119227a 100644
@@ -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)

include/linux/mfd/wl1273-core.h +0 -3

index c28cf76d5c31ee1c94a9319a2e2d318bf00283a6..b81a229135ed9f756c749122a8341816031c8311 100644
@@ -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)

include/vdso/limits.h +3 -0

index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
@@ -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))

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Signature · 1 line

Lee Jones [李琼斯]

Vincenzo Frascino

maintainer

2023-06-12 16:32 UTC #3 lore raw

Re: [PATCH v3 1/4] limits.h: add UCHAR_MAX, SCHAR_MAX, and SCHAR_MIN

In reply to Demi Marie Obenour

Hi Demi,

Quoted 12 lines · Demi Marie Obenour

Some drivers already defined these, and they will be used by sscanf() for overflow checks later. Also add SSIZE_MIN to limits.h, which will also be needed later.
Signed-off-by Demi Marie Obenour <[email protected]>
---
 .../media/atomisp/pci/hive_isp_css_include/platform_support.h  | 1 -
 include/linux/limits.h                                         | 1 +
 include/linux/mfd/wl1273-core.h                                | 3 ---
 include/vdso/limits.h                                          | 3 +++
 4 files changed, 4 insertions(+), 4 deletions(-)

...

include/vdso/limits.h line 7

Quoted 11 lines

include/vdso/limits.h +3 -0

index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
@@ -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))

Are you planning to use those definitions in the vDSO library?

If not can you please define them in linux/limits.h, the vdso headers contain only what is necessary for the vDSO library.

Thanks!

Quoted 3 lines

include/vdso/limits.h +0 -0

 #define USHRT_MAX	((unsigned short)~0U)
 #define SHRT_MAX	((short)(USHRT_MAX >> 1))
 #define SHRT_MIN	((short)(-SHRT_MAX - 1))

Signature · 2 lines

Regards,
Vincenzo

Demi Marie Obenour

author

2023-06-12 20:19 UTC #4 lore raw

Re: [PATCH v3 1/4] limits.h: add UCHAR_MAX, SCHAR_MAX, and SCHAR_MIN

Quoted 25 lines · Vincenzo Frascino

Hi Demi,
Quoted 12 lines · Demi Marie Obenour
Some drivers already defined these, and they will be used by sscanf() for overflow checks later. Also add SSIZE_MIN to limits.h, which will also be needed later.
Signed-off-by Demi Marie Obenour <[email protected]>
---
 .../media/atomisp/pci/hive_isp_css_include/platform_support.h  | 1 -
 include/linux/limits.h                                         | 1 +
 include/linux/mfd/wl1273-core.h                                | 3 ---
 include/vdso/limits.h                                          | 3 +++
 4 files changed, 4 insertions(+), 4 deletions(-)
...
Quoted 8 lines
include/vdso/limits.h +0 -0
index 0197888ad0e00b2f853d3f25ffa764f61cca7385..0cad0a2490e5efc194d874025eb3e3b846a5c7b4 100644
@@ -2,6 +2,9 @@
 #ifndef __VDSO_LIMITS_H
 #define __VDSO_LIMITS_H
 

Quoted 5 lines

include/vdso/limits.h line 7
Quoted 3 lines
include/vdso/limits.h +3 -0
+#define UCHAR_MAX	((unsigned char)~0U)
+#define SCHAR_MAX	((signed char)(UCHAR_MAX >> 1))
+#define SCHAR_MIN	((signed char)(-SCHAR_MAX - 1))
Are you planning to use those definitions in the vDSO library?

Nope. They were added here for consistency with the other *_{MIN,MAX} defines.

Quoted 2 lines

If not can you please define them in linux/limits.h, the vdso headers contain only what is necessary for the vDSO library.

Will fix in the next version.

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Demi Marie Obenour

author

2023-06-10 20:41 UTC #5 lore raw

PATCH v3 2/4 vsscanf(): Integer overflow is a conversion failure

In reply to Demi Marie Obenour

sscanf() and friends currently ignore integer overflow, but this is a bad idea. It is much better to detect integer overflow errors and consider this a conversion failure.

This implements Linus's suggestion of using '!' to treat integer overflow as wrapping. It does _not_ allow wrapping of unsigned conversions by default, though, as in at least some cases accepting negative numbers is _not_ intended.

Signed-off-by Demi Marie Obenour <[email protected]>

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

lib/vsprintf.c +69 ~~-21~~

index 40f560959b169b4c4ac6154d658cfe76cfd0c5a6..9e53355c35b1d6260631868228ede1d178fe3325 100644
@@ -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;

@@ -70,11 +70,13 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m
 	prefix_chars = cp - startp;
 	if (prefix_chars < max_chars) {
 		rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars);
-		/* FIXME */
+		*overflow = !!(rv & KSTRTOX_OVERFLOW);
 		cp += (rv & ~KSTRTOX_OVERFLOW);
 	} else {
 		/* Field too short for prefix + digit, skip over without converting */
 		cp = startp + max_chars;
+		/* Treat this as a conversion failure */
+		*overflow = true;
 	}
 
 	if (endp)

@@ -94,7 +96,9 @@ 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);
+	bool _placeholder;
+
+	return simple_strntoull(cp, INT_MAX, endp, base, &_placeholder);
 }
 EXPORT_SYMBOL(simple_strtoull);

@@ -130,18 +134,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;
 }
 
 /**

@@ -154,7 +162,9 @@ static long long simple_strntoll(const char *cp, size_t max_chars, char **endp,
  */
 long long simple_strtoll(const char *cp, char **endp, unsigned int base)
 {
-	return simple_strntoll(cp, INT_MAX, endp, base);
+	bool _placeholder;
+
+	return simple_strntoll(cp, INT_MAX, endp, base, &_placeholder);
 }
 EXPORT_SYMBOL(simple_strtoll);

@@ -3441,7 +3451,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, allow_overflow;
 
 	while (*fmt) {
 		/* skip any white space in format */

@@ -3464,6 +3474,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			break;
 		++fmt;
 
+		allow_overflow = *fmt == '!';
+		fmt += (int)allow_overflow;
+
 		/* skip this conversion.
 		 * advance both strings to next white space
 		 */

@@ -3649,45 +3662,80 @@ 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) && !allow_overflow)
+			break;
 
 		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) &&
+				    !allow_overflow)
+					return num;
 				*va_arg(args, signed char *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > UCHAR_MAX) && !allow_overflow)
+					return num;
 				*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) &&
+				    !allow_overflow)
+					return num;
 				*va_arg(args, short *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > USHRT_MAX) && !allow_overflow)
+					return num;
 				*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) &&
+				    !allow_overflow)
+					return num;
 				*va_arg(args, long *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > ULONG_MAX) && !allow_overflow)
+					return num;
 				*va_arg(args, unsigned long *) = val.u;
+			}
 			break;
 		case 'L':
+			/* No overflow check needed */
 			if (is_sign)
 				*va_arg(args, long long *) = val.s;
 			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 num;
+				*va_arg(args, ssize_t *) = val.s;
+			} else {
+				if (unlikely(val.u > SIZE_MAX) && !allow_overflow)
+					return num;
+				*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) &&
+				    !allow_overflow)
+					return num;
 				*va_arg(args, int *) = val.s;
-			else
+			} else {
+				if (unlikely(val.u > UINT_MAX) && !allow_overflow)
+					return num;
 				*va_arg(args, unsigned int *) = val.u;
+			}
 			break;
 		}
 		num++;

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Rasmus Villemoes

reviewer

2023-06-12 10:53 UTC #6 lore raw

Re: [PATCH v3 2/4] vsscanf(): Integer overflow is a conversion failure

Quoted 3 lines · Demi Marie Obenour

sscanf() and friends currently ignore integer overflow, but this is a bad idea. It is much better to detect integer overflow errors and consider this a conversion failure.

Perhaps. And maybe I even agree. But not like this:

lib/vsprintf.c int vsscanf(const char *buf, const char *fmt, va_list args) · line 3479

Quoted 9 lines

lib/vsprintf.c +3 -0

 	while (*fmt) {
 		/* skip any white space in format */

@@ -3464,6 +3474,9 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			break;
 		++fmt;
 
+		allow_overflow = *fmt == '!';
+		fmt += (int)allow_overflow;
+

You can't do that. Or, at least, you won't be able to actually use %!d anywhere, because the compiler will yell at you:

lib/vsprintf.c: In function ‘foobar’:
lib/vsprintf.c:3727:26: error: unknown conversion type character ‘!’ in format [-Werror=format=]
3727 | ret = sscanf("12345", "%!d", &val);
| ^

So NAK.

Also, when you make significant changes to the sscanf implementation, I'd expect the diffstat for the patch series to contain lib/test_scanf.c.

Rasmus

Demi Marie Obenour

author

2023-06-10 20:41 UTC #7 lore raw

PATCH v3 3/4 vsscanf(): do not skip spaces

In reply to Demi Marie Obenour

Passing spaces before e.g. an integer is usually
not intended. This was suggested by Christoph in
https://lore.kernel.org/lkml/[email protected]/.

Signed-off-by Demi Marie Obenour <[email protected]>

---
 lib/vsprintf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

lib/vsprintf.c +1 -7

index 9e53355c35b1d6260631868228ede1d178fe3325..665f6197f8313d653f67d7886b12c43942e058dd 100644
@@ -3551,8 +3551,6 @@ 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);
 
 			/* now copy until next white space */
 			while (*str && !isspace(*str) && field_width--)

@@ -3639,11 +3637,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			return num;
 		}
 
-		/* have some sort of integer conversion.
-		 * first, skip white space in buffer.
-		 */
-		str = skip_spaces(str);
-
+		/* have some sort of integer conversion. */
 		digit = *str;
 		if (is_sign && digit == '-') {
 			if (field_width == 1)

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Rasmus Villemoes

reviewer

2023-06-12 11:08 UTC #8 lore raw

Re: [PATCH v3 3/4] vsscanf(): do not skip spaces

Quoted 2 lines · Demi Marie Obenour

Passing spaces before e.g. an integer is usually
not intended.

Maybe, maybe not. But it's mandated by POSIX/C99.

And of course we are free to ignore that and implement our own semantics (though within the constraints that we really want -Wformat to help us), but there seems to be existing code in-tree that relies on this behavior. For example I think this will break fsl_sata_intr_coalescing_store() which uses a scanf format of "%u%u".

Sure, that could just say "%u %u" instead, but the point is that currently it doesn't. So without some reasonably thorough analysis across the tree, and updates of affected callers, NAK.

Rasmus

David Laight

2023-06-13 12:43 UTC #9 lore raw

RE: [PATCH v3 3/4] vsscanf(): do not skip spaces

From: Rasmus Villemoes

Quoted 14 lines

Sent: 12 June 2023 12:08
Quoted 2 lines · Demi Marie Obenour
Passing spaces before e.g. an integer is usually
not intended.
Maybe, maybe not. But it's mandated by POSIX/C99.
And of course we are free to ignore that and implement our own semantics (though within the constraints that we really want -Wformat to help us), but there seems to be existing code in-tree that relies on this behavior. For example I think this will break fsl_sata_intr_coalescing_store() which uses a scanf format of "%u%u".

Quoted 3 lines

Sure, that could just say "%u %u" instead, but the point is that currently it doesn't. So without some reasonably thorough analysis across the tree, and updates of affected callers, NAK.

It would almost certainly need to be " %u %u" to allow for userspace generating the input with "%6u %6u",

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

David Laight

2023-06-12 11:11 UTC #10 lore raw

RE: [PATCH v3 3/4] vsscanf(): do not skip spaces

In reply to Demi Marie Obenour

From: Demi Marie Obenour

Quoted 5 lines

Sent: 10 June 2023 21:41
Passing spaces before e.g. an integer is usually
not intended. This was suggested by Christoph in
https://lore.kernel.org/lkml/[email protected]/.

This is contrary to libc scanf and could easily affect userspace writing fixed width values into sysctl nodes (etc).

IIRC strtoul() (etc) are also expected to strip leading spaces.
Removing the sign in sscanf() may lead to "- 12" being
valid - which may not be desired.

David

Quoted 34 lines

Signed-off-by Demi Marie Obenour <[email protected]>

---
 lib/vsprintf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

lib/vsprintf.c +1 -8

index 9e53355c35b1d6260631868228ede1d178fe3325..665f6197f8313d653f67d7886b12c43942e058dd 100644
@@ -3551,8 +3551,6 @@ 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);
 
 			/* now copy until next white space */
 			while (*str && !isspace(*str) && field_width--)

@@ -3639,11 +3637,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			return num;
 		}
 
-		/* have some sort of integer conversion.
-		 * first, skip white space in buffer.
-		 */
-		str = skip_spaces(str);
-
+		/* have some sort of integer conversion. */
 		digit = *str;
 		if (is_sign && digit == '-') {
 			if (field_width == 1)
--

Quoted 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Demi Marie Obenour

author

2023-06-10 20:41 UTC #11 lore raw

PATCH v3 4/4 Reject NUL bytes in xenstore nodes

In reply to Demi Marie Obenour

This rejects bogus xenstore node values that include interior NUL bytes. These would be truncated by functions that expect NUL-terminated strings.

Signed-off-by Demi Marie Obenour <[email protected]>

---
 drivers/xen/xenbus/xenbus_xs.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/xen/xenbus/xenbus_xs.c +12 -5

index 12e02eb01f5991b31db451cc57037205359b347f..7cb2a22a7698ac40c81add23476594d9f27de8d0 100644
@@ -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);
 	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(p, fmt, result) <= 0)
 				ret = -EINVAL;
 			kfree(p);
 		} else

Signature · 3 lines

Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Andy Shevchenko

reviewer

2023-06-12 15:34 UTC #12 lore raw

In reply to Demi Marie Obenour

Quoted 4 lines · Demi Marie Obenour

Roger Pau Monné suggested making xenbus_scanf() stricter instead of using a custom parser. Christoph Hellwig asked why the normal vsscanf() cannot be made stricter. Richard Weinberger mentioned Linus Torvalds’s suggestion of using ! to allow overflow.

As Rasmus articulated, NAK w.o. test cases being added to all parts where your changes touch.

Signature · 2 lines

With Best Regards,
Andy Shevchenko