| Message ID | 20200323132254.47157-5-yuyufen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bdi: fix use-after-free for bdi device | expand |
On Mon, Mar 23, 2020 at 09:22:54PM +0800, Yufen Yu wrote: > diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h > index 82d2401fec37..1c0e2d0d6236 100644 > --- a/include/linux/backing-dev.h > +++ b/include/linux/backing-dev.h > @@ -525,12 +525,16 @@ static inline const char *bdi_dev_name(struct backing_dev_info *bdi) > static inline char *bdi_get_dev_name(struct backing_dev_info *bdi, > char *dname, int len) > { > + spin_lock_irq(&bdi->lock); > if (!bdi || !bdi->dev) { > + spin_unlock_irq(&bdi->lock); You can't test for (!bdi) right after you accessed bdi->lock :(
On 2020/3/23 22:04, Greg KH wrote: > On Mon, Mar 23, 2020 at 09:22:54PM +0800, Yufen Yu wrote: >> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h >> index 82d2401fec37..1c0e2d0d6236 100644 >> --- a/include/linux/backing-dev.h >> +++ b/include/linux/backing-dev.h >> @@ -525,12 +525,16 @@ static inline const char *bdi_dev_name(struct backing_dev_info *bdi) >> static inline char *bdi_get_dev_name(struct backing_dev_info *bdi, >> char *dname, int len) >> { >> + spin_lock_irq(&bdi->lock); >> if (!bdi || !bdi->dev) { >> + spin_unlock_irq(&bdi->lock); > > You can't test for (!bdi) right after you accessed bdi->lock :( Sorry for this coding error. Thanks, Yufen
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h index 4fc87dee005a..c98dac4a7953 100644 --- a/include/linux/backing-dev-defs.h +++ b/include/linux/backing-dev-defs.h @@ -227,6 +227,7 @@ struct backing_dev_info { #ifdef CONFIG_DEBUG_FS struct dentry *debug_dir; #endif + spinlock_t lock; /* protects dev */ }; enum { diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h index 82d2401fec37..1c0e2d0d6236 100644 --- a/include/linux/backing-dev.h +++ b/include/linux/backing-dev.h @@ -525,12 +525,16 @@ static inline const char *bdi_dev_name(struct backing_dev_info *bdi) static inline char *bdi_get_dev_name(struct backing_dev_info *bdi, char *dname, int len) { + spin_lock_irq(&bdi->lock); if (!bdi || !bdi->dev) { + spin_unlock_irq(&bdi->lock); strlcpy(dname, bdi_unknown_name, len); return NULL; } strlcpy(dname, dev_name(bdi->dev), len); + spin_unlock_irq(&bdi->lock); + return dname; } #endif /* _LINUX_BACKING_DEV_H */ diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 62f05f605fb5..aa9ba7dcc2d9 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -859,6 +859,7 @@ static int bdi_init(struct backing_dev_info *bdi) INIT_LIST_HEAD(&bdi->bdi_list); INIT_LIST_HEAD(&bdi->wb_list); init_waitqueue_head(&bdi->wb_waitq); + spin_lock_init(&bdi->lock); ret = cgwb_bdi_init(bdi); @@ -1007,15 +1008,19 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi) void bdi_unregister(struct backing_dev_info *bdi) { + struct device *dev = bdi->dev; + /* make sure nobody finds us on the bdi_list anymore */ bdi_remove_from_list(bdi); wb_shutdown(&bdi->wb); cgwb_bdi_unregister(bdi); - if (bdi->dev) { + if (dev) { bdi_debug_unregister(bdi); - device_unregister(bdi->dev); + spin_lock_irq(&bdi->lock); bdi->dev = NULL; + spin_unlock_irq(&bdi->lock); + device_unregister(dev); } if (bdi->owner) {
We reported a kernel crash in __blkg_prfill_rwstat() for use-after-free the bdi->dev. It tries to get the device name by 'bdi->dev', while the device and kobj->name has been freed by bdi_unregister(). Then, blkg_dev_name() will return an invalid name pointer, resulting in crash on string(). The race as following: CPU1 CPU2 blkg_print_stat_bytes __scsi_remove_device del_gendisk bdi_unregister put_device(bdi->dev) kfree(bdi->dev) __blkg_prfill_rwstat blkg_dev_name //use the freed bdi->dev dev_name(blkg->q->backing_dev_info->dev) bdi->dev = NULL To fix the bug, we add a new spinlock for bdi to protect the device. In addition, to fix crash in wb_workfn when bdi_unreigster(), commit 68f23b89067f ("memcg: fix a crash in wb_workfn when a device disappears") has created bdi_dev_name() to handle bdi->dev beening NULL. But, bdi->dev can be freed after bdi_dev_name() return successly, which may cause use-after-free for dev or kobj->name. After replacing bdi_dev_name() with bdi_get_dev_name() and protect bdi->dev by spinlock, we can fix the bug thoroughly. Signed-off-by: Yufen Yu <yuyufen@huawei.com> --- include/linux/backing-dev-defs.h | 1 + include/linux/backing-dev.h | 4 ++++ mm/backing-dev.c | 9 +++++++-- 3 files changed, 12 insertions(+), 2 deletions(-)