[PATCH 0/1] fix regression in hugetlbfs overflow checking

[PATCH 0/1] fix regression in hugetlbfs overflow checking

[Mike Kravetz]

Commit 63489f8e8211 ("hugetlbfs: check for pgoff value overflow")
introduced a regression in 32 bit kernels.  When creating the mask
to check vm_pgoff, it incorrectly specified that the size of a loff_t
was the size of a long.  This prevents mapping hugetlbfs files at
offsets greater than 4GB on 32 bit kernels.

The above is in the commit message.  63489f8e8211 has been sent upstream
and to stable, so cc'ing stable here as well.

I would appreciate some more eyes on this code.  There have been several
fixes and we keep running into issues.

Mike Kravetz (1):
  hugetlbfs: fix bug in pgoff overflow checking

 fs/hugetlbfs/inode.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

-- 
2.13.6

[PATCH 1/1] hugetlbfs: fix bug in pgoff overflow checking

[Mike Kravetz]

This is a fix for a regression in 32 bit kernels caused by an
invalid check for pgoff overflow in hugetlbfs mmap setup.  The
check incorrectly specified that the size of a loff_t was the
same as the size of a long.  The regression prevents mapping
hugetlbfs files at offset greater than 4GB on 32 bit kernels.

Fix the check by using sizeof(loff_t) to get size.  In addition,
make sure pgoff + length can be represented by a signed long
huge page offset.  This check is only necessary on 32 bit kernels.

Fixes: 63489f8e8211 ("hugetlbfs: check for pgoff value overflow")
Cc: <stable@vger.kernel.org>
Reported-by: Dan Rue <dan.rue@linaro.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b9a254dcc0e7..8450a1d75dfa 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -116,7 +116,8 @@ static void huge_pagevec_release(struct pagevec *pvec)
  * bit into account.
  */
 #define PGOFF_LOFFT_MAX \
-	(((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT + 1)))
+	(((1UL << (PAGE_SHIFT + 1)) - 1) << \
+	 ((sizeof(loff_t) * BITS_PER_BYTE) - (PAGE_SHIFT + 1)))
 
 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
@@ -138,21 +139,32 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 
 	/*
 	 * page based offset in vm_pgoff could be sufficiently large to
-	 * overflow a (l)off_t when converted to byte offset.
+	 * overflow a loff_t when converted to byte offset.
 	 */
-	if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
+	if ((loff_t)vma->vm_pgoff & (loff_t)PGOFF_LOFFT_MAX)
 		return -EINVAL;
 
-	/* must be huge page aligned */
+	/* vm_pgoff must be huge page aligned */
 	if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
 		return -EINVAL;
 
+	/*
+	 * Compute file offset of the end of this mapping
+	 */
 	vma_len = (loff_t)(vma->vm_end - vma->vm_start);
 	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
-	/* check for overflow */
+
+	/* Check to ensure this did not overflow loff_t */
 	if (len < vma_len)
 		return -EINVAL;
 
+	/*
+	 * On 32 bit systems, this check is necessary to ensure the last page
+	 * of mapping can be represented as a signed long huge page index.
+	 */
+	if ((len >> huge_page_shift(h)) > LONG_MAX)
+		return -EINVAL;
+
 	inode_lock(inode);
 	file_accessed(file);
 
-- 
2.13.6

Re: [PATCH 1/1] hugetlbfs: fix bug in pgoff overflow checking

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.16-rc7 next-20180329]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mike-Kravetz/hugetlbfs-fix-bug-in-pgoff-overflow-checking/20180329-231724
config: i386-randconfig-x000-201812 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   fs/hugetlbfs/inode.c: In function 'hugetlbfs_file_mmap':
>> fs/hugetlbfs/inode.c:119:35: warning: left shift count >= width of type [-Wshift-count-overflow]
     (((1UL << (PAGE_SHIFT + 1)) - 1) << \
                                      ^
   fs/hugetlbfs/inode.c:144:38: note: in expansion of macro 'PGOFF_LOFFT_MAX'
     if ((loff_t)vma->vm_pgoff & (loff_t)PGOFF_LOFFT_MAX)
                                         ^~~~~~~~~~~~~~~

vim +119 fs/hugetlbfs/inode.c

   110	
   111	/*
   112	 * Mask used when checking the page offset value passed in via system
   113	 * calls.  This value will be converted to a loff_t which is signed.
   114	 * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
   115	 * value.  The extra bit (- 1 in the shift value) is to take the sign
   116	 * bit into account.
   117	 */
   118	#define PGOFF_LOFFT_MAX \
 > 119		(((1UL << (PAGE_SHIFT + 1)) - 1) << \
   120		 ((sizeof(loff_t) * BITS_PER_BYTE) - (PAGE_SHIFT + 1)))
   121	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29191 bytes --]

