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
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: <[email protected]>
Reported-by: Dan Rue <[email protected]>
Signed-off-by: Mike Kravetz <[email protected]>
---
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
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
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
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: <[email protected]>
Reported-by: Dan Rue <[email protected]>
Signed-off-by: Mike Kravetz <[email protected]>
---
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
On 30 March 2018 at 16:54, Mike Kravetz <[email protected]> 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: <[email protected]>
> Reported-by: Dan Rue <[email protected]>
> Signed-off-by: Mike Kravetz <[email protected]>
Tested-by: Anders Roxell <[email protected]>
> ---
> 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
>