Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
To: axboe@kernel.dk, viro@zeniv.linux.org.uk, bvanassche@acm.org,
	gregkh@linuxfoundation.org, rostedt@goodmis.org,
	mingo@redhat.com, jack@suse.cz, ming.lei@redhat.com,
	nstange@suse.de, akpm@linux-foundation.org
Cc: mhocko@suse.com, yukuai3@huawei.com, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	Luis Chamberlain <mcgrof@kernel.org>
Subject: [PATCH v2 00/10] block: fix blktrace debugfs use after free
Date: Sun, 19 Apr 2020 19:45:19 +0000
Message-ID: <20200419194529.4872-1-mcgrof@kernel.org> (raw)

Upstream kernel.org korg#205713 [0] states 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 [1].

This patch series disputes that CVE, and shows how the issue was just
a complex misuse of debugfs within blktrace and fixes it.

On this v2 I've dropped two patches from my last series which are not
needed to ensure we can move back to a synchronous request_queue
removal. I've also addressed Ming's feedback on ensuring we keep
functionality working for when paritions are used for a blktrace.  That
effort lead me to ensuring we don't try to overwrite the request_queue
debugfs_dir, and add sanity checks in place so that what we give back
only what is expected.

Although my v1 patches also had fixed the kernel splat we get when we
try to reproduce the issue:

debugfs: Directory 'loop0' with parent 'block' already present!

This v2 series now provides a clear explanation for *why* this was
ultimately one of the reasons why we ended up with a crash.

The commit log for the actual fix, patch 3/10, "blktrace: fix debugfs
use after free" has also been extended to provide a better explanation
as to *how* overwriting the debugfs_dir leads to an eventual panic with
blktrace. I hope that helps, as it seems the root cause was still not
well explained in the commit log.

To make review easier, I've also added some helper functions with no
functional changes at first, and only extended them later.

Also changed is blk_queue_debugfs_register() to return an int, we do
this to not make the fact that we don't check for errors on
register_disk() or add_disk() any worse.

After the patch 3/10 "blktrace: fix debugfs use after free", is applied,
with the pr_warns(), and prior to reverting back to synchronous request_queue
removal, if we try to reproduce the issue with break-blktrace [2]'s
./run_0001.sh script, we'd see::

blk_debugfs: loop0 : registering request_queue debugfs directory twice is not allowed
blktrace: loop0: request_queue parent is gone

And sometimes only:

blk_debugfs: loop0 : registering request_queue debugfs directory twice is not allowed

After we revert back to synchronous request_queue removal this should no
longer be possible, and if it is, we want to hear about it. To help with
this two patches are added which change pr_warn() to BUG_ON()s after we flip
back to synchronous request_queue removal.

Note that on patch 6/10 "blk-debugfs: upgrade warns to BUG_ON() if
directory" I explain the syfs layout between a gendisk and the
request_queue. By reverting back to synchronous request_queue removal,
if someone manages to figure out a way to create a clash with
registering block devices, we expect to see a sysfs clash now instead of
a clash with debugfs, as the debugfs directory is removed now always
first, prior to clearing out the sysfs dir. *If* there are races
possible in these areas, we want to hear about them, and the BUG_ON()s
should make it clearer *where* the real issue is coming from.

Having an asynchronous request_queue removal has exposed other bugs
lingering around, however most importantly I think its revealing more
the value of adding error handling for __device_add_disk() and friends.
If its encouraged I could take a stab at finally addressing that for
good.

You can find this code on my git tree, on the 20200417-blktrace-fixes
branch, which is based on linux-next tag next-20200417 [3].

[0] https://bugzilla.kernel.org/show_bug.cgi?id=205713                          
[1] https://nvd.nist.gov/vuln/detail/CVE-2019-19770 
[2] https://github.com/mcgrof/break-blktrace
[3] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20200417-blktrace-fixes

Luis Chamberlain (10):
  block: move main block debugfs initialization to its own file
  blktrace: move blktrace debugfs creation to helper function
  blktrace: fix debugfs use after free
  block: revert back to synchronous request_queue removal
  blktrace: upgrade warns to BUG_ON() on unexpected circmunstances
  blk-debugfs: upgrade warns to BUG_ON() if directory is already found
  blktrace: move debugfs file creation to its own function
  blktrace: add checks for created debugfs files on setup
  block: panic if block debugfs dir is not created
  block: put_device() if device_add() fails

 block/Makefile               |   1 +
 block/blk-core.c             |  28 +++++---
 block/blk-debugfs.c          |  39 ++++++++++++
 block/blk-mq-debugfs.c       |   5 --
 block/blk-sysfs.c            |  47 ++++++++------
 block/blk.h                  |  18 ++++++
 block/genhd.c                |   4 +-
 include/linux/blkdev.h       |   7 +-
 include/linux/blktrace_api.h |   1 +
 kernel/trace/blktrace.c      | 120 +++++++++++++++++++++++++++++++----
 10 files changed, 218 insertions(+), 52 deletions(-)
 create mode 100644 block/blk-debugfs.c

