| Message ID | tencent_C2D6865561F23A8141BB145149ACC682B408@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] hugetlbfs: Fix integer overflow check in hugetlbfs_file_mmap() | expand |
On 07/20/23 22:49, Linke Li wrote: > From: Linke Li <lilinke99@gmail.com> > > ``` > 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 <lilinke99@gmail.com> > --- > 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 >
On Thu, Jul 20, 2023 at 7:50 AM Linke Li <lilinke99@foxmail.com> wrote: > > From: Linke Li <lilinke99@gmail.com> > > ``` > 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 <lilinke99@gmail.com> > --- > 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 >
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/202307210737.HUPwpBdW-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230721/202307210737.HUPwpBdW-lkp@intel.com/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 <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202307210737.HUPwpBdW-lkp@intel.com/ 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
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/tencent_C2D6865561F23A8141BB145149ACC682B408@qq.com/
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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202307241509.546e3574-oliver.sang@intel.com
[ 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.
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.
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);