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 <[email protected]>
> Cc: [email protected]
> ---
>
> 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;
> }
From 1584653559919789070@xxx Tue Nov 21 05:50:00 +0000 2017
X-GM-THRID: 1584621974020232218
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread