[PATCH] proc: fix /proc/*/map_files lookup

[PATCH] proc: fix /proc/*/map_files lookup

[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?).

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 <adobriyan@gmail.com>
Cc: stable@kernel.org
---

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

--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -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;
 }

Re: [PATCH] proc: fix /proc/*/map_files lookup

[Andrew Morton]

On Tue, 21 Nov 2017 00:27:06 +0300 Alexey Dobriyan <adobriyan@gmail.com> 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
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: stable@kernel.org

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

Re: [PATCH] proc: fix /proc/*/map_files lookup

[Alexey Dobriyan]

On Mon, Nov 20, 2017 at 02:16:14PM -0800, Andrew Morton wrote:
> On Tue, 21 Nov 2017 00:27:06 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> > 	very broken
> > 	# readlink '/proc/1/map_files/1000000000000000055a23af39000-55a23b05b000'
> > 	/lib/systemd/systemd
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: stable@kernel.org
> 
> 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.

Re: proc: fix /proc/*/map_files lookup

[Andrei Vagin]

On Tue, Nov 21, 2017 at 12:27:06AM +0300, Alexey Dobriyan 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.

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


> 
> 	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 <adobriyan@gmail.com>
> Cc: stable@kernel.org
> ---
> 
>  fs/proc/base.c |   29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -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;
>  }

Re: proc: fix /proc/*/map_files lookup

[Alexey Dobriyan]

On 11/28/17, Andrei Vagin <avagin@virtuozzo.com> wrote:
> On Tue, Nov 21, 2017 at 12:27:06AM +0300, Alexey Dobriyan 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.
>
> 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.

> 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 :-)

Re: proc: fix /proc/*/map_files lookup

[Andrew Morton]

On Mon, 27 Nov 2017 21:29:25 -0800 Andrei Vagin <avagin@virtuozzo.com> wrote:

> On Tue, Nov 21, 2017 at 12:27:06AM +0300, Alexey Dobriyan 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.
> 
> 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.

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.

Re: proc: fix /proc/*/map_files lookup

[Andrei Vagin]

On Wed, Nov 29, 2017 at 02:56:03PM -0800, Andrew Morton wrote:
> On Mon, 27 Nov 2017 21:29:25 -0800 Andrei Vagin <avagin@virtuozzo.com> wrote:
> 
> > On Tue, Nov 21, 2017 at 12:27:06AM +0300, Alexey Dobriyan 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.
> > 
> > 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.
> 
> 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.

> 
> 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/.