Message ID | 20230727145758.3880967-1-alexander.usyskin@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a985:0:b0:3e4:2afc:c1 with SMTP id t5csp1180904vqo; Thu, 27 Jul 2023 08:30:37 -0700 (PDT) X-Google-Smtp-Source: APBJJlEezfRJthYQJgazdK+qMDs2YWgoVBNBiErVoD4bfFm24fqtBYjx6oWBfUtjPGzwbp55w04N X-Received: by 2002:a05:6a20:3ca7:b0:134:8d7f:f4d9 with SMTP id b39-20020a056a203ca700b001348d7ff4d9mr7083731pzj.52.1690471837370; Thu, 27 Jul 2023 08:30:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690471837; cv=none; d=google.com; s=arc-20160816; b=vEI5AvvhjI63U76//DyG3mUNI5boPQZq1xntkxy7vLeWL2SOTxNjuBUQ9fG1D/AM5L OOtgZ5ewiAHEtdGJqc9CR/6uQKz6joddn4bfUfnvtZrM+OWF19YOBdeQ0yVNMjiqmftc x+sL193jB5VwUMtlYYUPuvKjwO7WfaYmqhntB2gDw1BuzJPD0dSd8vcsCCh7hVBqxyXH 8CBAcK6EgCAc3Sf68Q/l+BLtaJSATqM5zRZN1g7oT8qu5BYRP49uoUmFs1+cqN+A18DT 59NVK66sZBwFyXDisD5cJgDB2YOI0RUaJSPrD3fEAWc6FBhmOo8laXsOiKmK9iJmZi4C cetw== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=flLfnzCQiKfGyshH6D95jxjEBoZRhyOxmas2WDkZZkA=; fh=knNrmogx3g7asAA+VjHKlHhzYFCqpGoAqEXi6Cn+liw=; b=loNS5pi5hN9toHKh5TYytXl7t/KjAOjhnrkjxBCFIo027gGXKA+1MrvI+bVyWUHfvA wKZML4iy2UZTCuqJQwU38XBZS/vDweoWsKfjlLqJ5rVVVC/Wa3PcwpnHVM+UHxF4UJuG 5WO40jMkaneJmnLn3QLp0ZkNbJqxkD9mvESH2zHj+C8SUa04zQkftf6ClpTueg+ou0pS X5YzfFJkc84WFS+fbnh1pcXDMl5ctlj5goJ9l8cjcJanHNKOHgzAxG+sslH+MTb7M0ft N7WGw9zn2WzMFLZP6l4FXWNTwdSoEmJGISgtoIDif60RGkMsZWlX+FlDTqpxzyISAuIe /HOQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=IDcL46EZ; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u8-20020a631408000000b00547b25ea099si1341991pgl.682.2023.07.27.08.30.23; Thu, 27 Jul 2023 08:30:37 -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; dkim=pass header.i=@intel.com header.s=Intel header.b=IDcL46EZ; 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=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234146AbjG0PCe (ORCPT <rfc822;hanasaki@gmail.com> + 99 others); Thu, 27 Jul 2023 11:02:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47514 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234111AbjG0PCX (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 27 Jul 2023 11:02:23 -0400 Received: from mgamail.intel.com (unknown [134.134.136.126]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D89842D54 for <linux-kernel@vger.kernel.org>; Thu, 27 Jul 2023 08:02:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1690470142; x=1722006142; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=cZOumbvz7d5c3xoly7L/DH5QKt5aCye/K29l+UqNRao=; b=IDcL46EZcbxxpsFyvRDWR1kjVMBeNqVyv0IIRyLVyur1Ei4DAFnUUNeA QFq4MMoQbYMJuxU+ZhoVQQKj1M3wNO66in4gpkHjuaR1z0XD2KFPG1Qif Aw+CHTktU3wAlU8zBPdksMCTaW6mIswABGD8lTAn0/vwFwwEFmhJ62pfs AOTkA7fliBjSa/Nvjp6igcqVm74XS1Anzf+GOQZAOdSBklWWKUBiVYCmg faJinEReqEzbz5350M49h/oWnMosfe2FqRESkcTKYjF49/jJfqsfQhOAd KFOPMrolVbAyIHVIfXmGcyf1LnS9ffbcO1hjb0HDOO3K7yzcnhXtkmj73 w==; X-IronPort-AV: E=McAfee;i="6600,9927,10784"; a="353246758" X-IronPort-AV: E=Sophos;i="6.01,235,1684825200"; d="scan'208";a="353246758" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jul 2023 08:01:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10784"; a="792404744" X-IronPort-AV: E=Sophos;i="6.01,235,1684825200"; d="scan'208";a="792404744" Received: from sannilnx-dsk.jer.intel.com ([10.12.231.107]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Jul 2023 08:01:39 -0700 From: Alexander Usyskin <alexander.usyskin@intel.com> To: Miquel Raynal <miquel.raynal@bootlin.com>, Richard Weinberger <richard@nod.at>, Vignesh Raghavendra <vigneshr@ti.com>, linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Tomas Winkler <tomas.winkler@intel.com>, Alexander Usyskin <alexander.usyskin@intel.com>, Vitaly Lubart <vitaly.lubart@intel.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Zhang Xiaoxu <zhangxiaoxu5@huawei.com> Subject: [PATCH] mtd: fix use-after-free in mtd release Date: Thu, 27 Jul 2023 17:57:58 +0300 Message-Id: <20230727145758.3880967-1-alexander.usyskin@intel.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE, SPF_NONE,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: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1772588197372792982 X-GMAIL-MSGID: 1772588197372792982 |
Series |
mtd: fix use-after-free in mtd release
|
|
Commit Message
Usyskin, Alexander
July 27, 2023, 2:57 p.m. UTC
I case of partition device_unregister in mtd_device_release calls mtd_release which frees mtd_info structure for partition. All code after device_unregister in mtd_device_release thus works already freed memory. Move part of code to mtd_release and restict mtd->dev cleanup to non-partion object. For partition object such cleanup have no sense as partition mtd_info is removed. Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") Reviewed-by: Tomas Winkler <tomas.winkler@intel.com> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> --- drivers/mtd/mtdcore.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
Comments
On Thu, Jul 27, 2023 at 05:57:58PM +0300, Alexander Usyskin wrote: > I case of partition device_unregister in mtd_device_release In device_unregister() mtd_device_release() > calls mtd_release which frees mtd_info structure for partition. mtd_release() > All code after device_unregister in mtd_device_release thus device_unregister() mtd_device_release() > works already freed memory. uses? > Move part of code to mtd_release and restict mtd->dev cleanup mtd_release() > to non-partion object. > For partition object such cleanup have no sense as partition > mtd_info is removed. > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") Closes: ?
Hi Andy, andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:12:04 +0300: > On Thu, Jul 27, 2023 at 05:57:58PM +0300, Alexander Usyskin wrote: > > I case of partition device_unregister in mtd_device_release > > In > > device_unregister() > mtd_device_release() > > > calls mtd_release which frees mtd_info structure for partition. > > mtd_release() > > > All code after device_unregister in mtd_device_release thus > > device_unregister() > mtd_device_release() > > > works already freed memory. > > uses? > > > Move part of code to mtd_release and restict mtd->dev cleanup > > mtd_release() Yup, thanks for all these suggestions, I agree with them. > > to non-partion object. > > For partition object such cleanup have no sense as partition > > mtd_info is removed. > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > > Closes: ? Did I miss a recent update on the use of Fixes? I thought Closes was supposed to point at a bug report while Fixes would point to the faulty commit. Right now I feel like Fixes is the right tag, but if you have a source explaining why we should not longer do it like I am used to, I would appreciate a link. Thanks, Miquèl
On Thu, Jul 27, 2023 at 05:20:13PM +0200, Miquel Raynal wrote: > andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:12:04 > +0300: > > On Thu, Jul 27, 2023 at 05:57:58PM +0300, Alexander Usyskin wrote: ... > > > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > > > > Closes: ? > > Did I miss a recent update on the use of Fixes? They are orthogonal to each other. Actually Closes goes closer with Reported-by. I believe both of them needs to be added (by I might miss something). > I thought Closes was > supposed to point at a bug report while Fixes would point to the faulty > commit. Correct. > Right now I feel like Fixes is the right tag, Nobody objects that (see above). > but if you have a source explaining why we should not longer do it like > I am used to, I would appreciate a link. Since you know about Closes already, I think there is nothing to add.
Hi Andy, andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:58:29 +0300: > On Thu, Jul 27, 2023 at 05:20:13PM +0200, Miquel Raynal wrote: > > andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:12:04 > > +0300: > > > On Thu, Jul 27, 2023 at 05:57:58PM +0300, Alexander Usyskin wrote: > > ... > > > > > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > > > > > > Closes: ? > > > > Did I miss a recent update on the use of Fixes? > > They are orthogonal to each other. Actually Closes goes closer with > Reported-by. > > I believe both of them needs to be added (by I might miss something). > > > I thought Closes was > > supposed to point at a bug report while Fixes would point to the faulty > > commit. > > Correct. > > > Right now I feel like Fixes is the right tag, > > Nobody objects that (see above). > > > but if you have a source explaining why we should not longer do it like > > I am used to, I would appreciate a link. > > Since you know about Closes already, I think there is nothing to add. Ah sorry I misunderstood your first e-mail. I thought you were suggesting to replace Fixes by Closes. Sorry for the misunderstanding :) Thanks, Miquèl
> > Hi Andy, > > andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:58:29 > +0300: > > > On Thu, Jul 27, 2023 at 05:20:13PM +0200, Miquel Raynal wrote: > > > andriy.shevchenko@linux.intel.com wrote on Thu, 27 Jul 2023 18:12:04 > > > +0300: > > > > On Thu, Jul 27, 2023 at 05:57:58PM +0300, Alexander Usyskin wrote: > > > > ... > > > > > > > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > > > > > > > > Closes: ? > > > > > > Did I miss a recent update on the use of Fixes? > > > > They are orthogonal to each other. Actually Closes goes closer with > > Reported-by. > > > > I believe both of them needs to be added (by I might miss something). > > > > > I thought Closes was > > > supposed to point at a bug report while Fixes would point to the faulty > > > commit. > > > > Correct. > > > > > Right now I feel like Fixes is the right tag, > > > > Nobody objects that (see above). > > > > > but if you have a source explaining why we should not longer do it like > > > I am used to, I would appreciate a link. > > > > Since you know about Closes already, I think there is nothing to add. > > Ah sorry I misunderstood your first e-mail. I thought you were > suggesting to replace Fixes by Closes. Sorry for the misunderstanding :) > > Thanks, > Miquèl Miquel, is this patch helps with your original problem of devices not freed? Zhang, is this patch helps with your problem with KAsan? -- Thanks, Sasha
在 2023/7/30 19:10, Usyskin, Alexander 写道: > Miquel, is this patch helps with your original problem of devices not freed? > > Zhang, is this patch helps with your problem with KAsan? After this patch applied, the problem can still be reproduced.
Hi zhang, zhangxiaoxu5@huawei.com wrote on Mon, 31 Jul 2023 09:35:42 +0800: > 在 2023/7/30 19:10, Usyskin, Alexander 写道: > > Miquel, is this patch helps with your original problem of devices not freed? > > > > Zhang, is this patch helps with your problem with KAsan? > After this patch applied, the problem can still be reproduced. Did you test my patch as well? Does Kasan still complain with it? Thanks, Miquèl
在 2023/8/2 20:44, Miquel Raynal 写道: > Hi zhang, > > zhangxiaoxu5@huawei.com wrote on Mon, 31 Jul 2023 09:35:42 +0800: > >> 在 2023/7/30 19:10, Usyskin, Alexander 写道: >>> Miquel, is this patch helps with your original problem of devices not freed? >>> >>> Zhang, is this patch helps with your problem with KAsan? >> After this patch applied, the problem can still be reproduced. > > Did you test my patch as well? Does Kasan still complain with it? After this patch applied, the problem can still be reproduced. > > Thanks, > Miquèl
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 2466ea466466..46f15f676491 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -93,6 +93,9 @@ static void mtd_release(struct device *dev) struct mtd_info *mtd = dev_get_drvdata(dev); dev_t index = MTD_DEVT(mtd->index); + idr_remove(&mtd_idr, mtd->index); + of_node_put(mtd_get_of_node(mtd)); + if (mtd_is_partition(mtd)) release_mtd_partition(mtd); @@ -103,6 +106,7 @@ static void mtd_release(struct device *dev) static void mtd_device_release(struct kref *kref) { struct mtd_info *mtd = container_of(kref, struct mtd_info, refcnt); + bool is_partition = mtd_is_partition(mtd); debugfs_remove_recursive(mtd->dbg.dfs_dir); @@ -111,11 +115,13 @@ static void mtd_device_release(struct kref *kref) device_unregister(&mtd->dev); - /* Clear dev so mtd can be safely re-registered later if desired */ - memset(&mtd->dev, 0, sizeof(mtd->dev)); - - idr_remove(&mtd_idr, mtd->index); - of_node_put(mtd_get_of_node(mtd)); + /* + * Clear dev so mtd can be safely re-registered later if desired. + * Should not be done for partition, + * as it was already destroyed in device_unregister(). + */ + if (!is_partition) + memset(&mtd->dev, 0, sizeof(mtd->dev)); module_put(THIS_MODULE); }