[PATCH v2 1/2] userns: reject uid_map/gid_map values that do not fit in u32

[Natasha Klaus <natalie.klaus@runtimeverification.com>]

map_write() parses each line written to uid_map, gid_map or projid_map
with simple_strtoul() and stores the results in the u32 fields of
struct uid_gid_extent. On 64-bit kernels a value above U32_MAX is
silently truncated modulo 2^32. The coding assistant flagged this
truncation site while reading map_write(), and I confirmed it by hand
on a running kernel, where writing to the uid_map of a fresh user
namespace installs a different mapping than the one requested:

  "4294967296 1000 1"  ->  installed as "0 1000 1"
  "4294967297 1000 1"  ->  installed as "1 1000 1"
  "4294967301 1000 1"  ->  installed as "5 1000 1"

The same happens to the lower_first and count fields. Only a literal of
2^64 or more is rejected, and only because simple_strtoul() saturates
to ULLONG_MAX, which truncates onto the (u32)-1 value map_write()
already refuses. On 32-bit kernels unsigned long is itself 32 bits
wide, so the same truncation happens inside simple_strtoul().

The write succeeds, so a runtime that computes ranges and gets one
wrong ends up with a different map than it asked for and no error to
tell it so.

Parse each field with kstrtou32() instead. A new helper,
map_parse_u32(), terminates the field at the next whitespace or NUL,
parses it and puts the original character back, leaving pos where
simple_strtoul() used to leave it. The checks after each field, the
trailing junk check and every later check are unchanged and run in the
same order. A field that is not a decimal number fitting in u32 now
fails the write with -EINVAL, like any other malformed line.

Besides the range check, the one visible difference is that kstrtou32()
accepts a single leading '+', so "+5 1000 1" is now installed as
"5 1000 1" where it used to be rejected.

This makes the parser more correct; it is not a security fix: the
truncated values are valid ids and every later check still applies to
them.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Natasha Klaus <natalie.klaus@runtimeverification.com>
Assisted-by: LLM
---
kbuf is map_write()'s private copy of the user buffer from
memdup_user_nul(): it is NUL-terminated, nothing else refers to it, and
map_write() already overwrites each '\n' in it with '\0' and frees it
on return. So map_parse_u32() can write a '\0' after each field for
kstrtou32() and put the original character back.

 kernel/user_namespace.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 0bed462e9b2a..1e232f78cfd6 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -929,6 +929,26 @@ static bool verify_root_map(const struct file *file,
 	return true;
 }
 
+/*
+ * Parse the decimal u32 at *pos, up to the next whitespace or NUL, and
+ * leave *pos pointing at that character.
+ */
+static int map_parse_u32(char **pos, u32 *res)
+{
+	char *end = *pos;
+	char c;
+	int ret;
+
+	while (*end && !isspace(*end))
+		end++;
+	c = *end;
+	*end = '\0';
+	ret = kstrtou32(*pos, 10, res);
+	*end = c;
+	*pos = end;
+	return ret;
+}
+
 static ssize_t map_write(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos,
 			 int cap_setid,
@@ -1001,17 +1021,20 @@ static ssize_t map_write(struct file *file, const char __user *buf,
 		}
 
 		pos = skip_spaces(pos);
-		extent.first = simple_strtoul(pos, &pos, 10);
+		if (map_parse_u32(&pos, &extent.first))
+			goto out;
 		if (!isspace(*pos))
 			goto out;
 
 		pos = skip_spaces(pos);
-		extent.lower_first = simple_strtoul(pos, &pos, 10);
+		if (map_parse_u32(&pos, &extent.lower_first))
+			goto out;
 		if (!isspace(*pos))
 			goto out;
 
 		pos = skip_spaces(pos);
-		extent.count = simple_strtoul(pos, &pos, 10);
+		if (map_parse_u32(&pos, &extent.count))
+			goto out;
 		if (*pos && !isspace(*pos))
 			goto out;
 
-- 
2.34.1