Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Sep 2022 10:18:42 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     David Hildenbrand <david@redhat.com>, Peter Xu <peterx@redhat.com>
Cc:     Liu Shixin <liushixin2@huawei.com>,
        Liu Zixian <liuzixian4@huawei.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        John Hubbard <jhubbard@nvidia.com>
Subject: Re: [PATCH] mm: hugetlb: fix UAF in hugetlb_handle_userfault
Message-ID: <YyyY8prkoxy2qv+W@monkey>
References: <20220921083440.1267903-1-liushixin2@huawei.com>
 <YytOYH1MSo5cNoB6@monkey>
 <Yyuk83B4VHh+pbFp@monkey>
 <708d1ec7-5c25-5e45-0db8-bd97a64e0db1@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <708d1ec7-5c25-5e45-0db8-bd97a64e0db1@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?2V2eldos7fwgJqDEKqSjq/vTMDeybr9YrK8bmR8C0Eevss0Qksm25lFCcXYU?=
 =?us-ascii?Q?W2t6S1s+4ebwKy4aHlLhYaJSckin8Lvj0Dhh8AsomdTRoDeGZ4swXkNJn8l0?=
 =?us-ascii?Q?BxV5hKp2NM2sHht3lKxZ3kL43/02Y71JIq1Xc8P2215Xx5cAroWe1SyJ+Cr0?=
 =?us-ascii?Q?4aJvNABDYlzfik4gmqJGjFWNsLniQFZ5ZdUg3g7nFIhJeH0lthN1DyCxx+KZ?=
 =?us-ascii?Q?hRYiF8lbgCMETxp8k1k3cp0Ge3Oge7YKH9rruCz4aaj06g4ZB9W3/xaoYB3s?=
 =?us-ascii?Q?SEFJgCMU+WFnn7Xl6im5oyth1jsqoKZTSNmAlJEhj0js0ejyLPwHrRE9kIW0?=
 =?us-ascii?Q?ALAzDabBdPgNuQgsh7VC9OSWzzirULp9ahWdpb+HTrg8+THAa8kdEMt1LsNq?=
 =?us-ascii?Q?jkYrPQ4jDZNrYHlHI5HEGB5m3adP6oaBYqKWMycLhT1T0+Q8b9K0R46kaGNd?=
 =?us-ascii?Q?35FkDqj74wkTrjMgSPbNMep7mh7NMqIc5IVzgleP/Ox7rcTCG+7AI+31+aAM?=
 =?us-ascii?Q?Q7ZlCIOadgfexLFprp2+pePAqfCBmocnRPiv5w9kmvIF3AssM1ultC/iHard?=
 =?us-ascii?Q?I57r4X88Wi3vVNUwK1sm8ck0CUEB3aQcz86CmFwpe/BPODALMutPM2V3khvF?=
 =?us-ascii?Q?JHGvVpIY2zjAAlNA2eR9pbrTGS4zWpy/2JUWHeseQI6DT6deh8TtFc0njldc?=
 =?us-ascii?Q?8W8cmftFY2X1AsngrFoosckoE867V3YuAbXAEtdl0UMiicINHqZK0cMGNyTy?=
 =?us-ascii?Q?c3xhvniCAcRrZJVJ4OTQUJlC4Yt2gd4rqASVdQbZIn6fV8oyo1UWY7TS5yXM?=
 =?us-ascii?Q?IbBaF++ntogyPbCBfvhc8gTB3FHgP6yF42/3LgOQnxsiLsAL4KT7chYY6l57?=
 =?us-ascii?Q?tmJWbyiIV8pYUk+dVF2AyNooLdi0Cvc/gbI6N/YpbPWyjj51d3tBVjGGo8je?=
 =?us-ascii?Q?8IYeAqZqjNQmnTTsi6L208+7LwZkgunWCqh4028ipCUq7bErVyr7E95poQk+?=
 =?us-ascii?Q?Ob77dEo7I9/Ice6BuOxRj1EQXbvATxsj/fWz/yMd15wrDFN4PedKKpANr4m5?=
 =?us-ascii?Q?oBHL4T5bniG2Q6YUTswcxuGyVplWrx7w42uGBQLXUIg7zlOd8PxAFYBRD6vm?=
 =?us-ascii?Q?hvNceQwJ8CbAAqGSKtHxP+vSfaFR9Lt67f6LtZFTJvw+VzxN4BF1JpH3H9+p?=
 =?us-ascii?Q?E6J1Lfc7ZxIFQHa6gA3IEMfEcjpe1jiK1PPBOTDIrwog+XWvqYw/m2TRc+GT?=
 =?us-ascii?Q?uWuEdpdtW7Llzx8sk5mdU5qyEa1CQZoINqXlaQ7pN39Um6ixAmR+5uB2WbQ0?=
 =?us-ascii?Q?+3nFSZeP6/Co7f32Tpd49qzj/E+hE6PKgzExcXCdgp089fPWjoMtJmR7YeV5?=
 =?us-ascii?Q?gLyY2qd4Y16q66JWVNdbuLZVZnhLGxICuAoRjRfqoSvEb53lw5r2Tf/8x87c?=
 =?us-ascii?Q?3vnPH82ok66QIh7pmsLv1OKaT6mtptkNRqgkb64MZsKC2fRrnAhNAryphEfe?=
 =?us-ascii?Q?GraO3LmxNOgUaWtrsYYWBzvWs/7vhjiccHuK71p2qta7PttXpblNixAYG0w/?=
 =?us-ascii?Q?vSaAjF6MKaENkQeTk77IT41Cyz8rcyQ9b2cb+Fatek60BX4PMt2PaWUYRhRr?=
 =?us-ascii?Q?5w=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b7c20090-21bc-49dd-f37e-08da9cbe8138
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2022 17:18:45.3933
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YwjYoDthurL7VMbsOPeFu5ywcX5ikawFCqd+fhn8DRrJfxxy8EpdwIAKVdaXjxLabU41VqxnJC9c65x4IP3BvA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4503
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-22_12,2022-09-22_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=357 adultscore=0
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 suspectscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209220115
X-Proofpoint-ORIG-GUID: okIIxXQQe2uGPhqULWUTQvAITxIqb34g
X-Proofpoint-GUID: okIIxXQQe2uGPhqULWUTQvAITxIqb34g
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 09/22/22 09:46, David Hildenbrand wrote:
> On 22.09.22 01:57, Mike Kravetz wrote:
> > On 09/21/22 10:48, Mike Kravetz wrote:
> > > On 09/21/22 16:34, Liu Shixin wrote:
> > > > The vma_lock and hugetlb_fault_mutex are dropped before handling
> > > > userfault and reacquire them again after handle_userfault(), but
> > > > reacquire the vma_lock could lead to UAF[1] due to the following
> > > > race,
> > > > 
> > > > hugetlb_fault
> > > >    hugetlb_no_page
> > > >      /*unlock vma_lock */
> > > >      hugetlb_handle_userfault
> > > >        handle_userfault
> > > >          /* unlock mm->mmap_lock*/
> > > >                                             vm_mmap_pgoff
> > > >                                               do_mmap
> > > >                                                 mmap_region
> > > >                                                   munmap_vma_range
> > > >                                                     /* clean old vma */
> > > >          /* lock vma_lock again  <--- UAF */
> > > >      /* unlock vma_lock */
> > > > 
> > > > Since the vma_lock will unlock immediately after hugetlb_handle_userfault(),
> > > > let's drop the unneeded lock and unlock in hugetlb_handle_userfault() to fix
> > > > the issue.
> > > 
> > > Thank you very much!
> > > 
> > > When I saw this report, the obvious fix was to do something like what you have
> > > done below.  That looks fine with a few minor comments.
> > > 
> > > One question I have not yet answered is, "Does this same issue apply to
> > > follow_hugetlb_page()?".  I believe it does.  follow_hugetlb_page calls
> > > hugetlb_fault which could result in the fault being processed by userfaultfd.
> > > If we experience the race above, then the associated vma could no longer be
> > > valid when returning from hugetlb_fault.  follow_hugetlb_page and callers
> > > have a flag (locked) to deal with dropping mmap lock.  However, I am not sure
> > > if it is handled correctly WRT userfaultfd.  I think this needs to be answered
> > > before fixing.  And, if the follow_hugetlb_page code needs to be fixed it
> > > should be done at the same time.
> > > 
> > 
> > To at least verify this code path, I added userfaultfd handling to the gup_test
> > program in kernel selftests.  When doing basic gup test on a hugetlb page in
> > a userfaultfd registered range, I hit this warning:
> > 
> > [ 6939.867796] FAULT_FLAG_ALLOW_RETRY missing 1
> > [ 6939.871503] CPU: 2 PID: 5720 Comm: gup_test Not tainted 6.0.0-rc6-next-20220921+ #72
> > [ 6939.874562] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1.fc35 04/01/2014
> > [ 6939.877707] Call Trace:
> > [ 6939.878745]  <TASK>
> > [ 6939.879779]  dump_stack_lvl+0x6c/0x9f
> > [ 6939.881199]  handle_userfault.cold+0x14/0x1e
> > [ 6939.882830]  ? find_held_lock+0x2b/0x80
> > [ 6939.884370]  ? __mutex_unlock_slowpath+0x45/0x280
> > [ 6939.886145]  hugetlb_handle_userfault+0x90/0xf0
> > [ 6939.887936]  hugetlb_fault+0xb7e/0xda0
> > [ 6939.889409]  ? vprintk_emit+0x118/0x3a0
> > [ 6939.890903]  ? _printk+0x58/0x73
> > [ 6939.892279]  follow_hugetlb_page.cold+0x59/0x145
> > [ 6939.894116]  __get_user_pages+0x146/0x750
> > [ 6939.895580]  __gup_longterm_locked+0x3e9/0x680
> > [ 6939.897023]  ? seqcount_lockdep_reader_access.constprop.0+0xa5/0xb0
> > [ 6939.898939]  ? lockdep_hardirqs_on+0x7d/0x100
> > [ 6939.901243]  gup_test_ioctl+0x320/0x6e0
> > [ 6939.902202]  __x64_sys_ioctl+0x87/0xc0
> > [ 6939.903220]  do_syscall_64+0x38/0x90
> > [ 6939.904233]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > [ 6939.905423] RIP: 0033:0x7fbb53830f7b
> > 
> > This is because userfaultfd is expecting FAULT_FLAG_ALLOW_RETRY which is not
> > set in this path.
> 
> Right. Without being able to drop the mmap lock, we cannot continue. And we
> don't know if we can drop it without FAULT_FLAG_ALLOW_RETRY.
> 
> FAULT_FLAG_ALLOW_RETRY is only set when we can communicate to the caller
> that we dropped the mmap lock [e.g., int *locked parameter].
> 
> All code paths that pass NULL won't be able to handle --  especially
> surprisingly also pin_user_pages_fast() -- cannot trigger usefaultfd and
> will result in this warning.
> 
> 
> A "sane" example is access via /proc/self/mem via ptrace: we don't want to
> trigger userfaultfd, but instead simply fail the GUP get/pin.
> 
> 
> Now, this is just a printed *warning* (not a WARN/BUG/taint) that tells us
> that there is a GUP user that isn't prepared for userfaultfd. So it rather
> points out a missing GUP adaption -- incomplete userfaultfd support. And we
> seem to have plenty of that judging that pin_user_pages_fast_only().
> 
> Maybe the printed stack trace is a bit too much and makes this look very
> scary.
> 
> > 
> > Adding John, Peter and David on Cc: as they are much more fluent in all the
> > fault and FOLL combinations and might have immediate suggestions.  It is going
> > to take me a little while to figure out:
> > 1) How to make sure we get the right flags passed to handle_userfault
> 
> This is a GUP caller problem -- or rather, how GUP has to deal with
> userfaultfd.
> 
> > 2) How to modify follow_hugetlb_page as userfaultfd can certainly drop
> >     mmap_lock.  So we can not assume vma still exists upon return.
> 
> Again, we have to communicate to the GUP caller that we dropped the mmap
> lock. And that requires GUP caller changes.
> 

Thank you and Peter for replying!

The 'good news' is that there does not appear to be a case where userfaultfd
(via hugetlb_fault) drops the lock and follow_hugetlb_page is not prepard for
the consequences.  So, this is not an exposure as in hugetlb_handle_userfault
that is in need of an immediate fix.  i.e. A fix like that originally proposed
here is sufficient.

We can think about whether this specific calling sequence needs to be modified.
-- 
Mike Kravetz