linux-kernel - Re: [PATCH 10/15] scsi: qla2xxx: fix TMR failure handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250912143615.GB624@yadro.com>
Date: Fri, 12 Sep 2025 17:36:15 +0300
From: Dmitry Bogdanov <d.bogdanov@...ro.com>
To: Tony Battersby <tonyb@...ernetics.com>
CC: Nilesh Javali <njavali@...vell.com>,
	<GR-QLogic-Storage-Upstream@...vell.com>, "James E.J. Bottomley"
	<James.Bottomley@...senPartnership.com>, "Martin K. Petersen"
	<martin.petersen@...cle.com>, linux-scsi <linux-scsi@...r.kernel.org>,
	<target-devel@...r.kernel.org>, <scst-devel@...ts.sourceforge.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 10/15] scsi: qla2xxx: fix TMR failure handling

On Mon, Sep 08, 2025 at 03:02:49PM -0400, Tony Battersby wrote:
> 
> (target mode)
> 
> If handle_tmr() fails (e.g. -ENOMEM):
> - qlt_send_busy() makes no sense because it sends a SCSI command
>   response instead of a TMR response.

There is not only -ENOMEM can be returned by handle_tmr.

> - Calling mempool_free() directly can lead to memory-use-after-free.

No, it is a API contract between modules. If handle_tmr returned an error,
then the caller of handle_tmr is responsible to make a cleanup.
Otherwise, target module (tcm_qla2xxx) is responsible. The same rule is
for handle_cmd.

> 
> Instead just reject the TMR and send the TMR response since that code
> path is well-tested.  But be sure to set SCF_SCSI_TMR_CDB in case
> core_tmr_alloc_req() returns -ENOMEM; otherwise the wrong function will
> be called to free the mcmd.
> 
> Signed-off-by: Tony Battersby <tonyb@...ernetics.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c | 33 +++++++------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index da010de9ba8a..7c278f92ff3b 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -2005,7 +2005,6 @@ static void qlt_do_tmr_work(struct work_struct *work)
>         struct qla_hw_data *ha = mcmd->vha->hw;
>         int rc;
>         uint32_t tag;
> -       unsigned long flags;
> 
>         switch (mcmd->tmr_func) {
>         case QLA_TGT_ABTS:
> @@ -2020,34 +2019,16 @@ static void qlt_do_tmr_work(struct work_struct *work)
>             mcmd->tmr_func, tag);
> 
>         if (rc != 0) {
> -               spin_lock_irqsave(mcmd->qpair->qp_lock_ptr, flags);
> -               switch (mcmd->tmr_func) {
> -               case QLA_TGT_ABTS:
> -                       mcmd->fc_tm_rsp = FCP_TMF_REJECTED;
> -                       qlt_build_abts_resp_iocb(mcmd);
> -                       break;
> -               case QLA_TGT_LUN_RESET:
> -               case QLA_TGT_CLEAR_TS:
> -               case QLA_TGT_ABORT_TS:
> -               case QLA_TGT_CLEAR_ACA:
> -               case QLA_TGT_TARGET_RESET:
> -                       qlt_send_busy(mcmd->qpair, &mcmd->orig_iocb.atio,
> -                           qla_sam_status);

Sending SCSI status(other octets in FCP_RSP frame) in response to TMF is
against the standard. So, that is the real subject to fix.

> -                       break;
> -
> -               case QLA_TGT_ABORT_ALL:
> -               case QLA_TGT_NEXUS_LOSS_SESS:
> -               case QLA_TGT_NEXUS_LOSS:
> -                       qlt_send_notify_ack(mcmd->qpair,
> -                           &mcmd->orig_iocb.imm_ntfy, 0, 0, 0, 0, 0, 0);
> -                       break;
> -               }
> -               spin_unlock_irqrestore(mcmd->qpair->qp_lock_ptr, flags);
> -
>                 ql_dbg(ql_dbg_tgt_mgt, mcmd->vha, 0xf052,
>                     "qla_target(%d):  tgt_ops->handle_tmr() failed: %d\n",
>                     mcmd->vha->vp_idx, rc);
> -               mempool_free(mcmd, qla_tgt_mgmt_cmd_mempool);
> +               /*
> +                * SCF_SCSI_TMR_CDB might not have been set on error, but it
> +                * must be set for the mcmd to be freed properly.
> +                */
> +               mcmd->se_cmd.se_cmd_flags |= SCF_SCSI_TMR_CDB;
> +               mcmd->fc_tm_rsp = FCP_TMF_REJECTED;

FCP_TMF_REJECTED means that this TMF is not supported, FCP_TMF_FAILED is
more appretiate here.

> +               qlt_xmit_tm_rsp(mcmd);

qlt_xmit_tm_rsp does not free mcmd for TMF ABORT. So you introduce a memleak.

>         }
>  }
> 
> --
> 2.43.0
> 
> 
>