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@xxxxxxxxxx> 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
