Message ID | 20230330202046.795213-4-yukuai1@huaweicloud.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1092787vqo; Thu, 30 Mar 2023 05:37:30 -0700 (PDT) X-Google-Smtp-Source: AKy350bLZGXlqvt9nYfM2whN+v2U0G3mKpAE0LT3fq1t2paUGltaHXYq0QSey3AMIaECicfX5f5M X-Received: by 2002:aa7:9521:0:b0:62a:1267:2036 with SMTP id c1-20020aa79521000000b0062a12672036mr23134212pfp.34.1680179850266; Thu, 30 Mar 2023 05:37:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680179850; cv=none; d=google.com; s=arc-20160816; b=0nH9QbcOpo9BdQ5G3YfP7MtBg/6tyhcJH8kRiJHffphfq5hVG+wE+OxKs/T7GzbCq/ s1zhG+zQDzfw1hsv1Bg07WwgtIHb15BNSy6edIiAWXH1ABRE09pG2o3ELfKBD2BppIvn WAdc0/ga60dpIR3GyN0Rr0gahBYkgoYdyRnsgtdJz8In1ag/5vzzTG4mgXSpsUnzaW4k UVaGHFsWD0Cx+1PoTKT0z/ZTVpHVtiJ3iOqdpNLCRAdB2NxgsTfKF2wzN0KdlxNKqJVW hWZDmuwjPEMosH0bDfkEclCp3sIuLsnjV4xB3S+EQA3LVnMtNWxioZ8H1hACGcblD987 r/Kg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=mhmBJ6dFaxxJE70kalj6KsxcznteHHDUeIVORH6yedE=; b=V14ZqDQk6dIgoO85HLFZ3+pHYc9jeYPM7YAOyNImK4SsQaufOxIfVO2fSUEUjj6Cw5 PBYcWCF0+dSSj2ljC3GZVJqhivJ04w8L5BVj8W8HNEgj3wBObECY3WkHw5s1P7dCjwUC jn0xFpSkyage0ugqkMJuz/tmtXtIpx6EDFuLrwyAeIB4ypH18kD93QpeJu12H/mjJJRu 6glgfQQeH3xuwNIM4s+DCsQM0xIu8VUOR5GsstlHrK3Po1Lh2g24QSujj3LbJBxip6lS ueAO5IqIWAMMrHgoc8Pk3KUk9/4AKaR0xWeAwci1oIfSdt3ZgHueULOO5eq9qyJ0RKO/ 27GA== 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 Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id m4-20020a62f204000000b00593a808158esi32847187pfh.156.2023.03.30.05.37.17; Thu, 30 Mar 2023 05:37:30 -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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231599AbjC3MVf (ORCPT <rfc822;rua109.linux@gmail.com> + 99 others); Thu, 30 Mar 2023 08:21:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49704 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231477AbjC3MVa (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 30 Mar 2023 08:21:30 -0400 Received: from dggsgout12.his.huawei.com (dggsgout12.his.huawei.com [45.249.212.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A16A3868D; Thu, 30 Mar 2023 05:21:28 -0700 (PDT) Received: from mail02.huawei.com (unknown [172.30.67.143]) by dggsgout12.his.huawei.com (SkyGuard) with ESMTP id 4PnMwM4mRGz4f3r2G; Thu, 30 Mar 2023 20:21:23 +0800 (CST) Received: from huaweicloud.com (unknown [10.175.104.67]) by APP4 (Coremail) with SMTP id gCh0CgD3rLC5fiVkFoZ2GQ--.46838S7; Thu, 30 Mar 2023 20:21:25 +0800 (CST) From: Yu Kuai <yukuai1@huaweicloud.com> To: logang@deltatee.com, song@kernel.org Cc: linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, yukuai3@huawei.com, yukuai1@huaweicloud.com, yi.zhang@huawei.com, yangerkun@huawei.com Subject: [PATCH v3 3/3] md: protect md_thread with rcu Date: Fri, 31 Mar 2023 04:20:46 +0800 Message-Id: <20230330202046.795213-4-yukuai1@huaweicloud.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230330202046.795213-1-yukuai1@huaweicloud.com> References: <20230330202046.795213-1-yukuai1@huaweicloud.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: gCh0CgD3rLC5fiVkFoZ2GQ--.46838S7 X-Coremail-Antispam: 1UD129KBjvJXoWxXFyUZF4UXr1xurWkAw4kCrg_yoW5Cr4rp3 y3JFy3Ar48Ars8Zw4UG3WUA3WYqw1Iq3WUAry7Gw1fA3WUG3yaqry2kFy0qFn8Aa43AFs8 Jr15KayrZ3yDtw7anT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUBE14x267AKxVWrJVCq3wAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2jI8I6cxK62vIxIIY0VWUZVW8XwA2048vs2IY02 0E87I2jVAFwI0_JrWl82xGYIkIc2x26xkF7I0E14v26ryj6s0DM28lY4IEw2IIxxk0rwA2 F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Ar0_tr1l84ACjcxK6xIIjx v20xvEc7CjxVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwVC2 z280aVCY1x0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0V AKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1l Ox8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcxkI7VAKI48JM4x0x7Aq67IIx4CEVc8vx2IErc IFxwCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwC20s026c02F40E14v2 6r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E67AF67kF1VAFwI0_Jw0_GFylIxkGc2 Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCwCI42IY6xIIjxv20xvEc7CjxVAFwI0_ Gr0_Cr1lIxAIcVCF04k26cxKx2IYs7xG6r1j6r1xMIIF0xvEx4A2jsIE14v26r1j6r4UMI IF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYxBIdaVFxhVjvjDU0xZFpf9x0pRPEf5UUUUU = X-CM-SenderInfo: 51xn3trlr6x35dzhxuhorxvhhfrp/ X-CFilter-Loop: Reflected X-Spam-Status: No, score=0.0 required=5.0 tests=DATE_IN_FUTURE_06_12, SPF_HELO_NONE,SPF_NONE autolearn=unavailable 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761796266819429429?= X-GMAIL-MSGID: =?utf-8?q?1761796266819429429?= |
Series |
md: fix uaf for sync_thread
|
|
Commit Message
Yu Kuai
March 30, 2023, 8:20 p.m. UTC
From: Yu Kuai <yukuai3@huawei.com> Our test reports a uaf for 'mddev->sync_thread': T1 T2 md_start_sync md_register_thread // mddev->sync_thread is set raid1d md_check_recovery md_reap_sync_thread md_unregister_thread kfree md_wakeup_thread wake_up ->sync_thread was freed Root cause is that there is a small windown between register thread and wake up thread, where the thread can be freed concurrently. Currently, a global spinlock 'pers_lock' is borrowed to protect 'mddev->thread', this problem can be fixed likewise, however, there might be similar problem for other md_thread, and I really don't like the idea to borrow a global lock. This patch protect md_thread with rcu. Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- drivers/md/md.c | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)
Comments
Hi Yu, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on song-md/md-next] [also build test WARNING on device-mapper-dm/for-next linus/master v6.3-rc4 next-20230330] [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/Yu-Kuai/md-pass-a-md_thread-pointer-to-md_register_thread/20230330-202251 base: git://git.kernel.org/pub/scm/linux/kernel/git/song/md.git md-next patch link: https://lore.kernel.org/r/20230330202046.795213-4-yukuai1%40huaweicloud.com patch subject: [PATCH v3 3/3] md: protect md_thread with rcu config: x86_64-randconfig-s023 (https://download.01.org/0day-ci/archive/20230331/202303310252.5OJzxk7h-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/b6edd339e54dc14576816f285b99e0e1815ed1e0 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Yu-Kuai/md-pass-a-md_thread-pointer-to-md_register_thread/20230330-202251 git checkout b6edd339e54dc14576816f285b99e0e1815ed1e0 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/md/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303310252.5OJzxk7h-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/md/md.c:7905:18: sparse: sparse: incompatible types in comparison expression (different address spaces): >> drivers/md/md.c:7905:18: sparse: struct md_thread [noderef] __rcu * >> drivers/md/md.c:7905:18: sparse: struct md_thread * drivers/md/md.c:7939:9: sparse: sparse: incompatible types in comparison expression (different address spaces): drivers/md/md.c:7939:9: sparse: struct md_thread [noderef] __rcu * drivers/md/md.c:7939:9: sparse: struct md_thread * drivers/md/md.c:7952:9: sparse: sparse: incompatible types in comparison expression (different address spaces): drivers/md/md.c:7952:9: sparse: struct md_thread [noderef] __rcu * drivers/md/md.c:7952:9: sparse: struct md_thread * vim +7905 drivers/md/md.c 7899 7900 void md_wakeup_thread(struct md_thread **threadp) 7901 { 7902 struct md_thread *thread; 7903 7904 rcu_read_lock(); > 7905 thread = rcu_dereference(*threadp); 7906 if (thread) { 7907 pr_debug("md: waking up MD thread %s.\n", thread->tsk->comm); 7908 set_bit(THREAD_WAKEUP, &thread->flags); 7909 wake_up(&thread->wqueue); 7910 } 7911 rcu_read_unlock(); 7912 } 7913 EXPORT_SYMBOL(md_wakeup_thread); 7914
On 2023-03-30 14:20, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > Our test reports a uaf for 'mddev->sync_thread': > > T1 T2 > md_start_sync > md_register_thread > // mddev->sync_thread is set > raid1d > md_check_recovery > md_reap_sync_thread > md_unregister_thread > kfree > > md_wakeup_thread > wake_up > ->sync_thread was freed > > Root cause is that there is a small windown between register thread and > wake up thread, where the thread can be freed concurrently. > > Currently, a global spinlock 'pers_lock' is borrowed to protect > 'mddev->thread', this problem can be fixed likewise, however, there might > be similar problem for other md_thread, and I really don't like the idea to > borrow a global lock. > > This patch protect md_thread with rcu. > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/md/md.c | 32 ++++++++++---------------------- > 1 file changed, 10 insertions(+), 22 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 9e80c5491c9a..161231e01faa 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -70,11 +70,7 @@ > #include "md-bitmap.h" > #include "md-cluster.h" > > -/* pers_list is a list of registered personalities protected > - * by pers_lock. > - * pers_lock does extra service to protect accesses to > - * mddev->thread when the mutex cannot be held. > - */ > +/* pers_list is a list of registered personalities protected by pers_lock. */ > static LIST_HEAD(pers_list); > static DEFINE_SPINLOCK(pers_lock); > > @@ -802,13 +798,8 @@ void mddev_unlock(struct mddev *mddev) > } else > mutex_unlock(&mddev->reconfig_mutex); > > - /* As we've dropped the mutex we need a spinlock to > - * make sure the thread doesn't disappear > - */ > - spin_lock(&pers_lock); > md_wakeup_thread(&mddev->thread); > wake_up(&mddev->sb_wait); > - spin_unlock(&pers_lock); > } > EXPORT_SYMBOL_GPL(mddev_unlock); > > @@ -7921,13 +7912,16 @@ static int md_thread(void *arg) > > void md_wakeup_thread(struct md_thread **threadp) > { > - struct md_thread *thread = *threadp; > + struct md_thread *thread; > > + rcu_read_lock(); > + thread = rcu_dereference(*threadp); A couple points: I don't think we need a double pointer here. rcu_dereference() doesn't actually do anything but annotate the fact that we are accessing a pointer protected by rcu. It does require annotations on that pointer (__rcu) which is checked by sparse (I suspect this patch will produce a lot of sparse errors from kbuild bot). I think all we need is: void md_wakeup_thread(struct md_thread __rcu *rthread) { struct md_thread *thread; rcu_read_lock(); thread = rcu_dereference(rthread); ... rcu_read_unlock(); } The __rcu annotation will have to be added to all the pointers this function is called on as well as to md_register_thread() and md_unregister_thread(). And anything else that uses those pointers. Running sparse on the code and eliminating all new errors for the patch is important. Thanks, Logan
Hi, Logan! 在 2023/03/31 3:35, Logan Gunthorpe 写道: > > A couple points: > > I don't think we need a double pointer here. rcu_dereference() doesn't > actually do anything but annotate the fact that we are accessing a > pointer protected by rcu. It does require annotations on that pointer > (__rcu) which is checked by sparse (I suspect this patch will produce a > lot of sparse errors from kbuild bot). > > I think all we need is: > > void md_wakeup_thread(struct md_thread __rcu *rthread) > { > struct md_thread *thread; > > rcu_read_lock(); > thread = rcu_dereference(rthread); > ... > rcu_read_unlock(); > > } > > The __rcu annotation will have to be added to all the pointers this > function is called on as well as to md_register_thread() and > md_unregister_thread(). And anything else that uses those pointers. > Running sparse on the code and eliminating all new errors for the patch > is important. Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try to run sparse before sending the new version. Thanks, Kuai
Hi, 在 2023/03/31 9:08, Yu Kuai 写道: > Hi, Logan! > > 在 2023/03/31 3:35, Logan Gunthorpe 写道: >> >> A couple points: >> >> I don't think we need a double pointer here. rcu_dereference() doesn't >> actually do anything but annotate the fact that we are accessing a >> pointer protected by rcu. It does require annotations on that pointer >> (__rcu) which is checked by sparse (I suspect this patch will produce a >> lot of sparse errors from kbuild bot). >> >> I think all we need is: >> >> void md_wakeup_thread(struct md_thread __rcu *rthread) >> { >> struct md_thread *thread; >> >> rcu_read_lock(); >> thread = rcu_dereference(rthread); >> ... >> rcu_read_unlock(); >> >> } >> >> The __rcu annotation will have to be added to all the pointers this >> function is called on as well as to md_register_thread() and >> md_unregister_thread(). And anything else that uses those pointers. >> Running sparse on the code and eliminating all new errors for the patch >> is important. > > Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try > to run sparse before sending the new version. > By the way, I observed lots of sparse errors and warnings for current code, will it make sense to fix them? Thanks, Kuai
On 2023-03-31 00:36, Yu Kuai wrote: >> Yes, you're right, I'll remove patch 2 and update patch 3. And I'll try >> to run sparse before sending the new version. >> > > By the way, I observed lots of sparse errors and warnings for current > code, will it make sense to fix them? Yup, I fixed a bunch for raid5 last year. There was one I could not fix though. I would say effort spent fixing those is well spent. Thanks! Logan
diff --git a/drivers/md/md.c b/drivers/md/md.c index 9e80c5491c9a..161231e01faa 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -70,11 +70,7 @@ #include "md-bitmap.h" #include "md-cluster.h" -/* pers_list is a list of registered personalities protected - * by pers_lock. - * pers_lock does extra service to protect accesses to - * mddev->thread when the mutex cannot be held. - */ +/* pers_list is a list of registered personalities protected by pers_lock. */ static LIST_HEAD(pers_list); static DEFINE_SPINLOCK(pers_lock); @@ -802,13 +798,8 @@ void mddev_unlock(struct mddev *mddev) } else mutex_unlock(&mddev->reconfig_mutex); - /* As we've dropped the mutex we need a spinlock to - * make sure the thread doesn't disappear - */ - spin_lock(&pers_lock); md_wakeup_thread(&mddev->thread); wake_up(&mddev->sb_wait); - spin_unlock(&pers_lock); } EXPORT_SYMBOL_GPL(mddev_unlock); @@ -7921,13 +7912,16 @@ static int md_thread(void *arg) void md_wakeup_thread(struct md_thread **threadp) { - struct md_thread *thread = *threadp; + struct md_thread *thread; + rcu_read_lock(); + thread = rcu_dereference(*threadp); if (thread) { pr_debug("md: waking up MD thread %s.\n", thread->tsk->comm); set_bit(THREAD_WAKEUP, &thread->flags); wake_up(&thread->wqueue); } + rcu_read_unlock(); } EXPORT_SYMBOL(md_wakeup_thread); @@ -7955,7 +7949,7 @@ int md_register_thread(struct md_thread **threadp, return err; } - *threadp = thread; + rcu_assign_pointer(*threadp, thread); return 0; } EXPORT_SYMBOL(md_register_thread); @@ -7964,18 +7958,12 @@ void md_unregister_thread(struct md_thread **threadp) { struct md_thread *thread; - /* - * Locking ensures that mddev_unlock does not wake_up a - * non-existent thread - */ - spin_lock(&pers_lock); thread = *threadp; - if (!thread) { - spin_unlock(&pers_lock); + if (!thread) return; - } - *threadp = NULL; - spin_unlock(&pers_lock); + + rcu_assign_pointer(*threadp, NULL); + synchronize_rcu(); pr_debug("interrupting MD-thread pid %d\n", task_pid_nr(thread->tsk)); kthread_stop(thread->tsk);