[PATCH 0/2] xen/scsiback: fix command-tag handling on pre-completion error paths

[PATCH 0/2] xen/scsiback: fix command-tag handling on pre-completion error paths

[Michael Bommarito]

scsiback_get_pend_req() hands a pvSCSI frontend request a session tag and
a zeroed se_cmd.  Two error paths that run before the command completes
through the target core mishandle that command and leak (or, in one case,
underflow) the tag.

Impact: a pvSCSI guest can exhaust a LUN's per-session command tag pool,
stopping the LUN, via crafted ring requests; for the first case the
refcount underflow also panics the host under panic_on_warn.

Patch 1 fixes scsiback_do_cmd_fn(): on a failed grant map and on an
unknown request type the never-initialised command (cmd_kref == 0) is
freed with transport_generic_free_cmd(), which underflows the zero
refcount and leaks the tag.

Patch 2 fixes scsiback_device_action(): when target_submit_tmr() fails the
err: path frees nothing.  transport_generic_free_cmd() cannot be used there
either, since the command is initialised by then and se_tmr_req has already
been freed on one error sub-path.

Both paths go through one helper that returns just the tag.

Patch 1's underflow was reproduced on a Xen dom0 (guest to host, with a
panic_on_warn host panic); with the series applied the same request is
handled with no underflow.

Michael Bommarito (2):
  xen/scsiback: free unsubmitted command instead of double-putting it
  xen/scsiback: free the command tag on the TMR submit-failure path

 drivers/xen/xen-scsiback.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)


base-commit: 5200f5f493f79f14bbdc349e402a40dfb32f23c8
-- 
2.53.0

[PATCH 1/2] xen/scsiback: free unsubmitted command instead of double-putting it

[Michael Bommarito]