Re: [PATCH 0/1] fix regression in hugetlbfs overflow checking

[Mike Kravetz]

On 03/28/2018 09:16 PM, Mike Kravetz wrote:
> Commit 63489f8e8211 ("hugetlbfs: check for pgoff value overflow")
> introduced a regression in 32 bit kernels.  When creating the mask
> to check vm_pgoff, it incorrectly specified that the size of a loff_t
> was the size of a long.  This prevents mapping hugetlbfs files at
> offsets greater than 4GB on 32 bit kernels.

Well, the kbuild test robot found issues with that as well. :(

I stepped back and did some analysis on what really needs to be
checked WRT arguments causing overflow in the hugetlbfs mmap routine.
For my reference more than anything, here are type sizes to be
concerned with on 32 and 64 bit systems:

Data or type		32 bit system	64 bit system
------------		-------------	-------------
vm_pgoff		32		64
size_t			32		64
loff_t			64		64
huge_page_index		32		64

There are three areas of concern:

1) Will the page offset value passed in vm_pgoff overflow a loff_t?

   On 64 bit systems, vm_off is 64 bits and loff_t is 64 bits.  When
   hugetlbfs mmap is entered via the normal mmap path, the file offset
   is PAGE_SHIFT'ed right and put into vm_pgoff.  However, the
   remap_file_pages system call allows a user to specify a page offset
   which is put directly into vm_pgoff.  Converting from a page offset
   to byte offset is required to get offsets within the underlying
   hugetlbfs file.  In both cases, the value in vm_pgoff when converted
   to a byte offset could overflow a loff_t.

   On 32 bit systems, vm_pgoff is 32 bits and loff_t is 64 bits.  Therefore,
   when converting to a byte address we will never overflow a loff_t.  In
   addition, on 32 bit systems it is perfectly valid for vm_pgoff to be
   as big as ULONG_MAX.  This allows for hugetlbfs files greater than 4GB
   in size.

2) Does vm_pgoff when converted to bytes plus the length of the mapping
   overflow a loff_t?

   On 64 bit systems, we have validated that vm_pgoff will not overflow a
   loff_t when converted to bytes.  But, it is still possible for this
   value plus length to overflow a loff_t.

   On 32 bit systems, vm_pgoff is a 32 bits and can be at most ULONG_MAX.
   Converting this value to bytes is a PAGE_SHIFT left into a 64 bit loff_t.
   length is a 32 bits and can be at most LONG_MAX.  Adding these two
   values can not overflow a 64 bit loff_t.

3) Can vm_pgoff and (vm_pgoff + length) be represented as huge page
   offsets with a signed long?  The hugetlbfs reservation management
   code uses longs for huge page offsets into the underlying file.

   On 64 bit systems, the checks in 1) and 2) have ensured that both values
   can be represented by a loff_t which is a signed 64 bit value.  These
   values will be shifted right by 'huge page shift'  Therefore, they can
   certainly be represented by a signed long.

  On 32 bit systems pg_off can be at most ULONG_MAX.  This value will be
  right shifted 'huge_page_shift - PAGE_SHIFT' bits.  So, as long as
  huge_page_shift is one or more greater than PAGE_SHIFT this value can
  be represented with a signed long.  Adding the maximum length value
  (LONG_MAX) the maximum pg_off byte converted value, would result in
  one more significant bit being set.  For example assuming
  PAGE_SHIFT = 12: 0x0ffffffff000 + 0x00ffffffff = 0x1000ffffefff.
  To represent this as a huge page index, we right shift 'huge_page_shift
  - PAGE_SHIFT' bits.  As long as huge_page_shift is 2 or more greater
  than PAGE_SHIFT, this value can be represented with a signed long.

If we make the following assumptions:
- PAGE_SHIFT will be 2 or more less than BITS_PER_LONG
- huge_page_shift will be 2 or more greater than PAGE_SHIFT
then we need to make to make following checks in the code.

1) Check the upper PAGE_SHIFT+1 bits of vm_pgoff on 64 bit systems.
   Explicitly disable on 32 bit systems.

/*
 * Mask used when checking the page offset value passed in via system
 * calls.  This value will be converted to a loff_t which is signed.
 * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
 * value.  The extra bit (- 1 in the shift value) is to take the sign
 * bit into account.
 */
#define PGOFF_LOFFT_MAX \
	(((1UL << (PAGE_SHIFT + 1)) - 1) <<  (BITS_PER_LONG - (PAGE_SHIFT + 1)))
...
	/*
	 * page based offset in vm_pgoff could be sufficiently large to
	 * overflow a loff_t when converted to byte offset.  This can
	 * only happen on architectures where sizeof(loff_t) ==
	 * sizeof(unsigned long).  So, only check in those instances.
	 */
	if (sizeof(unsigned long) == sizeof(loff_t)) {
		if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
			return -EINVAL;
	}

2) Convert vm_pgoff and length to loff_t byte offset.  Add together and
   check for overflow.  This is unnecessary on 32 bit systems as described
   above, but likely not worth the conditional code.

	vma_len = (loff_t)(vma->vm_end - vma->vm_start);
	len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
	/* check for overflow */
	if (len < vma_len)
		return -EINVAL;

3) After making the above two checks, pg_off and (pg_off + length) can
   be represented as huge page offsets with a signed long.  So, no checks
   needed.

I know this is long and a bore to read.  However, I wanted to get some
feedback before sending another patch.  I have attempted to fix this several
times and seem to always over look some detail.  Hopefully, this will
help others to think of additional concerns.

-- 
Mike Kravetz

[PATCH v2] hugetlbfs: fix bug in pgoff overflow checking

[Mike Kravetz]

This is a fix for a regression in 32 bit kernels caused by an
invalid check for pgoff overflow in hugetlbfs mmap setup.  The
check incorrectly specified that the size of a loff_t was the
same as the size of a long.  The regression prevents mapping
hugetlbfs files at offsets greater than 4GB on 32 bit kernels.

On 32 bit kernels conversion from a page based unsigned long can
not overflow a loff_t byte offset.  Therefore, skip this check
if sizeof(unsigned long) != sizeof(loff_t).

Fixes: 63489f8e8211 ("hugetlbfs: check for pgoff value overflow")
Cc: <stable@vger.kernel.org>
Reported-by: Dan Rue <dan.rue@linaro.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 fs/hugetlbfs/inode.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b9a254dcc0e7..d508c7844681 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -138,10 +138,14 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 
 	/*
 	 * page based offset in vm_pgoff could be sufficiently large to
-	 * overflow a (l)off_t when converted to byte offset.
+	 * overflow a loff_t when converted to byte offset.  This can
+	 * only happen on architectures where sizeof(loff_t) ==
+	 * sizeof(unsigned long).  So, only check in those instances.
 	 */
-	if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
-		return -EINVAL;
+	if (sizeof(unsigned long) == sizeof(loff_t)) {
+		if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
+			return -EINVAL;
+	}
 
 	/* must be huge page aligned */
 	if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
-- 
2.13.6

Re: [PATCH v2] hugetlbfs: fix bug in pgoff overflow checking

[Anders Roxell]

On 30 March 2018 at 16:54, Mike Kravetz <mike.kravetz@oracle.com> wrote:
> This is a fix for a regression in 32 bit kernels caused by an
> invalid check for pgoff overflow in hugetlbfs mmap setup.  The
> check incorrectly specified that the size of a loff_t was the
> same as the size of a long.  The regression prevents mapping
> hugetlbfs files at offsets greater than 4GB on 32 bit kernels.
>
> On 32 bit kernels conversion from a page based unsigned long can
> not overflow a loff_t byte offset.  Therefore, skip this check
> if sizeof(unsigned long) != sizeof(loff_t).
>
> Fixes: 63489f8e8211 ("hugetlbfs: check for pgoff value overflow")
> Cc: <stable@vger.kernel.org>
> Reported-by: Dan Rue <dan.rue@linaro.org>
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Tested-by: Anders Roxell <anders.roxell@linaro.org>

> ---
>  fs/hugetlbfs/inode.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index b9a254dcc0e7..d508c7844681 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -138,10 +138,14 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
>
>         /*
>          * page based offset in vm_pgoff could be sufficiently large to
> -        * overflow a (l)off_t when converted to byte offset.
> +        * overflow a loff_t when converted to byte offset.  This can
> +        * only happen on architectures where sizeof(loff_t) ==
> +        * sizeof(unsigned long).  So, only check in those instances.
>          */
> -       if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
> -               return -EINVAL;
> +       if (sizeof(unsigned long) == sizeof(loff_t)) {
> +               if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
> +                       return -EINVAL;
> +       }
>
>         /* must be huge page aligned */
>         if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> --
> 2.13.6
>