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