linux-kernel - Re: [PATCH] target/iscsi: Fix detection of excess number of login exchanges

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: <7650feb7-264c-b142-d8f0-23576ffe7d29@suse.com>
Date:   Fri, 11 Mar 2022 10:15:01 +0100
From:   Petr Pavlu <petr.pavlu@...e.com>
To:     Mike Christie <michael.christie@...cle.com>
Cc:     linux-scsi@...r.kernel.org, target-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Petr Pavlu <ppavlu@...e.cz>,
        martin.petersen@...cle.com
Subject: Re: [PATCH] target/iscsi: Fix detection of excess number of login
 exchanges

On 3/1/22 2:23 AM, Mike Christie wrote:
> On 2/22/22 6:42 AM, Petr Pavlu wrote:
>> From: Petr Pavlu <petr.pavlu@...e.com>
>>
>> Function iscsi_target_do_login() attempts to cancel a connection when
>> a number of login exchanges reaches MAX_LOGIN_PDUS (7). This is done by
>> having a local counter and incrementing+checking it as the function
>> processes requests in a loop. A problem is that since the login rework in
>> back in 2013, the function always processes only a single request and the
>> loop is terminated at the end of the first iteration. This means the
>> counter reaches only value 1 and any excess number of login requests is
>> never rejected.
>>
>> Fix the problem by introducing iscsi_login.negotiation_exchanges counter
>> and update the logic to count exchanges per each login phase as described
>> in RFC 7143:
>>> 6.2. Text Mode Negotiation:
>>> [...]
>>> In the Login Phase (see Section 6.3), every stage is a separate
>>> negotiation. [...]
>>> [...]
>>> An iSCSI initiator or target MAY terminate a negotiation that does
>>> not terminate within an implementation-specific reasonable time or
>>> number of exchanges but SHOULD allow at least six (6) exchanges.
>>
> 
> It wasn't clear to me what this fixes. Today, are initiators sending more
> than 6 exchanges and if so what happens to the target? Is it crashing or
> annoying to user or cause some sort of endless login so we run out of
> resources? Or is this more of code cleanup?
> 
> When does this happen and with what initiators?

This issue is only something that I noticed while reading through the target
code because of some different problem. In that sense, the patch is more
a code cleanup. My tests to verify the patch were also artificial (attached).

I have now additionally tried some simple examples with sending extensive
number of Login requests in a loop to the target and did not observe any
immediate problem with running out of resources. A possible alternative might
be therefore to remove this logic, not sure.

Thanks,
Petr

View attachment "target-test.py" of type "text/x-python" (4999 bytes)