1. Patchew
  2. Xen
  3. View series

[PATCH] xen/privcmd: prevent integer overflow on 32 bit systems

Dan Carpenter posted 1 patch 1 year, 9 months ago
Failed in applying to current master (apply log)
drivers/xen/privcmd.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
Posted by Dan Carpenter 1 year, 9 months ago 
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))
+			return -EINVAL;
 		if (!access_ok(m.err, m.num * (sizeof(*m.err))))
 			return -EFAULT;
 		break;
-- 
2.35.1
Re: [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
Posted by kernel test robot 1 year, 9 months ago 
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
Re: [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
Posted by Oleksandr Tyshchenko 1 year, 9 months ago 
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
Re: [PATCH] xen/privcmd: prevent integer overflow on 32 bit systems
Posted by Dan Carpenter 1 year, 9 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.