Re: [PATCH v7 09/17] nvme: Implement cross-controller reset completion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] nvme: UAF and deadlock in nvme_ccr_work() when controller is
  not LIVE
--

commit ccb451fbdaf89eaa6e4be835d2b43da8b0854559
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Implement cross-controller reset completion

This commit adds ctrl->ccr_work and handles NVME_AER_NOTICE_CCR_COMPLETED
events by reading the NVME_LOG_CCR log page and completing matching entries on
ctrl->ccr_list.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 27c33dc49175e..54b83092a9e8f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
[ ... ]
> @@ -5060,6 +5061,48 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
>  	kfree(log);
>  }
>  
> +static void nvme_ccr_work(struct work_struct *work)
> +{
> +	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ccr_work);
> +	struct nvme_ccr_entry *ccr;
> +	struct nvme_ccr_log_entry *entry;
> +	struct nvme_ccr_log *log;
> +	int num_entries, ret, i;
> +	unsigned long flags;
> +
> +	log = kmalloc_obj(*log);
> +	if (!log)
> +		return;
> +
> +	ret = nvme_get_log(ctrl, 0, NVME_LOG_CCR, 0x01,
> +			   0x00, log, sizeof(*log), 0);

[Severity: High]
Can nvme_ccr_work() submit a Get Log Page command to a destroyed or quiesced
ctrl->admin_q when the controller is no longer live?

During controller deletion, nvme_do_delete_ctrl() calls nvme_stop_ctrl()
before ctrl->ops->delete_ctrl() stops the admin queue, and
nvme_uninit_ctrl() runs only after nvme_remove_admin_tag_set() has already
destroyed ctrl->admin_q and freed ctrl->admin_tagset.  If an in-flight AEN
completes with NVME_AER_NOTICE_CCR_COMPLETED after nvme_stop_ctrl() cancels
ctrl->ccr_work, nvme_ccr_work() is re-queued and can access ctrl->admin_q
while or after it is destroyed:

CPU0                                  CPU1
nvme_do_delete_ctrl()
  nvme_stop_ctrl()
    cancel_work_sync(&ctrl->ccr_work)
                                      nvme_complete_async_event()
                                        nvme_handle_aen_notice()
                                          queue_work(nvme_wq, &ctrl->ccr_work)
  ctrl->ops->delete_ctrl()
    ...
    nvme_remove_admin_tag_set()
      blk_mq_destroy_queue(ctrl->admin_q)
      blk_mq_free_tag_set(ctrl->admin_tagset)
                                      nvme_ccr_work()
                                        nvme_get_log(ctrl, ...)
                                          <-- use-after-free on admin_q
  nvme_uninit_ctrl()
    cancel_work_sync(&ctrl->ccr_work)

Because nvme_ccr_work() does not check nvme_ctrl_state(ctrl) ==
NVME_CTRL_LIVE, it can also run while ctrl->admin_q is quiesced during reset
or transport error recovery; nvme_get_log() then blocks in blk_execute_rq()
waiting for the queue to be unquiesced, and the next nvme_stop_ctrl() ->
cancel_work_sync(&ctrl->ccr_work) deadlocks before teardown can unquiesce
ctrl->admin_q.

Checking nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE at the start of
nvme_ccr_work() (like nvme_ana_work() and nvme_scan_work()) and cancelling
ctrl->ccr_work in nvme_remove_admin_tag_set() before destroying
ctrl->admin_q would avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=9