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] [thread-next>] [day] [month] [year] [list]

Message-ID: <526d0d1b-79f1-47fd-bc44-6727898f381c@cybernetics.com>
Date: Tue, 16 Sep 2025 12:04:11 -0400
From: Tony Battersby <tonyb@...ernetics.com>
To: Dmitry Bogdanov <d.bogdanov@...ro.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 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.

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.

Tony