Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932363AbdDPXre convert rfc822-to-8bit (ORCPT ); Sun, 16 Apr 2017 19:47:34 -0400 Received: from tyo162.gate.nec.co.jp ([114.179.232.162]:47976 "EHLO tyo162.gate.nec.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932266AbdDPXrb (ORCPT ); Sun, 16 Apr 2017 19:47:31 -0400 From: Naoya Horiguchi To: Mike Kravetz CC: "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" , Vegard Nossum , Dmitry Vyukov , Hillf Danton , Michal Hocko , "Kirill A . Shutemov" , Andrey Ryabinin , Andrew Morton Subject: Re: [PATCH] hugetlbfs: fix offset overflow in huegtlbfs mmap Thread-Topic: [PATCH] hugetlbfs: fix offset overflow in huegtlbfs mmap Thread-Index: AQHSsxZkjTMJHDy4hEGBUT6eou/C9qHDolkAgALYVoCAAZ7bgA== Date: Sun, 16 Apr 2017 23:43:50 +0000 Message-ID: <20170416234349.GA3395@hori1.linux.bs1.fc.nec.co.jp> References: <1491951118-30678-1-git-send-email-mike.kravetz@oracle.com> <20170414033210.GA12973@hori1.linux.bs1.fc.nec.co.jp> In-Reply-To: Accept-Language: en-US, ja-JP Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.128.101.19] Content-Type: text/plain; charset="iso-2022-jp" Content-ID: <7997CABDB2A4C944A9B13908A926003D@gisp.nec.co.jp> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2045 Lines: 50 On Sat, Apr 15, 2017 at 03:58:59PM -0700, Mike Kravetz wrote: > On 04/13/2017 08:32 PM, Naoya Horiguchi wrote: > > On Tue, Apr 11, 2017 at 03:51:58PM -0700, Mike Kravetz wrote: > > ... > >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c > >> index 7163fe0..dde8613 100644 > >> --- a/fs/hugetlbfs/inode.c > >> +++ b/fs/hugetlbfs/inode.c > >> @@ -136,17 +136,26 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma) > >> vma->vm_flags |= VM_HUGETLB | VM_DONTEXPAND; > >> vma->vm_ops = &hugetlb_vm_ops; > >> > >> + /* > >> + * Offset passed to mmap (before page shift) could have been > >> + * negative when represented as a (l)off_t. > >> + */ > >> + if (((loff_t)vma->vm_pgoff << PAGE_SHIFT) < 0) > >> + return -EINVAL; > >> + > >> 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) > >> + return -EINVAL; > > > > Andrew sent this patch to Linus today, so I know it's a little too late, but > > I think that getting len directly from vma like below might be a simpler fix. > > > > len = (loff_t)(vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT)); > > > > This shouldn't overflow because vma->vm_{end|start|pgoff} are unsigned long, > > but if worried you can add VM_BUG_ON_VMA(len < 0, vma). > > Thanks Naoya, > > I am pretty sure the checks are necessary. You are correct in that > vma->vm_{end|start|pgoff} are unsigned long. However, pgoff can be > a REALLY big value that becomes negative when shifted. > > Note that pgoff is simply the off_t offset value passed from the user cast > to unsigned long and shifted right by PAGE_SHIFT. There is nothing to > prevent a user from passing a 'signed' negative value. In the reproducer > provided, the value passed from user space is 0x8000000000000000ULL. OK, thank you for explanation. You're right. - Naoya