| Message ID | 20200402000002.7442-1-mcgrof@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | block: address blktrace use-after-free | expand |
On Wed, Apr 01, 2020 at 11:59:59PM +0000, Luis Chamberlain wrote: > Upstream kernel.org korg#205713 contends that there is a UAF in > the core debugfs debugfs_remove() function, and has gone through > pushing for a CVE for this, CVE-2019-19770. As you point out, that CVE is crazy and pointless, thanks for seeing this through. greg k-h
On Wed, Apr 01, 2020 at 11:59:59PM +0000, Luis Chamberlain wrote: > Upstream kernel.org korg#205713 contends that there is a UAF in > the core debugfs debugfs_remove() function, and has gone through > pushing for a CVE for this, CVE-2019-19770. > > If correct then parent dentries are not positive, and this would > have implications far beyond this bug report. Thankfully, upon review > with Nicolai, he wasn't buying it. His suspicions that this was just > a blktrace issue were spot on, and this patch series demonstrates > that, provides a reproducer, and provides a solution to the issue. > > We there would like to contend CVE-2019-19770 as invalid. The > implications suggested are not correct, and this issue is only > triggerable with root, by shooting yourself on the foot by misuing > blktrace. > > If you want this on a git tree, you can get it from linux-next > 20200401-blktrace-fix-uaf branch [2]. > > Wider review, testing, and rants are appreciated. > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=205713 > [1] https://nvd.nist.gov/vuln/detail/CVE-2019-19770 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200401-blktrace-fix-uaf > > Luis Chamberlain (3): > block: move main block debugfs initialization to its own file > blktrace: fix debugfs use after free > block: avoid deferral of blk_release_queue() work > > block/Makefile | 1 + > block/blk-core.c | 9 +-------- > block/blk-debugfs.c | 27 +++++++++++++++++++++++++++ > block/blk-mq-debugfs.c | 5 ----- > block/blk-sysfs.c | 21 ++++++++------------- > block/blk.h | 17 +++++++++++++++++ > include/linux/blktrace_api.h | 1 - > kernel/trace/blktrace.c | 19 ++++++++----------- > 8 files changed, 62 insertions(+), 38 deletions(-) > create mode 100644 block/blk-debugfs.c BTW, Yu Kuai posted one patch for this issue, looks that approach is simpler: https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ Thanks, Ming
On Fri, Apr 03, 2020 at 04:19:29PM +0800, Ming Lei wrote: > On Wed, Apr 01, 2020 at 11:59:59PM +0000, Luis Chamberlain wrote: > > Upstream kernel.org korg#205713 contends that there is a UAF in > > the core debugfs debugfs_remove() function, and has gone through > > pushing for a CVE for this, CVE-2019-19770. > > > > If correct then parent dentries are not positive, and this would > > have implications far beyond this bug report. Thankfully, upon review > > with Nicolai, he wasn't buying it. His suspicions that this was just > > a blktrace issue were spot on, and this patch series demonstrates > > that, provides a reproducer, and provides a solution to the issue. > > > > We there would like to contend CVE-2019-19770 as invalid. The > > implications suggested are not correct, and this issue is only > > triggerable with root, by shooting yourself on the foot by misuing > > blktrace. > > > > If you want this on a git tree, you can get it from linux-next > > 20200401-blktrace-fix-uaf branch [2]. > > > > Wider review, testing, and rants are appreciated. > > > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=205713 > > [1] https://nvd.nist.gov/vuln/detail/CVE-2019-19770 > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200401-blktrace-fix-uaf > > > > Luis Chamberlain (3): > > block: move main block debugfs initialization to its own file > > blktrace: fix debugfs use after free > > block: avoid deferral of blk_release_queue() work > > > > block/Makefile | 1 + > > block/blk-core.c | 9 +-------- > > block/blk-debugfs.c | 27 +++++++++++++++++++++++++++ > > block/blk-mq-debugfs.c | 5 ----- > > block/blk-sysfs.c | 21 ++++++++------------- > > block/blk.h | 17 +++++++++++++++++ > > include/linux/blktrace_api.h | 1 - > > kernel/trace/blktrace.c | 19 ++++++++----------- > > 8 files changed, 62 insertions(+), 38 deletions(-) > > create mode 100644 block/blk-debugfs.c > > BTW, Yu Kuai posted one patch for this issue, looks that approach > is simpler: > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ I cannot see how renaming the possible target directory to a temporary directory instead of unifying it for both SQ and MQ could be any simpler. IMHO this keeps the mess and fragile nature of the issue. The approach taken here unifies the directory we should use for both SQ and MQ and makes the deferral issue a completely separate issue addressed in the last patch. Luis
On 2020-04-03 01:19, Ming Lei wrote: > BTW, Yu Kuai posted one patch for this issue, looks that approach > is simpler: > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ That approach is indeed simpler but at the expense of performing dynamic memory allocation in a cleanup path, something I'm not enthusiast about. Bart.
On Fri, Apr 03, 2020 at 07:13:47AM -0700, Bart Van Assche wrote: > On 2020-04-03 01:19, Ming Lei wrote: > > BTW, Yu Kuai posted one patch for this issue, looks that approach > > is simpler: > > > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ > > That approach is indeed simpler but at the expense of performing dynamic > memory allocation in a cleanup path, something I'm not enthusiast about. I also think that its important to annotate that there are actually two issues here. Not one. One is the misuse of debugfs, the other is how the deferral exposed the misuse and complications of its misuse. Luis
On 2020/4/3 16:19, Ming Lei wrote: > BTW, Yu Kuai posted one patch for this issue, looks that approach > is simpler: > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ > > I think the issue might not be fixed with the patch seires. At first, I think there are two key points for the issure: 1. The final release of queue is delayed in a workqueue 2. The creation of 'q->debugfs_dir' might failed(only if 1 exist) And if we can fix any of the above problem, the UAF issue will be fixed. (BTW, I did not come up with a good idea for problem 1, and my approach is for problem 2.) The third patch "block: avoid deferral of blk_release_queue() work" is not enough to fix problem 1: a. if CONFIG_DEBUG_KOBJECT_RELEASE is enable: static void kobject_release(struct kref *kref) { struct kobject *kobj = container_of(kref, struct kobject, kref); #ifdef CONFIG_DEBUG_KOBJECT_RELEASE unsigned long delay = HZ + HZ * (get_random_int() & 0x3); pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n", ┊kobject_name(kobj), kobj, __func__, kobj->parent, delay); INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup); schedule_delayed_work(&kobj->release, delay); #else kobject_cleanup(kobj); #endif } b. when 'kobject_put' is called from blk_cleanup_queue, can we make sure it is the last reference? Futhermore, I do understand the second patch fix the UAF problem by using 'q->debugfs_dir' instead of 'q->blk_trace->dir', but the problem 2 still exist and need to be fixed. Thanks! Yu Kuai
On Tue, Apr 07, 2020 at 10:47:01AM +0800, yukuai (C) wrote: > On 2020/4/3 16:19, Ming Lei wrote: > > > BTW, Yu Kuai posted one patch for this issue, looks that approach > > is simpler: > > > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ > > > > > > I think the issue might not be fixed with the patch seires. > > At first, I think there are two key points for the issure: > 1. The final release of queue is delayed in a workqueue > 2. The creation of 'q->debugfs_dir' might failed(only if 1 exist) > And if we can fix any of the above problem, the UAF issue will be fixed. > (BTW, I did not come up with a good idea for problem 1, and my approach > is for problem 2.) > > The third patch "block: avoid deferral of blk_release_queue() work" is > not enough to fix problem 1: > a. if CONFIG_DEBUG_KOBJECT_RELEASE is enable: > static void kobject_release(struct kref *kref) > { > struct kobject *kobj = container_of(kref, struct kobject, kref); > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE > unsigned long delay = HZ + HZ * (get_random_int() & 0x3); > pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n", > ┊kobject_name(kobj), kobj, __func__, kobj->parent, delay); > INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup); > > schedule_delayed_work(&kobj->release, delay); > #else > kobject_cleanup(kobj); > #endif > } > b. when 'kobject_put' is called from blk_cleanup_queue, can we make sure > it is the last reference? You are right, I think I know the fix for this now. Will run some more tests. Luis
On Tue, Apr 7, 2020 at 1:00 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Tue, Apr 07, 2020 at 10:47:01AM +0800, yukuai (C) wrote: > > On 2020/4/3 16:19, Ming Lei wrote: > > > > > BTW, Yu Kuai posted one patch for this issue, looks that approach > > > is simpler: > > > > > > https://lore.kernel.org/linux-block/20200324132315.22133-1-yukuai3@huawei.com/ > > > > > > > > > > I think the issue might not be fixed with the patch seires. > > > > At first, I think there are two key points for the issure: > > 1. The final release of queue is delayed in a workqueue > > 2. The creation of 'q->debugfs_dir' might failed(only if 1 exist) > > And if we can fix any of the above problem, the UAF issue will be fixed. > > (BTW, I did not come up with a good idea for problem 1, and my approach > > is for problem 2.) > > > > The third patch "block: avoid deferral of blk_release_queue() work" is > > not enough to fix problem 1: > > a. if CONFIG_DEBUG_KOBJECT_RELEASE is enable: > > static void kobject_release(struct kref *kref) > > { > > struct kobject *kobj = container_of(kref, struct kobject, kref); > > #ifdef CONFIG_DEBUG_KOBJECT_RELEASE > > unsigned long delay = HZ + HZ * (get_random_int() & 0x3); > > pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n", > > ┊kobject_name(kobj), kobj, __func__, kobj->parent, delay); > > INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup); > > > > schedule_delayed_work(&kobj->release, delay); > > #else > > kobject_cleanup(kobj); > > #endif > > } > > b. when 'kobject_put' is called from blk_cleanup_queue, can we make sure > > it is the last reference? > > You are right, I think I know the fix for this now. Will run some more > tests. Yeap, we were just not refcounting during blktrace. I'll send a fix as part of the series. Luis