* Re: [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
2022-07-15 8:20 [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems Dan Carpenter
@ 2022-07-15 8:56 ` Oleksandr Tyshchenko
2022-07-16 10:12 ` Dan Carpenter
2022-07-19 18:57 ` kernel test robot
1 sibling, 1 reply; 4+ messages in thread
From: Oleksandr Tyshchenko @ 2022-07-15 8:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stefano Stabellini, Andres Lagar-Cavilla, Konrad Rzeszutek Wilk,
David Vrabel, xen-devel@lists.xenproject.org,
kernel-janitors@vger.kernel.org, Juergen Gross
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;
--
Regards,
Oleksandr Tyshchenko
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
2022-07-15 8:20 [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems Dan Carpenter
2022-07-15 8:56 ` Oleksandr Tyshchenko
@ 2022-07-19 18:57 ` kernel test robot
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-07-19 18:57 UTC (permalink / raw)
To: Dan Carpenter, Juergen Gross
Cc: llvm, kbuild-all, Stefano Stabellini, Oleksandr Tyshchenko,
Andres Lagar-Cavilla, Konrad Rzeszutek Wilk, David Vrabel,
xen-devel, kernel-janitors
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 4+ messages in thread