From: Linke Li <[email protected]>
```
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;
```
There is a signed integer overflow in the code, which is undefined
behavior according to the C stacnard. Although this works, it's
still a bit ugly and static checkers will complain.
Using macro "check_add_overflow" to do the overflow check can
effectively detect integer overflow and avoid any undefined behavior.
Signed-off-by: Linke Li <[email protected]>
---
v3: fix checkpatch warning and better description.
fs/hugetlbfs/inode.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 7b17ccfa039d..326a8c0af5f6 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -154,10 +154,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
return -EINVAL;
- 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)
+ if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
return -EINVAL;
inode_lock(inode);
--
2.25.1
On Thu, Jul 20, 2023 at 7:50 AM Linke Li <[email protected]> wrote:
>
> From: Linke Li <[email protected]>
>
> ```
> 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;
> ```
>
> There is a signed integer overflow in the code, which is undefined
> behavior according to the C stacnard. Although this works, it's
typo: s/stacnard/standard/
I think the commit message should explicitly mention that the Linux
kernel is built with -fno-strict-overflow, but IMO that kind of makes
this patch moot.
> still a bit ugly and static checkers will complain.
>
> Using macro "check_add_overflow" to do the overflow check can
> effectively detect integer overflow and avoid any undefined behavior.
>
> Signed-off-by: Linke Li <[email protected]>
> ---
> v3: fix checkpatch warning and better description.
> fs/hugetlbfs/inode.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 7b17ccfa039d..326a8c0af5f6 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -154,10 +154,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> return -EINVAL;
>
> - 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)
> + if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
> return -EINVAL;
>
> inode_lock(inode);
> --
> 2.25.1
>
--
Thanks,
~Nick Desaulniers
On 07/20/23 22:49, Linke Li wrote:
> From: Linke Li <[email protected]>
>
> ```
> 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;
> ```
>
> There is a signed integer overflow in the code, which is undefined
> behavior according to the C stacnard. Although this works, it's
> still a bit ugly and static checkers will complain.
>
> Using macro "check_add_overflow" to do the overflow check can
> effectively detect integer overflow and avoid any undefined behavior.
>
> Signed-off-by: Linke Li <[email protected]>
> ---
> v3: fix checkpatch warning and better description.
> fs/hugetlbfs/inode.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 7b17ccfa039d..326a8c0af5f6 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -154,10 +154,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> return -EINVAL;
>
> - vma_len = (loff_t)(vma->vm_end - vma->vm_start);
I don't think you wanted to delete the above line as ...
> - len = vma_len + ((loff_t)vma->vm_pgoff << PAGE_SHIFT);
> - /* check for overflow */
> - if (len < vma_len)
> + if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
.. vma_len is uninitialized here.
> return -EINVAL;
>
> inode_lock(inode);
> --
> 2.25.1
>
--
Mike Kravetz
Hi Linke,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.5-rc2 next-20230720]
[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/Linke-Li/hugetlbfs-Fix-integer-overflow-check-in-hugetlbfs_file_mmap/20230720-225128
base: linus/master
patch link: https://lore.kernel.org/r/tencent_C2D6865561F23A8141BB145149ACC682B408%40qq.com
patch subject: [PATCH v3] hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap()
config: s390-randconfig-r024-20230720 (https://download.01.org/0day-ci/archive/20230721/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230721/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> fs/hugetlbfs/inode.c:157:25: warning: variable 'vma_len' is uninitialized when used here [-Wuninitialized]
157 | if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
| ^~~~~~~
include/linux/overflow.h:67:47: note: expanded from macro 'check_add_overflow'
67 | __must_check_overflow(__builtin_add_overflow(a, b, d))
| ^
fs/hugetlbfs/inode.c:123:21: note: initialize the variable 'vma_len' to silence this warning
123 | loff_t len, vma_len;
| ^
| = 0
1 warning generated.
vim +/vma_len +157 fs/hugetlbfs/inode.c
108
109 /*
110 * Mask used when checking the page offset value passed in via system
111 * calls. This value will be converted to a loff_t which is signed.
112 * Therefore, we want to check the upper PAGE_SHIFT + 1 bits of the
113 * value. The extra bit (- 1 in the shift value) is to take the sign
114 * bit into account.
115 */
116 #define PGOFF_LOFFT_MAX \
117 (((1UL << (PAGE_SHIFT + 1)) - 1) << (BITS_PER_LONG - (PAGE_SHIFT + 1)))
118
119 static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
120 {
121 struct inode *inode = file_inode(file);
122 struct hugetlbfs_inode_info *info = HUGETLBFS_I(inode);
123 loff_t len, vma_len;
124 int ret;
125 struct hstate *h = hstate_file(file);
126
127 /*
128 * vma address alignment (but not the pgoff alignment) has
129 * already been checked by prepare_hugepage_range. If you add
130 * any error returns here, do so after setting VM_HUGETLB, so
131 * is_vm_hugetlb_page tests below unmap_region go the right
132 * way when do_mmap unwinds (may be important on powerpc
133 * and ia64).
134 */
135 vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
136 vma->vm_ops = &hugetlb_vm_ops;
137
138 ret = seal_check_future_write(info->seals, vma);
139 if (ret)
140 return ret;
141
142 /*
143 * page based offset in vm_pgoff could be sufficiently large to
144 * overflow a loff_t when converted to byte offset. This can
145 * only happen on architectures where sizeof(loff_t) ==
146 * sizeof(unsigned long). So, only check in those instances.
147 */
148 if (sizeof(unsigned long) == sizeof(loff_t)) {
149 if (vma->vm_pgoff & PGOFF_LOFFT_MAX)
150 return -EINVAL;
151 }
152
153 /* must be huge page aligned */
154 if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
155 return -EINVAL;
156
> 157 if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
158 return -EINVAL;
159
160 inode_lock(inode);
161 file_accessed(file);
162
163 ret = -ENOMEM;
164 if (!hugetlb_reserve_pages(inode,
165 vma->vm_pgoff >> huge_page_order(h),
166 len >> huge_page_shift(h), vma,
167 vma->vm_flags))
168 goto out;
169
170 ret = 0;
171 if (vma->vm_flags & VM_WRITE && inode->i_size < len)
172 i_size_write(inode, len);
173 out:
174 inode_unlock(inode);
175
176 return ret;
177 }
178
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, Jul 20, 2023 at 10:49:52PM +0800, Linke Li wrote:
> +++ b/fs/hugetlbfs/inode.c
> @@ -154,10 +154,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> return -EINVAL;
>
> - 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)
> + if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
> return -EINVAL;
Doesn't this check duplicate that performed by file_mmap_ok()? Can't we
just delete the check, or is there a code path that leads here while
avoiding file_mmap_ok()?
Hello,
kernel test robot noticed "WARNING:at_mm/hugetlb.c:#hugetlb_reserve_pages" on:
commit: c5ffbcff59bdefdc2f54a0d12955da6e3fe79e21 ("[PATCH v3] hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap()")
url: https://github.com/intel-lab-lkp/linux/commits/Linke-Li/hugetlbfs-Fix-integer-overflow-check-in-hugetlbfs_file_mmap/20230720-225128
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git bfa3037d828050896ae52f6467b6ca2489ae6fb1
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH v3] hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap()
in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:
runtime: 300s
group: group-04
nr_groups: 5
test-description: Trinity is a linux system call fuzz tester.
test-url: http://codemonkey.org.uk/projects/trinity/
compiler: clang-15
test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-lkp/[email protected]
[ 39.198826][ T3397] ------------[ cut here ]------------
[ 39.199142][ T3397] hugetlb_reserve_pages called with a negative range
[ 39.199541][ T3397] WARNING: CPU: 0 PID: 3397 at mm/hugetlb.c:6852 hugetlb_reserve_pages+0x39/0x410
[ 39.200009][ T3397] Modules linked in:
[ 39.200211][ T3397] CPU: 0 PID: 3397 Comm: trinity-main Not tainted 6.5.0-rc2-00053-gc5ffbcff59bd #1
[ 39.200711][ T3397] EIP: hugetlb_reserve_pages+0x39/0x410
[ 39.200992][ T3397] Code: 80 cc 03 00 00 8b 58 34 8b 40 38 c7 45 e4 00 00 00 00 89 cf 29 d7 7d 24 68 28 48 ca 82 68 43 ec bd 82 e8 3a 9a e2 ff 83 c4
08 <0f> 0b 31 db 89 d8 83 c4 18 5e 5f 5b 5d 31 c9 31 d2 c3 89 55 e0 89
[ 39.201989][ T3397] EAX: 00000000 EBX: 83cdb4a8 ECX: 00000000 EDX: 00000000
[ 39.202347][ T3397] ESI: ec0002d8 EDI: ffffffff EBP: ec7d9dc8 ESP: ec7d9da4
[ 39.202749][ T3397] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068 EFLAGS: 00010292
[ 39.203139][ T3397] CR0: 80050033 CR2: 01100000 CR3: 6b83c000 CR4: 00040690
[ 39.203534][ T3397] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 39.203893][ T3397] DR6: fffe0ff0 DR7: 00000400
[ 39.204132][ T3397] Call Trace:
[ 39.204302][ T3397] ? show_regs+0x4d/0x60
[ 39.204553][ T3397] ? hugetlb_reserve_pages+0x39/0x410
[ 39.204827][ T3397] ? __warn+0xa2/0x180
[ 39.205036][ T3397] ? hugetlb_reserve_pages+0x39/0x410
[ 39.205308][ T3397] ? hugetlb_reserve_pages+0x39/0x410
[ 39.205613][ T3397] ? report_bug+0x96/0x120
[ 39.205845][ T3397] ? vprintk_emit+0x6b/0x120
[ 39.206082][ T3397] ? exc_overflow+0x40/0x40
[ 39.206315][ T3397] ? handle_bug+0x29/0x50
[ 39.206569][ T3397] ? exc_invalid_op+0x17/0x40
[ 39.206816][ T3397] ? handle_exception+0x115/0x115
[ 39.207075][ T3397] ? exc_overflow+0x40/0x40
[ 39.207307][ T3397] ? hugetlb_reserve_pages+0x39/0x410
[ 39.207612][ T3397] ? exc_overflow+0x40/0x40
[ 39.207852][ T3397] ? hugetlb_reserve_pages+0x39/0x410
[ 39.208126][ T3397] ? hugetlbfs_file_mmap+0xb3/0x1b0
[ 39.208392][ T3397] hugetlbfs_file_mmap+0x116/0x1b0
[ 39.208689][ T3397] mmap_region+0x4c1/0x930
[ 39.208918][ T3397] ? vm_unmapped_area+0x1be/0x2b0
[ 39.209183][ T3397] do_mmap+0x352/0x4f0
[ 39.209396][ T3397] vm_mmap_pgoff+0x80/0x100
[ 39.209659][ T3397] ksys_mmap_pgoff+0x1aa/0x290
[ 39.209907][ T3397] __ia32_sys_mmap_pgoff+0x1c/0x30
[ 39.210168][ T3397] __do_fast_syscall_32+0x90/0xe0
[ 39.210425][ T3397] ? lockdep_hardirqs_on+0x85/0x110
[ 39.210720][ T3397] do_fast_syscall_32+0x28/0x60
[ 39.210973][ T3397] do_SYSENTER_32+0x12/0x20
[ 39.211204][ T3397] entry_SYSENTER_32+0xa6/0x10c
[ 39.211452][ T3397] EIP: 0x77fb6539
[ 39.211668][ T3397] Code: 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00
[ 39.212668][ T3397] EAX: ffffffda EBX: 00000000 ECX: 00001000 EDX: 00000001
[ 39.213024][ T3397] ESI: 00050022 EDI: 00000023 EBP: 00000000 ESP: 7ff736bc
[ 39.213379][ T3397] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b EFLAGS: 00000246
[ 39.213794][ T3397] irq event stamp: 2287591
[ 39.214020][ T3397] hardirqs last enabled at (2287599): [<8108610a>] __up_console_sem+0x5a/0x70
[ 39.214468][ T3397] hardirqs last disabled at (2287606): [<810860f1>] __up_console_sem+0x41/0x70
[ 39.214944][ T3397] softirqs last enabled at (2287262): [<81007a66>] do_softirq_own_stack+0x26/0x30
[ 39.215410][ T3397] softirqs last disabled at (2287253): [<81007a66>] do_softirq_own_stack+0x26/0x30
[ 39.215891][ T3397] ---[ end trace 0000000000000000 ]---
To reproduce:
# build kernel
cd linux
cp config-6.5.0-rc2-00053-gc5ffbcff59bd .config
make HOSTCC=clang-15 CC=clang-15 ARCH=i386 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=clang-15 CC=clang-15 ARCH=i386 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 07/24/23 04:47, Matthew Wilcox wrote:
> On Thu, Jul 20, 2023 at 10:49:52PM +0800, Linke Li wrote:
> > +++ b/fs/hugetlbfs/inode.c
> > @@ -154,10 +154,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
> > if (vma->vm_pgoff & (~huge_page_mask(h) >> PAGE_SHIFT))
> > return -EINVAL;
> >
> > - 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)
> > + if (check_add_overflow(vma_len, (loff_t)vma->vm_pgoff << PAGE_SHIFT, &len))
> > return -EINVAL;
>
> Doesn't this check duplicate that performed by file_mmap_ok()? Can't we
> just delete the check, or is there a code path that leads here while
> avoiding file_mmap_ok()?
Thanks for pointing that out.
Yes, from my reading/understanding that is a repeat.
It looks like most of the overflow checking in hugetlbfs_file_mmap is a
repeat of checks done previously. I remember adding this code in
response to a checker or someone pointing out the potential for overflow:
/*
* 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;
}
However, file_mmap_ok seems to handle this as well. The important thing that
needs to be done in hugetlbfs_file_mmap is checking for huge page alignment.
I have added this code cleanup to my list if someone does not do it first.
--
Mike Kravetz