Re: [PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] nvme-fc: use-after-free in nvme_fc_match_disconn_ls() after nvme_fc_ctrl_put()
--

commit 8782ae8cb9a0539907c2a9d660e71e6ada06052f
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

This commit switches the rport->ctrl_list traversal in
nvme_fc_match_disconn_ls() from list_for_each_entry_safe() to
list_for_each_entry() to avoid caching a next entry that could be freed while
rport->lock is dropped.

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0b9ff50d26ff3..edf192055618c 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>  {
>  	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>  					&lsop->rqstbuf->rq_dis_assoc;
> -	struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
> +	struct nvme_fc_ctrl *ctrl, *ret = NULL;
>  	struct nvmefc_ls_rcv_op *oldls = NULL;
>  	u64 association_id = be64_to_cpu(rqst->associd.association_id);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rport->lock, flags);
>  
> -	list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
> +	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
>  		if (!nvme_fc_ctrl_get(ctrl))
>  			continue;
>  		spin_lock(&ctrl->lock);

[Severity: Critical]
Does switching to list_for_each_entry() in nvme_fc_match_disconn_ls() in
drivers/nvme/host/fc.c still leave a use-after-free on ctrl itself when
nvme_fc_handle_ls_rqst_work() -> nvme_fc_ls_disconnect_assoc() ->
nvme_fc_match_disconn_ls() iterates past a non-matching controller that is
concurrently being torn down?

At the end of each non-matching iteration in nvme_fc_match_disconn_ls(), the
loop drops rport->lock and releases the reference on ctrl before the loop step
runs:

	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
		if (!nvme_fc_ctrl_get(ctrl))
			continue;
		...
		if (ret)
			/* leave the ctrl get reference */
			break;
		spin_unlock_irqrestore(&rport->lock, flags);
		nvme_fc_ctrl_put(ctrl);
		spin_lock_irqsave(&rport->lock, flags);
	}

Because list_for_each_entry() evaluates ctrl = list_next_entry(ctrl,
ctrl_list) after nvme_fc_ctrl_put(ctrl) and after rport->lock was dropped and
re-acquired, ctrl is no longer pinned when ctrl->ctrl_list.next is read. If
nvme_fc_ctrl_put(ctrl) (or a concurrent nvme_fc_free_ctrl() while rport->lock
is dropped) drops ctrl->ref to zero, nvme_fc_ctrl_free() unlinks and frees
ctrl:

	spin_lock_irqsave(&ctrl->rport->lock, flags);
	owns_opts = !list_empty(&ctrl->ctrl_list);
	list_del(&ctrl->ctrl_list);
	spin_unlock_irqrestore(&ctrl->rport->lock, flags);
	...
	kfree(ctrl);

When the loop step then reads ctrl->ctrl_list.next, it reads from the freed
ctrl struct and follows LIST_POISON1 into the next iteration (or loops
forever if a concurrent nvme_fc_init_ctrl() error path ran
list_del_init(&ctrl->ctrl_list)).

Since rport->lock already serializes against list_del() in
nvme_fc_ctrl_free() and list_del_init() in nvme_fc_init_ctrl(), could
nvme_fc_match_disconn_ls() avoid dropping rport->lock in the loop altogether
by checking association_id == ctrl->association_id first and only calling
nvme_fc_ctrl_get(ctrl) when a matching controller is found?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106015842.3534898-1-kanie@linux.alibaba.com?part=2