[PATCH] mtd: fix use-after-free in mtd release

[PATCH] mtd: fix use-after-free in mtd release

[Alexander Usyskin]

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(-)

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);
 }
-- 
2.34.1

Re: [PATCH] mtd: fix use-after-free in mtd release

[Andy Shevchenko]

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: ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mtd: fix use-after-free in mtd release

[Miquel Raynal]

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

Re: [PATCH] mtd: fix use-after-free in mtd release

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] mtd: fix use-after-free in mtd release

[Miquel Raynal]

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

RE: [PATCH] mtd: fix use-after-free in mtd release

[Usyskin, Alexander]

> 
> 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

Re: [PATCH] mtd: fix use-after-free in mtd release

[zhangxiaoxu (A)]

在 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.

Re: [PATCH] mtd: fix use-after-free in mtd release

[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?

Thanks,
Miquèl

Re: [PATCH] mtd: fix use-after-free in mtd release

[huaweicloud]

在 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