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