Forwarded: [PATCH] nvme: hold a controller reference while scan_work is queued or running

[syzbot <syzbot+76c0f0ce8f1e846b4f84@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] nvme: hold a controller reference while scan_work is queued or running
Author: kartikey406@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

syzbot reported a slab-use-after-free in nvme_query_zone_info(), called
from the async namespace scan:

  BUG: KASAN: slab-use-after-free in nvme_query_zone_info+0x4fd/0x6f0
  Read of size 4 by task kworker/u8:13
  nvme_query_zone_info
  nvme_update_ns_info_block
  nvme_alloc_ns
  nvme_scan_ns
  async_run_entry_fn

The freed object is the effects log (ctrl->cels) allocated by
nvme_get_effects_log(). It was freed by a write to the sysfs
delete_controller attribute, which dropped the last reference on the
controller via nvme_sysfs_delete() -> put_device() -> nvme_free_ctrl()
-> nvme_free_cels() while the scan was still running.

nvme_scan_work() waits for all of its async scan jobs before it
returns, so the jobs cannot outlive the work. However, nothing keeps the
controller alive while scan_work is pending or running, so the final
controller reference can be dropped underneath it and the effects log is
freed while the scan is still dereferencing it.

Take a controller reference in nvme_queue_scan() when scan_work is
queued, and drop it when nvme_scan_work() finishes. If queue_work()
reports that the work was already pending, drop the extra reference
because the pending instance already owns one. Convert the early returns
in nvme_scan_work() to a common exit so the reference is dropped on
every path.

Reported-by: syzbot+76c0f0ce8f1e846b4f84@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=76c0f0ce8f1e846b4f84
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/nvme/host/core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index beea23d04a70..11599b0061b1 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -165,8 +165,11 @@ void nvme_queue_scan(struct nvme_ctrl *ctrl)
 	/*
 	 * Only new queue scan work when admin and IO queues are both alive
 	 */
-	if (nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE && ctrl->tagset)
-		queue_work(nvme_wq, &ctrl->scan_work);
+	if (nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE && ctrl->tagset) {
+		nvme_get_ctrl(ctrl);
+		if (!queue_work(nvme_wq, &ctrl->scan_work))
+			nvme_put_ctrl(ctrl);
+	}
 }
 
 /*
@@ -4635,7 +4638,7 @@ static void nvme_scan_work(struct work_struct *work)
 
 	/* No tagset on a live ctrl means IO queues could not created */
 	if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE || !ctrl->tagset)
-		return;
+		goto out;
 
 	/*
 	 * Identify controller limits can change at controller reset due to
@@ -4648,7 +4651,7 @@ static void nvme_scan_work(struct work_struct *work)
 	if (ret < 0) {
 		dev_warn(ctrl->device,
 			"reading non-mdts-limits failed: %d\n", ret);
-		return;
+		goto out;
 	}
 
 	if (test_and_clear_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events)) {
@@ -4679,6 +4682,8 @@ static void nvme_scan_work(struct work_struct *work)
 		/* Re-read the ANA log page to not miss updates */
 		queue_work(nvme_wq, &ctrl->ana_work);
 #endif
+out:
+	nvme_put_ctrl(ctrl);
 }
 
 /*
-- 
2.43.0