* [bug report] cachestat: implement cachestat syscall
@ 2023-05-05 8:44 Dan Carpenter
2023-05-05 16:27 ` Nhat Pham
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2023-05-05 8:44 UTC (permalink / raw)
To: nphamcs; +Cc: linux-mm
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?
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] cachestat: implement cachestat syscall
2023-05-05 8:44 [bug report] cachestat: implement cachestat syscall Dan Carpenter
@ 2023-05-05 16:27 ` Nhat Pham
2023-05-05 20:05 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Nhat Pham @ 2023-05-05 16:27 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] cachestat: implement cachestat syscall
2023-05-05 16:27 ` Nhat Pham
@ 2023-05-05 20:05 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-05-05 20:05 UTC (permalink / raw)
To: Nhat Pham; +Cc: linux-mm
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-05 20:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 8:44 [bug report] cachestat: implement cachestat syscall Dan Carpenter
2023-05-05 16:27 ` Nhat Pham
2023-05-05 20:05 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.