Received: by 2002:a05:6358:5282:b0:b5:90e7:25cb with SMTP id g2csp3605701rwa; Tue, 23 Aug 2022 07:20:04 -0700 (PDT) X-Google-Smtp-Source: AA6agR4QwarI0n1dQLIbNm7XZcqMKhv3oXS7lv8pWGk/LP/XMzUW3Kx5uDZgyG4u/4MYvfpzR0aw X-Received: by 2002:a17:902:6b4c:b0:171:38ab:e762 with SMTP id g12-20020a1709026b4c00b0017138abe762mr24396893plt.42.1661264404281; Tue, 23 Aug 2022 07:20:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1661264404; cv=none; d=google.com; s=arc-20160816; b=Y8Eem2tWxeLALrtozgbZqujTmcNN7luFJeWF0sxQnTydSX0N7OCnokcszyKMzfAFhM T0Mgb3QNbRH8A+b1z4XGRpffhAKQKu8QPI+8gVsi3cqmGNFDKr+Svp/947hIt0b1VxQ+ U2Qxs/+rGIXsZfOR/9489HNVUymQ63I/Wp1+k7rb36R7MGKA13LWZ2EF1h+XxR/RL5A8 RtVTKnn2tVaXXB880yb5TNzYw3lli0lJx/DDJOYREK8w1yRAMOX8P3GHWfBKtUk9Cwnq B3kwxelXg9lqi7YdL49S5IiyuIBXBCkms2tnhZIwzHsEYZKKcBN2aAYFkV/ELdB+/i9L jvcA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to :mime-version:user-agent:date:message-id:from:cc:references:to :subject; bh=mspWW8MvDpdPHdcT1bYktS3VjQTrmJxWVSFNy36w0a0=; b=WFMiA2i53Db/GMyrRhSdWqA4T4WuxB99wA8ZUuqLmjZIqu3Cs/gXPdaMfDzlPcXzrE NfveThHeXr/xTwDmTnQxXAFzlA3mE4yLZfqbCDMf548KDfte5MlGVmXfPoOmM8dmIfK7 r1wOIMxCwoWlXRj7EaGeSKzJ69JUNrIo3vGBXyV3Ioi42In/a71D+UEqSa56sjYzAAaP IMNS3McQ5uZEfI0CVCispTTE4tqBBcV2DBDeGJS8HrX6HWsSJikfO41CWyfc0zgBFqPz 792gGXLqv/y9bg18VADqS/h7J+tTkXFqTBbp024HpSWzlZvN26VbvzwBSxTSCBv2ugk5 rLHA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id q6-20020a056a00084600b0052ef99d2e85si15573810pfk.46.2022.08.23.07.19.51; Tue, 23 Aug 2022 07:20:04 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240924AbiHWON1 (ORCPT + 99 others); Tue, 23 Aug 2022 10:13:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54760 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242585AbiHWONE (ORCPT ); Tue, 23 Aug 2022 10:13:04 -0400 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A3DAF264E35; Tue, 23 Aug 2022 04:24:29 -0700 (PDT) Received: from canpemm500010.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4MBmYM4bVnznThm; Tue, 23 Aug 2022 19:03:19 +0800 (CST) Received: from [10.174.178.185] (10.174.178.185) by canpemm500010.china.huawei.com (7.192.105.118) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 23 Aug 2022 19:05:37 +0800 Subject: Re: [PATCH -next] btrfs: fix use-after-free in btrfs_get_global_root To: Filipe Manana References: <20220823015931.421355-1-yebin10@huawei.com> <20220823090657.GB3171944@falcondesktop> CC: , , , , From: yebin Message-ID: <6304B481.9010505@huawei.com> Date: Tue, 23 Aug 2022 19:05:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20220823090657.GB3171944@falcondesktop> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.178.185] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To canpemm500010.china.huawei.com (7.192.105.118) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE 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: X-Mailing-List: linux-kernel@vger.kernel.org On 2022/8/23 17:06, Filipe Manana wrote: > On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote: >> Syzkaller reported UAF as follows: >> ================================================================== >> BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10 >> Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11 >> >> CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2 >> Workqueue: btrfs-qgroup-rescan btrfs_work_helper >> Call Trace: >> >> dump_stack_lvl+0x6e/0x91 >> print_report.cold+0xb2/0x6bb >> kasan_report+0xa8/0x130 >> kasan_check_range+0x13f/0x1d0 >> btrfs_get_global_root+0x663/0xa10 >> btrfs_get_fs_root_commit_root+0xa5/0x150 >> find_parent_nodes+0x92f/0x2990 >> btrfs_find_all_roots_safe+0x12d/0x220 >> btrfs_find_all_roots+0xbb/0xd0 >> btrfs_qgroup_rescan_worker+0x600/0xc30 >> btrfs_work_helper+0xff/0x750 >> process_one_work+0x52c/0x930 >> worker_thread+0x352/0x8c0 >> kthread+0x1b9/0x200 >> ret_from_fork+0x22/0x30 >> >> >> Allocated by task 1895: >> kasan_save_stack+0x1e/0x40 >> __kasan_kmalloc+0xa9/0xe0 >> btrfs_alloc_root+0x40/0x820 >> btrfs_create_tree+0xf8/0x500 >> btrfs_quota_enable+0x30a/0x1120 >> btrfs_ioctl+0x50a3/0x59f0 >> __x64_sys_ioctl+0x130/0x170 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Freed by task 1895: >> kasan_save_stack+0x1e/0x40 >> kasan_set_track+0x21/0x30 >> kasan_set_free_info+0x20/0x40 >> __kasan_slab_free+0x127/0x1c0 >> kfree+0xa8/0x2d0 >> btrfs_put_root+0x1ca/0x230 >> btrfs_quota_enable+0x87c/0x1120 >> btrfs_ioctl+0x50a3/0x59f0 >> __x64_sys_ioctl+0x130/0x170 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> ================================================================== >> >> Above issue may happens as follows: >> p1 p2 >> btrfs_quota_enable >> spin_lock(&fs_info->qgroup_lock); >> fs_info->quota_root = quota_root; >> spin_unlock(&fs_info->qgroup_lock); >> >> ret = qgroup_rescan_init -> return error >> if (ret) >> btrfs_put_root(quota_root); >> kfree(root); >> >> if (ret) { >> ulist_free(fs_info->qgroup_ulist); >> fs_info->qgroup_ulist = NULL; >> btrfs_sysfs_del_qgroups(fs_info); >> } btrfs_qgroup_rescan_worker >> btrfs_find_all_roots >> btrfs_find_all_roots_safe >> find_parent_nodes >> btrfs_get_fs_root_commit_root >> btrfs_grab_root(fs_info->quota_root) >> -> quota_root already freed I have described the process of the issue here. I can write it in more detail. >> Syzkaller also reported another issue: >> ================================================================== >> BUG: KASAN: use-after-free in ulist_release+0x30/0xb3 >> Read of size 8 at addr ffff88811413d048 by task rep/2921 >> >> CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3 >> rep[2921] cmdline: ./rep >> Call Trace: >> >> dump_stack_lvl+0x6e/0x91 >> print_report.cold+0xb2/0x6bb >> kasan_report+0xa8/0x130 >> ulist_release+0x30/0xb3 >> ulist_reinit+0x16/0x56 >> btrfs_qgroup_free_refroot+0x288/0x3f0 >> btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0 >> commit_fs_roots+0x28c/0x430 >> btrfs_commit_transaction+0x9a6/0x1b40 >> btrfs_qgroup_rescan+0x7e/0x130 >> btrfs_ioctl+0x48ed/0x59f0 >> __x64_sys_ioctl+0x130/0x170 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> >> Allocated by task 2900: >> kasan_save_stack+0x1e/0x40 >> __kasan_kmalloc+0xa9/0xe0 >> ulist_alloc+0x5c/0xe0 >> btrfs_quota_enable+0x1b2/0x1160 >> btrfs_ioctl+0x50a3/0x59f0 >> __x64_sys_ioctl+0x130/0x170 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> >> Freed by task 2900: >> kasan_save_stack+0x1e/0x40 >> kasan_set_track+0x21/0x30 >> kasan_set_free_info+0x20/0x40 >> __kasan_slab_free+0x127/0x1c0 >> kfree+0xa8/0x2d0 >> ulist_free.cold+0x15/0x1a >> btrfs_quota_enable+0x8bf/0x1160 >> btrfs_ioctl+0x50a3/0x59f0 >> __x64_sys_ioctl+0x130/0x170 >> do_syscall_64+0x3b/0x90 >> entry_SYSCALL_64_after_hwframe+0x63/0xcd >> ================================================================== >> >> To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init >> return success. >> >> Signed-off-by: Ye Bin >> --- >> fs/btrfs/qgroup.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index db723c0026bd..16f0b038295a 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info) >> if (ret) >> goto out_free_path; >> >> - /* >> - * Set quota enabled flag after committing the transaction, to avoid >> - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot >> - * creation. >> - */ >> - spin_lock(&fs_info->qgroup_lock); >> - fs_info->quota_root = quota_root; >> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags); >> - spin_unlock(&fs_info->qgroup_lock); >> - >> ret = qgroup_rescan_init(fs_info, 0, 1); >> if (!ret) { >> + /* >> + * Set quota enabled flag after committing the transaction, to >> + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent >> + * snapshot creation. >> + */ >> + spin_lock(&fs_info->qgroup_lock); >> + fs_info->quota_root = quota_root; >> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags); >> + spin_unlock(&fs_info->qgroup_lock); >> + > But how can the race happen? The changelog should explain that. > > To me this suggests that after we set BTRFS_FS_QUOTA_ENABLED and set the > quota root, but before we called qgroup_rescan_init() at btrfs_quota_enable(), > some other task started the rescan worker first - I can only think of > someone else calling the ioctl to start the rescan worker (btrfs_ioctl_quota_rescan()). Yes, rescan worker is triggered by ioctl(btrfs_ioctl_quota_rescan()). > In that case we get "ret == -EINPROGRESS" at btrfs_quota_enable(). > > So please provide a detailed explanation in the log of how the race can > happen. > > This solution is also buggy. Because in case of an error, we will leave the > quota tree created, the qgroup relation, etc. That is, we don't undo > what btrfs_create_tree(), add_qgroup_item(), add_qgroup_rb(), etc did > Which means a future btrfs_quota_enable() call would fail, and calling > btrfs_quota_disable() to undo all those things will not work either, > because fs_info->quota_root is NULL. As 'ret' is non-zero will release resources which you mentioned. btrfs_quota_enable ... out_free_path: btrfs_free_path(path); out_free_root: if (ret) btrfs_put_root(quota_root); out: if (ret) { ulist_free(fs_info->qgroup_ulist); fs_info->qgroup_ulist = NULL; btrfs_sysfs_del_qgroups(fs_info); } mutex_unlock(&fs_info->qgroup_ioctl_lock); if (ret && trans) btrfs_end_transaction(trans); else if (trans) ret = btrfs_end_transaction(trans); ulist_free(ulist); ... > > I would suggest ignoring the error of qgroup_rescan_init() if it's > -EINPROGRESS, and ASSERT if it's anything different from 0 or > -EINPROGRESS. Also add a comment mentioning we can get -EINPROGRESS > because someone may have called the qgroup rescan ioctl. > > Thanks. > > > > >> qgroup_rescan_zero_tracking(fs_info); >> fs_info->qgroup_rescan_running = true; >> btrfs_queue_work(fs_info->qgroup_rescan_workers, >> -- >> 2.31.1 >> > . >