| Message ID | YtEjVdG+pp9DGz++@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/privcmd: prevent integer overflow on 32 bit systems | expand |
On 15.07.22 11:20, Dan Carpenter wrote: Hello Dan > The "m.num * sizeof(*m.arr)" multiplication can have an integer overflow > on 32 bit systems. Probably no one really uses this software on 32 bit > systems, but it's still worth fixing the bug if only to make the static > checker happy. > > Fixes: ceb90fa0a800 ("xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/xen/privcmd.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index ad17166b0ef6..1e59b76c618e 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -456,6 +456,8 @@ static long privcmd_ioctl_mmap_batch( > if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch))) > return -EFAULT; > /* Returns per-frame error in m.arr. */ > + if (m.num > SIZE_MAX / sizeof(*m.arr)) > + return -EINVAL; > m.err = NULL; > if (!access_ok(m.arr, m.num * sizeof(*m.arr))) > return -EFAULT; > @@ -464,6 +466,8 @@ static long privcmd_ioctl_mmap_batch( > if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2))) > return -EFAULT; > /* Returns per-frame error code in m.err. */ > + if (m.num > SIZE_MAX / sizeof(*m.arr)) Looks like here we need to check against sizeof(*m.err) which is used in the multiplication below. > + return -EINVAL; > if (!access_ok(m.err, m.num * (sizeof(*m.err)))) > return -EFAULT; > break;
On Fri, Jul 15, 2022 at 08:56:30AM +0000, Oleksandr Tyshchenko wrote: > > On 15.07.22 11:20, Dan Carpenter wrote: > > > Hello Dan > > > The "m.num * sizeof(*m.arr)" multiplication can have an integer overflow > > on 32 bit systems. Probably no one really uses this software on 32 bit > > systems, but it's still worth fixing the bug if only to make the static > > checker happy. > > > > Fixes: ceb90fa0a800 ("xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/xen/privcmd.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > > index ad17166b0ef6..1e59b76c618e 100644 > > --- a/drivers/xen/privcmd.c > > +++ b/drivers/xen/privcmd.c > > @@ -456,6 +456,8 @@ static long privcmd_ioctl_mmap_batch( > > if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch))) > > return -EFAULT; > > /* Returns per-frame error in m.arr. */ > > + if (m.num > SIZE_MAX / sizeof(*m.arr)) > > + return -EINVAL; > > m.err = NULL; > > if (!access_ok(m.arr, m.num * sizeof(*m.arr))) > > return -EFAULT; > > @@ -464,6 +466,8 @@ static long privcmd_ioctl_mmap_batch( > > if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2))) > > return -EFAULT; > > /* Returns per-frame error code in m.err. */ > > + if (m.num > SIZE_MAX / sizeof(*m.arr)) > > Looks like here we need to check against sizeof(*m.err) which is used in > the multiplication below. Oh, yeah. Sorry! Need to redo that. regards, dan carpenter
Hi Dan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on xen-tip/linux-next]
[also build test WARNING on linus/master v5.19-rc7 next-20220719]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/xen-privcmd-prevent-integer-overflow-on-32-bit-systems/20220715-162307
base: https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220720/202207200236.GeswjpCk-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fa0c7639e91fa1cd0cf2ff0445a1634a90fe850a)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/ea22ebd83753c7181043e69251b78f0be73675ad
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Dan-Carpenter/xen-privcmd-prevent-integer-overflow-on-32-bit-systems/20220715-162307
git checkout ea22ebd83753c7181043e69251b78f0be73675ad
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/acpi/ drivers/ata/ drivers/rtc/ drivers/thermal/intel/ drivers/xen/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/xen/privcmd.c:459:13: warning: result of comparison of constant 2305843009213693951 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
if (m.num > SIZE_MAX / sizeof(*m.arr))
~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/xen/privcmd.c:469:13: warning: result of comparison of constant 2305843009213693951 with expression of type 'unsigned int' is always false [-Wtautological-constant-out-of-range-compare]
if (m.num > SIZE_MAX / sizeof(*m.arr))
~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +459 drivers/xen/privcmd.c
441
442 static long privcmd_ioctl_mmap_batch(
443 struct file *file, void __user *udata, int version)
444 {
445 struct privcmd_data *data = file->private_data;
446 int ret;
447 struct privcmd_mmapbatch_v2 m;
448 struct mm_struct *mm = current->mm;
449 struct vm_area_struct *vma;
450 unsigned long nr_pages;
451 LIST_HEAD(pagelist);
452 struct mmap_batch_state state;
453
454 switch (version) {
455 case 1:
456 if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
457 return -EFAULT;
458 /* Returns per-frame error in m.arr. */
> 459 if (m.num > SIZE_MAX / sizeof(*m.arr))
460 return -EINVAL;
461 m.err = NULL;
462 if (!access_ok(m.arr, m.num * sizeof(*m.arr)))
463 return -EFAULT;
464 break;
465 case 2:
466 if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
467 return -EFAULT;
468 /* Returns per-frame error code in m.err. */
469 if (m.num > SIZE_MAX / sizeof(*m.arr))
470 return -EINVAL;
471 if (!access_ok(m.err, m.num * (sizeof(*m.err))))
472 return -EFAULT;
473 break;
474 default:
475 return -EINVAL;
476 }
477
478 /* If restriction is in place, check the domid matches */
479 if (data->domid != DOMID_INVALID && data->domid != m.dom)
480 return -EPERM;
481
482 nr_pages = DIV_ROUND_UP(m.num, XEN_PFN_PER_PAGE);
483 if ((m.num <= 0) || (nr_pages > (LONG_MAX >> PAGE_SHIFT)))
484 return -EINVAL;
485
486 ret = gather_array(&pagelist, m.num, sizeof(xen_pfn_t), m.arr);
487
488 if (ret)
489 goto out;
490 if (list_empty(&pagelist)) {
491 ret = -EINVAL;
492 goto out;
493 }
494
495 if (version == 2) {
496 /* Zero error array now to only copy back actual errors. */
497 if (clear_user(m.err, sizeof(int) * m.num)) {
498 ret = -EFAULT;
499 goto out;
500 }
501 }
502
503 mmap_write_lock(mm);
504
505 vma = find_vma(mm, m.addr);
506 if (!vma ||
507 vma->vm_ops != &privcmd_vm_ops) {
508 ret = -EINVAL;
509 goto out_unlock;
510 }
511
512 /*
513 * Caller must either:
514 *
515 * Map the whole VMA range, which will also allocate all the
516 * pages required for the auto_translated_physmap case.
517 *
518 * Or
519 *
520 * Map unmapped holes left from a previous map attempt (e.g.,
521 * because those foreign frames were previously paged out).
522 */
523 if (vma->vm_private_data == NULL) {
524 if (m.addr != vma->vm_start ||
525 m.addr + (nr_pages << PAGE_SHIFT) != vma->vm_end) {
526 ret = -EINVAL;
527 goto out_unlock;
528 }
529 if (xen_feature(XENFEAT_auto_translated_physmap)) {
530 ret = alloc_empty_pages(vma, nr_pages);
531 if (ret < 0)
532 goto out_unlock;
533 } else
534 vma->vm_private_data = PRIV_VMA_LOCKED;
535 } else {
536 if (m.addr < vma->vm_start ||
537 m.addr + (nr_pages << PAGE_SHIFT) > vma->vm_end) {
538 ret = -EINVAL;
539 goto out_unlock;
540 }
541 if (privcmd_vma_range_is_mapped(vma, m.addr, nr_pages)) {
542 ret = -EINVAL;
543 goto out_unlock;
544 }
545 }
546
547 state.domain = m.dom;
548 state.vma = vma;
549 state.va = m.addr;
550 state.index = 0;
551 state.global_error = 0;
552 state.version = version;
553
554 BUILD_BUG_ON(((PAGE_SIZE / sizeof(xen_pfn_t)) % XEN_PFN_PER_PAGE) != 0);
555 /* mmap_batch_fn guarantees ret == 0 */
556 BUG_ON(traverse_pages_block(m.num, sizeof(xen_pfn_t),
557 &pagelist, mmap_batch_fn, &state));
558
559 mmap_write_unlock(mm);
560
561 if (state.global_error) {
562 /* Write back errors in second pass. */
563 state.user_gfn = (xen_pfn_t *)m.arr;
564 state.user_err = m.err;
565 ret = traverse_pages_block(m.num, sizeof(xen_pfn_t),
566 &pagelist, mmap_return_errors, &state);
567 } else
568 ret = 0;
569
570 /* If we have not had any EFAULT-like global errors then set the global
571 * error to -ENOENT if necessary. */
572 if ((ret == 0) && (state.global_error == -ENOENT))
573 ret = -ENOENT;
574
575 out:
576 free_page_list(&pagelist);
577 return ret;
578
579 out_unlock:
580 mmap_write_unlock(mm);
581 goto out;
582 }
583
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index ad17166b0ef6..1e59b76c618e 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -456,6 +456,8 @@ static long privcmd_ioctl_mmap_batch( if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch))) return -EFAULT; /* Returns per-frame error in m.arr. */ + if (m.num > SIZE_MAX / sizeof(*m.arr)) + return -EINVAL; m.err = NULL; if (!access_ok(m.arr, m.num * sizeof(*m.arr))) return -EFAULT; @@ -464,6 +466,8 @@ static long privcmd_ioctl_mmap_batch( if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2))) return -EFAULT; /* Returns per-frame error code in m.err. */ + if (m.num > SIZE_MAX / sizeof(*m.arr)) + return -EINVAL; if (!access_ok(m.err, m.num * (sizeof(*m.err)))) return -EFAULT; break;
The "m.num * sizeof(*m.arr)" multiplication can have an integer overflow on 32 bit systems. Probably no one really uses this software on 32 bit systems, but it's still worth fixing the bug if only to make the static checker happy. Fixes: ceb90fa0a800 ("xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/xen/privcmd.c | 4 ++++ 1 file changed, 4 insertions(+)