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; > }
