| Message ID | 20230727145758.3880967-1-alexander.usyskin@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: fix use-after-free in mtd release | expand |
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?
在 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); }
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(-)