linux-kernel - Re: [DMARC Error]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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250917130641.GD624@yadro.com>
Date: Wed, 17 Sep 2025 16:06:41 +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: [DMARC Error]Re: [PATCH 10/15] scsi: qla2xxx: fix TMR failure
 handling

On Tue, Sep 16, 2025 at 12:04:11PM -0400, Tony Battersby wrote:
> 
> On 9/12/25 10:36, Dmitry Bogdanov wrote:
> > On Mon, Sep 08, 2025 at 03:02:49PM -0400, Tony Battersby wrote:
> >> 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.
> 
> Indeed.  I will remove mention of -ENOMEM since it isn't really relevant.
> 
> >> +               mcmd->fc_tm_rsp = FCP_TMF_REJECTED;
> >>
> > FCP_TMF_REJECTED means that this TMF is not supported, FCP_TMF_FAILED is
> > more appretiate here.
> 
> I will make that change.
> 
> >> - 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.
> >> +               qlt_xmit_tm_rsp(mcmd);
> > qlt_xmit_tm_rsp does not free mcmd for TMF ABORT. So you introduce a memleak.
> 
> I just tested it, and there is no memleak.  qlt_build_abts_resp_iocb()
> sets req->outstanding_cmds[h] to mcmd, and then
> qlt_handle_abts_completion() calls ->free_mcmd after getting a response
> from the ISP.

ha->tgt.tgt_ops->free_mcmd(mcmd) can be called when mcmd was handled by the
target core, i.e. handle_tmr() returned 0. LIO's tcm_qla2xxx_free_mcmd calls
target core function transport_generic_free_cmd to clear core's command
object. Which in turns calls eventually mempool_free in qla2xxx.
But when handle_tmr returned an error then there is no that core's
command object and LIO will not free the mcmd. That is how a memleak
will happen.

> The original code had a memory-use-after-free by calling
> qlt_build_abts_resp_iocb() and then mempool_free(), and
> then qlt_handle_abts_completion() used the freed mcmd.  I can reword the
> commit message to make this clearer.

BTW, the easyest way to just to fix that use-after-free is to use
qlt_24xx_send_abts_resp instead of qlt_build_abts_resp_iocb like in other
places, where there is no mcmd and there is no need to wait the
completion from FW.

We use this patch:


From: Dmitry Bogdanov <d.bogdanov@...ro.com>
Date: Thu, 27 Apr 2023 15:41:55 +0300
Subject: [PATCH] scsi: qla2xxx: fix use-after-free on ABTS_RESP sending

If an abort was rejected by LIO Core qla2xxx sends ABTS_RESP with rejected
status. But it does not save mcmd pointer which is freed right after sending
the response to FW. It leads to use-after-free at the completion from FW.

Use a correct function to send ABTS_RESP when the abort is rejected by LIO Core.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@...ro.com>
---
 drivers/scsi/qla2xxx/qla_target.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index b9d816b8341e..43eafdc765a3 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -2162,7 +2162,9 @@ static void qlt_do_tmr_work(struct work_struct *work)
 		switch (mcmd->tmr_func) {
 		case QLA_TGT_ABTS:
 			mcmd->fc_tm_rsp = FCP_TMF_REJECTED;
-			qlt_build_abts_resp_iocb(mcmd);
+			qlt_24xx_send_abts_resp(mcmd->qpair,
+						&mcmd->orig_iocb.abts,
+						FCP_TMF_REJECTED, false);
 			break;
 		case QLA_TGT_LUN_RESET:
 		case QLA_TGT_CLEAR_TS:
-- 
2.25.1





BR,
 Dmitry