proc: fix /proc/*/map_files lookup

PATCH Applied Alexey Dobriyan 3 participants 7 messages Nov 20, 2017 – Nov 29, 2017 Proc filesystem

lore

Share & tools

lore

Alexey Dobriyan

author

2017-11-20 21:27 UTC #0 lore raw

PATCH 1/1 proc: fix /proc/*/map_files lookup

Current code does:

	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)

However sscanf() is broken garbage.

It silently accepts whitespace between format specifiers (did you know that?).

It silently accepts valid strings which result in integer overflow.

Do not use sscanf() for any even remotely reliable parsing code.

	OK
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/               55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000    '
	/lib/systemd/systemd

	very broken
	# readlink '/proc/1/map_files/1000000000000000055a23af39000-55a23b05b000'
	/lib/systemd/systemd

Signed-off-by Alexey Dobriyan <[email protected]>

Cc [email protected]

---

 fs/proc/base.c |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

fs/proc/base.c +28 -1

@@ -100,6 +100,8 @@
 #include "internal.h"
 #include "fd.h"
 
+#include "../../lib/kstrtox.h"
+
 /* NOTE:
  *	Implementing inode permission operations in /proc is almost
  *	certainly an error.  Permission checks need to happen during

@@ -1907,8 +1909,33 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
 static int dname_to_vma_addr(struct dentry *dentry,
 			     unsigned long *start, unsigned long *end)
 {
-	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
+	const char *str = dentry->d_name.name;
+	unsigned long long sval, eval;
+	unsigned int len;
+
+	len = _parse_integer(str, 16, &sval);
+	if (len & KSTRTOX_OVERFLOW)
+		return -EINVAL;
+	if (sval != (unsigned long)sval)
+		return -EINVAL;
+	str += len;
+
+	if (*str != '-')
 		return -EINVAL;
+	str++;
+
+	len = _parse_integer(str, 16, &eval);
+	if (len & KSTRTOX_OVERFLOW)
+		return -EINVAL;
+	if (eval != (unsigned long)eval)
+		return -EINVAL;
+	str += len;
+
+	if (*str != '\0')
+		return -EINVAL;
+
+	*start = sval;
+	*end = eval;
 
 	return 0;
 }

Andrew Morton

2017-11-20 22:16 UTC #1 lore raw

Quoted 29 lines · On Tue, 21 Nov 2017 00:27:06 +0300 Alexey Dobriyan <[email protected]> wrote:

Current code does:
	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
However sscanf() is broken garbage.
It silently accepts whitespace between format specifiers (did you know that?).
It silently accepts valid strings which result in integer overflow.
Do not use sscanf() for any even remotely reliable parsing code.
	OK
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/               55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000    '
	/lib/systemd/systemd

	very broken
	# readlink '/proc/1/map_files/1000000000000000055a23af39000-55a23b05b000'
	/lib/systemd/systemd

Quoted 2 lines

Signed-off-by Alexey Dobriyan <[email protected]>
Cc [email protected]

OK, but why is this a problem worthy of backporting into -stable kernels?

Alexey Dobriyan

author

2017-11-21 05:48 UTC #2 lore raw

Quoted 10 lines · Andrew Morton

Quoted 6 lines · On Tue, 21 Nov 2017 00:27:06 +0300 Alexey Dobriyan <[email protected]> wrote:
	very broken
	# readlink '/proc/1/map_files/1000000000000000055a23af39000-55a23b05b000'
	/lib/systemd/systemd
Signed-off-by Alexey Dobriyan <[email protected]>
Cc [email protected]
OK, but why is this a problem worthy of backporting into -stable kernels?

Greg is making 4.14 LTS release. Maybe it should be fixed early than wait until someone will start depending on it.

Andrei Vagin

2017-11-28 05:29 UTC #3 lore raw

In reply to Alexey Dobriyan

Quoted 4 lines · Alexey Dobriyan

Current code does:

	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)

Quoted 8 lines

However sscanf() is broken garbage.
It silently accepts whitespace between format specifiers (did you know that?).
It silently accepts valid strings which result in integer overflow.
Do not use sscanf() for any even remotely reliable parsing code.

This patch breaks criu, criu has one places where a file name is generated as map_files/%p-%p

openat(1048572, "map_files/0x7f9912dd5000-0x7f9912de4000", O_RDWR) = -1 ENOENT (No such file or directory) <0.000015>

And this code worked before this patch and it doesn't work with this patch. And you have to know that we never break user-space programs ;)

But seriously, the patch looks good to me, but I would prefer to not queue it into stable kernels.

Thanks,
Andrei

Quoted 70 lines

	OK
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/               55a23af39000-55a23b05b000'
	/lib/systemd/systemd

	broken
	# readlink '/proc/1/map_files/55a23af39000-55a23b05b000    '
	/lib/systemd/systemd

	very broken
	# readlink '/proc/1/map_files/1000000000000000055a23af39000-55a23b05b000'
	/lib/systemd/systemd

Signed-off-by Alexey Dobriyan <[email protected]>

Cc [email protected]

---

 fs/proc/base.c |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

fs/proc/base.c +28 -1

@@ -100,6 +100,8 @@
 #include "internal.h"
 #include "fd.h"
 
+#include "../../lib/kstrtox.h"
+
 /* NOTE:
  *	Implementing inode permission operations in /proc is almost
  *	certainly an error.  Permission checks need to happen during

@@ -1907,8 +1909,33 @@ bool proc_fill_cache(struct file *file, struct dir_context *ctx,
 static int dname_to_vma_addr(struct dentry *dentry,
 			     unsigned long *start, unsigned long *end)
 {
-	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
+	const char *str = dentry->d_name.name;
+	unsigned long long sval, eval;
+	unsigned int len;
+
+	len = _parse_integer(str, 16, &sval);
+	if (len & KSTRTOX_OVERFLOW)
+		return -EINVAL;
+	if (sval != (unsigned long)sval)
+		return -EINVAL;
+	str += len;
+
+	if (*str != '-')
 		return -EINVAL;
+	str++;
+
+	len = _parse_integer(str, 16, &eval);
+	if (len & KSTRTOX_OVERFLOW)
+		return -EINVAL;
+	if (eval != (unsigned long)eval)
+		return -EINVAL;
+	str += len;
+
+	if (*str != '\0')
+		return -EINVAL;
+
+	*start = sval;
+	*end = eval;
 
 	return 0;
 }

Alexey Dobriyan

author

2017-11-28 10:12 UTC #4 lore raw

Quoted 10 lines · Andrei Vagin

Quoted 9 lines · Alexey Dobriyan
Current code does:
	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
However sscanf() is broken garbage.
It silently accepts whitespace between format specifiers (did you know that?).

Quoted 8 lines

Quoted 3 lines
It silently accepts valid strings which result in integer overflow.
Do not use sscanf() for any even remotely reliable parsing code.
This patch breaks criu, criu has one places where a file name is generated as map_files/%p-%p
openat(1048572, "map_files/0x7f9912dd5000-0x7f9912de4000", O_RDWR) = -1

glibc prints null pointer as "(nil)", so %p is broken even if lookup is fixed.

Quoted 2 lines

And this code worked before this patch and it doesn't work with this patch. And you have to know that we never break user-space programs ;)

OK :-)

Andrew Morton

2017-11-29 22:56 UTC #5 lore raw

In reply to Andrei Vagin

Quoted 17 lines · On Mon, 27 Nov 2017 21:29:25 -0800 Andrei Vagin <[email protected]> wrote:

Quoted 4 lines · Alexey Dobriyan
Current code does:
	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
Quoted 8 lines
However sscanf() is broken garbage.
It silently accepts whitespace between format specifiers (did you know that?).
It silently accepts valid strings which result in integer overflow.
Do not use sscanf() for any even remotely reliable parsing code.
This patch breaks criu, criu has one places where a file name is generated as map_files/%p-%p

Quoted 7 lines

openat(1048572, "map_files/0x7f9912dd5000-0x7f9912de4000", O_RDWR) = -1 ENOENT (No such file or directory) <0.000015>
And this code worked before this patch and it doesn't work with this patch. And you have to know that we never break user-space programs ;)
But seriously, the patch looks good to me, but I would prefer to not queue it into stable kernels.

The patch breaks CRIU but you're OK with merging it? How does that work ;)

Now I'm worried that it will break other things.

Andrei Vagin

2017-11-29 23:23 UTC #6 lore raw

Quoted 21 lines · Andrew Morton

Quoted 19 lines · On Mon, 27 Nov 2017 21:29:25 -0800 Andrei Vagin <[email protected]> wrote:
Quoted 4 lines · Alexey Dobriyan
Current code does:
	if (sscanf(dentry->d_name.name, "%lx-%lx", start, end) != 2)
Quoted 8 lines
However sscanf() is broken garbage.
It silently accepts whitespace between format specifiers (did you know that?).
It silently accepts valid strings which result in integer overflow.
Do not use sscanf() for any even remotely reliable parsing code.
This patch breaks criu, criu has one places where a file name is generated as map_files/%p-%p
openat(1048572, "map_files/0x7f9912dd5000-0x7f9912de4000", O_RDWR) = -1 ENOENT (No such file or directory) <0.000015>

Quoted 7 lines

Quoted 5 lines
And this code worked before this patch and it doesn't work with this patch. And you have to know that we never break user-space programs ;)
But seriously, the patch looks good to me, but I would prefer to not queue it into stable kernels.
The patch breaks CRIU but you're OK with merging it? How does that work ;)

It was a bug in criu. And this bug is on a minor path, which works when memfd_create() isn't available. It is a reason why I ask to not backport this patch to stable kernels.

In CRIU this bug can be triggered, only if this patch will be backported to a kernel which version is lower than v3.16.

Quoted 2 lines

Now I'm worried that it will break other things.

I think a chance is very small. All programs should use names which listed in /proc/PID/map_files/.