Re: [bug report] cachestat: implement cachestat syscall

[Nhat Pham <nphamcs@gmail.com>]

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

>
>   4283          memset(&cs, 0, sizeof(struct cachestat));
>   4284          mapping = f.file->f_mapping;
>   4285          filemap_cachestat(mapping, first_index, last_index, &cs);
>   4286          fdput(f);
>   4287
>   4288          if (copy_to_user(cstat, &cs, sizeof(struct cachestat)))
>   4289                  return -EFAULT;
>   4290
>   4291          return 0;
>   4292  }
>
> regards,
> dan carpenter