scsiback_get_pend_req() obtains a command tag and returns a
vscsibk_pend whose embedded se_cmd has only been memset to 0, so
its cmd_kref is 0; the se_cmd is initialised (kref_init() via
target_init_cmd()) only later, in scsiback_cmd_exec(), on the
successful VSCSIIF_ACT_SCSI_CDB path. The two error paths in
scsiback_do_cmd_fn() taken before the command is submitted -- a
failed scsiback_gnttab_data_map() and an unknown ring_req.act --
call transport_generic_free_cmd(&pending_req->se_cmd, 0), which
kref_put()s a refcount of 0. That underflows it ("refcount_t:
underflow; use-after-free") and, as the release function is not
run, leaks the command tag.

Impact: a pvSCSI guest can leak every command tag of a LUN's
session, stopping the LUN, by submitting requests with a bad
grant reference or an unknown request type; under panic_on_warn
the refcount underflow panics the host.

Add a helper that just returns the tag with target_free_tag() and
sends the error response. It frees the tag while the v2p reference
still pins the session, and snapshots the response fields
beforehand because freeing the tag can let another ring reuse the
pending_req slot.

Fixes: 2dbcdf33dbf6 ("xen-scsiback: Convert to percpu_ida tag allocation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
Reproduced on a Xen dom0 (Linux 6.1.y) exporting a pvSCSI LUN to a guest.
A frontend that sends a single ring request with an unknown action type
drives scsiback_do_cmd_fn() into transport_generic_free_cmd() on the
never-initialised command and logs

  refcount_t: underflow; use-after-free
  WARNING: ... refcount_warn_saturate
   transport_generic_free_cmd+0x... [target_core_mod]
   scsiback_do_cmd_fn+0x... [xen_scsiback]
   scsiback_irq_fn+0x... [xen_scsiback]

from the vscsiif IRQ thread, and panics the dom0 under panic_on_warn.  The
failed grant-map path reaches the same free.  With this patch the same
request is answered with DID_ERROR and the tag is returned, with no
underflow.  These error paths are unchanged since 2dbcdf33dbf6, so mainline
is affected identically.

 drivers/xen/xen-scsiback.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index e33f95c91b096..f324732eba7f8 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -611,6 +611,25 @@ static void scsiback_disconnect(struct vscsibk_info *info)
 	xenbus_unmap_ring_vfree(info->dev, info->ring.sring);
 }
 
+/*
+ * Send the error response for a request that did not reach the target core
+ * and return its tag.  Free the tag before the response drops the v2p
+ * reference that keeps the session alive, and snapshot what the response
+ * needs since returning the tag can let the slot be reused.
+ */
+static void scsiback_resp_and_free(struct vscsibk_pend *pending_req,
+				   int32_t result)
+{
+	struct vscsibk_info *info = pending_req->info;
+	struct v2p_entry *v2p = pending_req->v2p;
+	struct se_session *se_sess = v2p->tpg->tpg_nexus->tvn_se_sess;
+	u16 rqid = pending_req->rqid;
+
+	target_free_tag(se_sess, &pending_req->se_cmd);
+	scsiback_send_response(info, NULL, result, 0, rqid);
+	kref_put(&v2p->kref, scsiback_free_translation_entry);
+}
+
 static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	enum tcm_tmreq_table act, int tag)
 {
@@ -792,9 +811,8 @@ static int scsiback_do_cmd_fn(struct vscsibk_info *info,
 		case VSCSIIF_ACT_SCSI_CDB:
 			if (scsiback_gnttab_data_map(&ring_req, pending_req)) {
 				scsiback_fast_flush_area(pending_req);
-				scsiback_do_resp_with_sense(NULL,
-						DID_ERROR << 16, 0, pending_req);
-				transport_generic_free_cmd(&pending_req->se_cmd, 0);
+				scsiback_resp_and_free(pending_req,
+						       DID_ERROR << 16);
 			} else {
 				scsiback_cmd_exec(pending_req);
 			}
@@ -808,9 +826,7 @@ static int scsiback_do_cmd_fn(struct vscsibk_info *info,
 			break;
 		default:
 			pr_err_ratelimited("invalid request\n");
-			scsiback_do_resp_with_sense(NULL, DID_ERROR << 16, 0,
-						    pending_req);
-			transport_generic_free_cmd(&pending_req->se_cmd, 0);
+			scsiback_resp_and_free(pending_req, DID_ERROR << 16);
 			break;
 		}
 
-- 
2.53.0

Re: [PATCH 1/2] xen/scsiback: free unsubmitted command instead of double-putting it

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1549 bytes --]

On 11.06.26 14:30, Michael Bommarito wrote:
> scsiback_get_pend_req() obtains a command tag and returns a
> vscsibk_pend whose embedded se_cmd has only been memset to 0, so
> its cmd_kref is 0; the se_cmd is initialised (kref_init() via
> target_init_cmd()) only later, in scsiback_cmd_exec(), on the
> successful VSCSIIF_ACT_SCSI_CDB path. The two error paths in
> scsiback_do_cmd_fn() taken before the command is submitted -- a
> failed scsiback_gnttab_data_map() and an unknown ring_req.act --
> call transport_generic_free_cmd(&pending_req->se_cmd, 0), which
> kref_put()s a refcount of 0. That underflows it ("refcount_t:
> underflow; use-after-free") and, as the release function is not
> run, leaks the command tag.
> 
> Impact: a pvSCSI guest can leak every command tag of a LUN's
> session, stopping the LUN, by submitting requests with a bad
> grant reference or an unknown request type; under panic_on_warn
> the refcount underflow panics the host.
> 
> Add a helper that just returns the tag with target_free_tag() and
> sends the error response. It frees the tag while the v2p reference
> still pins the session, and snapshots the response fields
> beforehand because freeing the tag can let another ring reuse the
> pending_req slot.
> 
> Fixes: 2dbcdf33dbf6 ("xen-scsiback: Convert to percpu_ida tag allocation")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 2/2] xen/scsiback: free the command tag on the TMR submit-failure path

[Michael Bommarito]

scsiback_device_action() obtains a command tag in
scsiback_get_pend_req() and submits a task-management request with
target_submit_tmr(). When target_submit_tmr() fails it returns < 0
and scsiback jumps to the err: label, which sends a response but
frees nothing, leaking the tag.

Impact: a pvSCSI guest can leak the command tags of a LUN's
session, stopping the LUN, by issuing VSCSIIF_ACT_SCSI_ABORT or
RESET requests whenever target_submit_tmr() fails.

transport_generic_free_cmd() cannot be used here. By the time
target_submit_tmr() returns an error it has already run
__target_init_cmd() (so se_cmd->cmd_kref is one, not zero), and on
its target_get_sess_cmd() error path it has freed se_cmd->se_tmr_req
via core_tmr_release_req() while leaving SCF_SCSI_TMR_CDB set and
the pointer dangling. Letting the command release run
target_free_cmd_mem() would then double-free se_tmr_req.

Use the same helper, which returns just the tag, on this path too.

Fixes: 2dbcdf33dbf6 ("xen-scsiback: Convert to percpu_ida tag allocation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 drivers/xen/xen-scsiback.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index f324732eba7f8..c7036e0e41bda 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -658,7 +658,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req,
 	return;
 
 err:
-	scsiback_do_resp_with_sense(NULL, err, 0, pending_req);
+	scsiback_resp_and_free(pending_req, err);
 }
 
 /*
-- 
2.53.0

Re: [PATCH 2/2] xen/scsiback: free the command tag on the TMR submit-failure path

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1337 bytes --]

On 11.06.26 14:30, Michael Bommarito wrote:
> scsiback_device_action() obtains a command tag in
> scsiback_get_pend_req() and submits a task-management request with
> target_submit_tmr(). When target_submit_tmr() fails it returns < 0
> and scsiback jumps to the err: label, which sends a response but
> frees nothing, leaking the tag.
> 
> Impact: a pvSCSI guest can leak the command tags of a LUN's
> session, stopping the LUN, by issuing VSCSIIF_ACT_SCSI_ABORT or
> RESET requests whenever target_submit_tmr() fails.
> 
> transport_generic_free_cmd() cannot be used here. By the time
> target_submit_tmr() returns an error it has already run
> __target_init_cmd() (so se_cmd->cmd_kref is one, not zero), and on
> its target_get_sess_cmd() error path it has freed se_cmd->se_tmr_req
> via core_tmr_release_req() while leaving SCF_SCSI_TMR_CDB set and
> the pointer dangling. Letting the command release run
> target_free_cmd_mem() would then double-free se_tmr_req.
> 
> Use the same helper, which returns just the tag, on this path too.
> 
> Fixes: 2dbcdf33dbf6 ("xen-scsiback: Convert to percpu_ida tag allocation")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 0/2] xen/scsiback: fix command-tag handling on pre-completion error paths

[Martin K. Petersen]

Michael,

> scsiback_get_pend_req() hands a pvSCSI frontend request a session tag
> and a zeroed se_cmd. Two error paths that run before the command
> completes through the target core mishandle that command and leak (or,
> in one case, underflow) the tag.

Applied to 7.2/scsi-staging, thanks!

-- 
Martin K. Petersen