-- 
2.25.1


             reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-19 19:45 Luis Chamberlain [this message]
2020-04-19 19:45 ` [PATCH v2 01/10] block: move main block debugfs initialization to its own file Luis Chamberlain
2020-04-19 21:06   ` Bart Van Assche
2020-04-19 19:45 ` [PATCH v2 02/10] blktrace: move blktrace debugfs creation to helper function Luis Chamberlain
2020-04-19 21:11   ` Bart Van Assche
2020-04-22  7:12   ` Christoph Hellwig
2020-04-19 19:45 ` [PATCH v2 03/10] blktrace: fix debugfs use after free Luis Chamberlain
2020-04-19 21:55   ` Bart Van Assche
2020-04-20  0:04     ` Luis Chamberlain
2020-04-20  0:38       ` Bart Van Assche
2020-04-20 18:46         ` Luis Chamberlain
2020-04-20 20:16   ` Greg KH
2020-04-20 20:41     ` Luis Chamberlain
2020-04-21  7:00       ` Greg KH
2020-04-22  7:28         ` Luis Chamberlain
2020-04-22  9:43           ` Ming Lei
2020-04-22 10:31             ` Luis Chamberlain
2020-04-24 23:47             ` Luis Chamberlain
2020-04-22  7:29       ` Christoph Hellwig
2020-04-22  7:34         ` Luis Chamberlain
2020-04-22  7:27   ` Christoph Hellwig
2020-04-22  7:48     ` Luis Chamberlain
2020-04-22  8:10       ` Christoph Hellwig
2020-04-22  8:26         ` Luis Chamberlain
2020-04-19 19:45 ` [PATCH v2 04/10] block: revert back to synchronous request_queue removal Luis Chamberlain
2020-04-19 22:23   ` Bart Van Assche
2020-04-20 18:59     ` Luis Chamberlain
2020-04-20 21:11       ` Bart Van Assche
2020-04-20 21:51         ` Luis Chamberlain
2020-04-19 19:45 ` [PATCH v2 05/10] blktrace: upgrade warns to BUG_ON() on unexpected circmunstances Luis Chamberlain
2020-04-19 22:50   ` Bart Van Assche
2020-04-19 23:07     ` Luis Chamberlain
2020-04-20 23:20       ` Steven Rostedt
2020-04-19 19:45 ` [PATCH v2 06/10] blk-debugfs: upgrade warns to BUG_ON() if directory is already found Luis Chamberlain
2020-04-20 11:36   ` Greg KH
2020-04-19 19:45 ` [PATCH v2 07/10] blktrace: move debugfs file creation to its own function Luis Chamberlain
2020-04-19 22:55   ` Bart Van Assche
2020-04-20 11:37     ` Greg KH
2020-04-19 19:45 ` [PATCH v2 08/10] blktrace: add checks for created debugfs files on setup Luis Chamberlain
2020-04-19 22:57   ` Bart Van Assche
2020-04-19 23:05     ` Luis Chamberlain
2020-04-19 23:17       ` Bart Van Assche
2020-04-20 11:40         ` Greg KH
2020-04-20 18:44           ` Luis Chamberlain
2020-04-20 20:11             ` Greg KH
2020-04-20 20:20               ` Luis Chamberlain
2020-04-21  6:55                 ` Greg KH
2020-04-20 11:39   ` Greg KH
2020-04-19 19:45 ` [PATCH v2 09/10] block: panic if block debugfs dir is not created Luis Chamberlain
2020-04-19 23:08   ` Bart Van Assche
2020-04-20 11:38   ` Greg KH
2020-04-19 19:45 ` [PATCH v2 10/10] block: put_device() if device_add() fails Luis Chamberlain
2020-04-19 23:40   ` Bart Van Assche
2020-04-24 22:32     ` Luis Chamberlain
2020-04-25  1:58       ` Bart Van Assche
2020-04-25  2:12         ` Luis Chamberlain
2020-04-19 19:48 ` [PATCH v2 00/10] block: fix blktrace debugfs use after free Luis Chamberlain

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200419194529.4872-1-mcgrof@kernel.org \
    --to=mcgrof@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=bvanassche@acm.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jack@suse.cz \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=ming.lei@redhat.com \
    --cc=mingo@redhat.com \
    --cc=nstange@suse.de \
    --cc=rostedt@goodmis.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yukuai3@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git