Re: [bug report] cachestat: implement cachestat syscall

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, May 05, 2023 at 09:27:49AM -0700, Nhat Pham wrote:
> On Fri, May 5, 2023 at 1:44 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Hello Nhat Pham,
> >
> > The patch 5c289a59b1d0: "cachestat: implement cachestat syscall" from
> > May 2, 2023, leads to the following Smatch static checker warning:
> >
> >         mm/filemap.c:4282 __do_sys_cachestat()
> >         warn: potential integer overflow from user (local copy) 'csr.off + csr.len'
> >
> > mm/filemap.c
> >   4250  SYSCALL_DEFINE4(cachestat, unsigned int, fd,
> >   4251                  struct cachestat_range __user *, cstat_range,
> >   4252                  struct cachestat __user *, cstat, unsigned int, flags)
> >   4253  {
> >   4254          struct fd f = fdget(fd);
> >   4255          struct address_space *mapping;
> >   4256          struct cachestat_range csr;
> >   4257          struct cachestat cs;
> >   4258          pgoff_t first_index, last_index;
> >   4259
> >   4260          if (!f.file)
> >   4261                  return -EBADF;
> >   4262
> >   4263          if (copy_from_user(&csr, cstat_range,
> >
> > csr comes from the user.
> >
> >   4264                          sizeof(struct cachestat_range))) {
> >   4265                  fdput(f);
> >   4266                  return -EFAULT;
> >   4267          }
> >   4268
> >   4269          /* hugetlbfs is not supported */
> >   4270          if (is_file_hugepages(f.file)) {
> >   4271                  fdput(f);
> >   4272                  return -EOPNOTSUPP;
> >   4273          }
> >   4274
> >   4275          if (flags != 0) {
> >   4276                  fdput(f);
> >   4277                  return -EINVAL;
> >   4278          }
> >   4279
> >   4280          first_index = csr.off >> PAGE_SHIFT;
> >   4281          last_index =
> >   4282                  csr.len == 0 ? ULONG_MAX : (csr.off + csr.len - 1) >> PAGE_SHIFT;
> >                                                     ^^^^^^^^^^^^^^^^^^^^^^
> > This can integer overflow.   Do we need some checking to ensure that
> > first_index < last_index?
> 
> If first_index < last_index, it won't crash. The folio walk won't do
> anything, so the user will just receive all-zeros stats. I think this
> is fine.
> 
> Is there anything I could do to make the checker happy? :)
> 

No.  I can't this release check because it's so often a situation like
this where the integer overflow is harmless.  Reading this code, I
was pretty sure that filemap_cachestat() would turn into a no-op as you
say however it seemed worth checking given that the code is new.

regards,
dan carpenter