Two problems in ibmvfc_nvme_ls_abort():
1. Use-after-free / stale pointer dereference.
ibmvfc_init_ls_abort() reads abt_evt = ls_abort->private and
immediately dereferences abt_evt->tgt. ibmvfc_ls_req_done() calls
ibmvfc_free_event() under host_lock, which returns the event slot to
the pool. If the LS completes naturally just before ls_abort is
called, abt_evt points to a freed (and potentially reused) event,
making the dereference a UAF.
Fix by taking host_lock before reading ls_abort->private and checking
evt->free (set to 1 by ibmvfc_free_event() under host_lock) to detect
whether the original LS has already completed. If so, there is
nothing to cancel and we return early. ibmvfc_get_event() is also
moved inside the lock so the validity check and event allocation are
atomic with respect to the completion path.
2. Blocking wait on timeout workqueue (same class as the FCP abort fix).
The original code called wait_for_completion() from ls_abort, which
is invoked by the NVMe-FC transport from a context that must not
block.
Fix by replacing ibmvfc_sync_nvme_completion with a dedicated async
callback ibmvfc_nvme_ls_abort_done() that logs any non-zero MAD
status, drops the target kref, and frees the event.
ibmvfc_send_event() guarantees the callback fires on both success and
failure paths, so ibmvfc_nvme_ls_abort() returns immediately after
ibmvfc_send_event().
ibmvfc_sync_nvme_completion is now unused and is removed.
ibmvfc_init_ls_abort() is updated to take the validated abt_evt pointer
directly instead of deriving it from ls_abort->private.
Fixes: 20bec08f0208 ("scsi: ibmvfc: implement nvme-fc LS abort handling
callback")
Signed-off-by: Tyrel Datwyler <[email protected]>
---
drivers/scsi/ibmvscsi/ibmvfc-nvme.c | 68 +++++++++++++++--------------
1 file changed, 36 insertions(+), 32 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
index d23e5f31f8b5..475177cda103 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-nvme.c
@@ -103,6 +103,7 @@ static void ibmvfc_ls_req_done(struct ibmvfc_event *evt)
rc = -EIO;
evt->ls_req->done(evt->ls_req, rc);
+ evt->ls_req = NULL;
kref_put(&tgt->kref, ibmvfc_release_tgt);
ibmvfc_free_event(evt);
@@ -164,21 +165,12 @@ static int ibmvfc_nvme_ls_req(struct nvme_fc_local_port
*lport,
return 0;
}
-static void ibmvfc_sync_nvme_completion(struct ibmvfc_event *evt)
+static void ibmvfc_init_ls_abort(struct ibmvfc_event *evt,
+ struct ibmvfc_event *abt_evt)
{
- /* copy the response back */
- if (evt->sync_iu)
- *evt->sync_iu = *evt->xfer_iu;
-
- complete(&evt->comp);
-}
-
-static void ibmvfc_init_ls_abort(struct ibmvfc_event *evt, struct
nvmefc_ls_req *ls_abort)
-{
- struct ibmvfc_tmf *tmf;
- struct ibmvfc_event *abt_evt = ls_abort->private;
struct ibmvfc_target *tgt = abt_evt->tgt;
struct ibmvfc_host *vhost = evt->vhost;
+ struct ibmvfc_tmf *tmf;
tmf = &evt->iu.tmf;
memset(tmf, 0, sizeof(*tmf));
@@ -192,8 +184,18 @@ static void ibmvfc_init_ls_abort(struct ibmvfc_event *evt,
struct nvmefc_ls_req
tmf->cancel_key = cpu_to_be32((u64)abt_evt);
tmf->my_cancel_key = cpu_to_be32((u64)evt);
tmf->assoc_id = cpu_to_be64(tgt->assoc_id);
+}
+
+static void ibmvfc_nvme_ls_abort_done(struct ibmvfc_event *evt)
+{
+ u16 status = be16_to_cpu(evt->xfer_iu->mad_common.status);
+
+ if (status)
+ ibmvfc_dbg(evt->vhost, "ls_abort: cancel MAD failed with
rc=%x\n",
+ status);
- init_completion(&evt->comp);
+ kref_put(&evt->tgt->kref, ibmvfc_release_tgt);
+ ibmvfc_free_event(evt);
}
static void ibmvfc_nvme_ls_abort(struct nvme_fc_local_port *lport,
@@ -202,34 +204,36 @@ static void ibmvfc_nvme_ls_abort(struct
nvme_fc_local_port *lport,
{
struct ibmvfc_host *vhost = lport->private;
struct ibmvfc_target *tgt = rport->private;
- struct ibmvfc_event *evt;
- union ibmvfc_iu rsp;
+ struct ibmvfc_event *evt, *abt_evt;
unsigned long flags;
- u16 status = IBMVFC_MAD_CRQ_ERROR;
+
+ spin_lock_irqsave(&vhost->host->host_lock, flags);
+
+ /*
+ * If the original LS has already completed naturally, abt_evt will
+ * have been freed back to the pool (evt->free set to 1 under
+ * host_lock by ibmvfc_free_event()). Nothing left to cancel.
+ */
+ abt_evt = ls_abort->private;
+ if (!abt_evt || atomic_read(&abt_evt->free) || abt_evt->ls_req !=
ls_abort) {
+ spin_unlock_irqrestore(&vhost->host->host_lock, flags);
+ return;
+ }
evt = ibmvfc_get_event(&vhost->crq);
- if (!vhost->logged_in || !evt)
+ if (!vhost->logged_in || !evt) {
+ spin_unlock_irqrestore(&vhost->host->host_lock, flags);
return;
+ }
- spin_lock_irqsave(&vhost->host->host_lock, flags);
kref_get(&tgt->kref);
- ibmvfc_init_event(evt, ibmvfc_sync_nvme_completion, IBMVFC_MAD_FORMAT);
- ibmvfc_init_ls_abort(evt, ls_abort);
- evt->sync_iu = &rsp;
+ ibmvfc_init_event(evt, ibmvfc_nvme_ls_abort_done, IBMVFC_MAD_FORMAT);
+ ibmvfc_init_ls_abort(evt, abt_evt);
+ evt->tgt = tgt;
if (ibmvfc_send_event(evt, vhost, default_timeout))
- goto out;
-
- spin_unlock_irqrestore(&vhost->host->host_lock, flags);
-
- wait_for_completion(&evt->comp);
- status = be16_to_cpu(rsp.mad_common.status);
- spin_lock_irqsave(&vhost->host->host_lock, flags);
- ibmvfc_free_event(evt);
-out:
+ kref_put(&tgt->kref, ibmvfc_release_tgt);
spin_unlock_irqrestore(&vhost->host->host_lock, flags);
- ibmvfc_dbg(vhost, "ls_abort: cancel failed with rc=%x\n", status);
- kref_put(&tgt->kref, ibmvfc_release_tgt);
}
static void ibmvfc_nvme_done(struct ibmvfc_event *evt)
--
2.55.0
