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

[Petr Pavlu <petr.pavlu@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

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@suse.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

[-- Attachment #2: target-test.py --]
[-- Type: text/x-python, Size: 4998 bytes --]

#!/usr/bin/python3

import socket

# Test excess number of exchanges during security negotiation.
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s.connect(('192.168.122.30', 3260))
    send = (
        b'\x43'                      # 00: -> Immediate marker + Login request
        b'\x00'                      # 01: -> Continue security negotiation
        b'\x00'                      # 02: VersionMax
        b'\x00'                      # 03: VersionMin
        b'\x00'                      # 04: TotalAHSLength
        b'\x00\x00\x8c'              # 05: DataSegmentLength
        b'\x0c\xfd\x37\x00\x00\x01'  # 08: ISID -> OUI, SUSE, Qualifier=1
        b'\x00\x00'                  # 14: TSIH
        b'\x00\x00\x00\x00'          # 16: InitiatorTaskTag
        b'\x00\x00'                  # 20: CID
        b'\x00\x00'                  # 22: Reserved
        b'\x00\x00\x00\x00'          # 24: CmdSN
        b'\x00\x00\x00\x00'          # 28: ExpStatSN
        b'\x00\x00\x00\x00'          # 32: Reserved
        b'\x00\x00\x00\x00'          # 36: Reserved
        b'\x00\x00\x00\x00'          # 40: Reserved
        b'\x00\x00\x00\x00'          # 44: Reserved
        # 48: Login parameters
        b'InitiatorName=iqn.1996-04.de.suse:01:6fd07434abf9\x00'
        b'SessionType=Normal\x00'
        b'TargetName=iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.8726b6baa6b9\x00'
        # 188: Padding to 4 bytes
        b''
        # 188:
    )
    for i in range(1, 8):
        print(f"Send #{i}: {send}")
        s.sendall(send)
        data = s.recv(1024)
        print(f"Response: {data}")
        assert data[0] == 0x23  # Opcode
        assert data[36] == (0 if i < 7 else 3)  # Status-Class
        assert data[37] == 0  # Status-Detail

print()

# Test excess number of exchanges during operation parameter negotiation.
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
    s.connect(('192.168.122.30', 3260))
    send = (
        b'\x43'                      # 00: -> Immediate marker + Login request
        b'\x81'                      # 01: -> Transfer to operational parameter negotiation
        b'\x00'                      # 02: VersionMax
        b'\x00'                      # 03: VersionMin
        b'\x00'                      # 04: TotalAHSLength
        b'\x00\x00\x9c'              # 05: DataSegmentLength
        b'\x0c\xfd\x37\x00\x00\x01'  # 08: ISID -> OUI, SUSE, Qualifier=1
        b'\x00\x00'                  # 14: TSIH
        b'\x00\x00\x00\x00'          # 16: InitiatorTaskTag
        b'\x00\x00'                  # 20: CID
        b'\x00\x00'                  # 22: Reserved
        b'\x00\x00\x00\x00'          # 24: CmdSN
        b'\x00\x00\x00\x00'          # 28: ExpStatSN
        b'\x00\x00\x00\x00'          # 32: Reserved
        b'\x00\x00\x00\x00'          # 36: Reserved
        b'\x00\x00\x00\x00'          # 40: Reserved
        b'\x00\x00\x00\x00'          # 44: Reserved
        # 48: Login parameters
        b'InitiatorName=iqn.1996-04.de.suse:01:6fd07434abf9\x00'
        b'SessionType=Normal\x00'
        b'TargetName=iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.8726b6baa6b9\x00'
        b'AuthMethod=None\x00'
        # 204: Padding to 4 bytes
        b''
        # 204:
    )
    print(f"Send #1: {send}")
    s.sendall(send)
    data = s.recv(1024)
    print(f"Response: {data}")
    assert data[0] == 0x23  # Opcode
    assert data[1] == 0x81  # -> Transfer to operational parameter negotiation
    assert data[36] == 0  # Status-Class
    assert data[37] == 0  # Status-Detail

    send = (
        b'\x43'                      # 00: -> Immediate marker + Login request
        b'\x04'                      # 01: -> Continue operational parameter negotiation
        b'\x00'                      # 02: VersionMax
        b'\x00'                      # 03: VersionMin
        b'\x00'                      # 04: TotalAHSLength
        b'\x00\x00\x12'              # 05: DataSegmentLength
        b'\x0c\xfd\x37\x00\x00\x01'  # 08: ISID -> OUI, SUSE, Qualifier=1
        b'\x00\x00'                  # 14: TSIH
        b'\x00\x00\x00\x00'          # 16: InitiatorTaskTag
        b'\x00\x00'                  # 20: CID
        b'\x00\x00'                  # 22: Reserved
        b'\x00\x00\x00\x00'          # 24: CmdSN
        b'\x00\x00\x00\x00'          # 28: ExpStatSN
        b'\x00\x00\x00\x00'          # 32: Reserved
        b'\x00\x00\x00\x00'          # 36: Reserved
        b'\x00\x00\x00\x00'          # 40: Reserved
        b'\x00\x00\x00\x00'          # 44: Reserved
        # 48: Login parameters
        b'HeaderDigest=None\x00'
        # 66: Padding to 4 bytes
        b'\x00\x00'
        # 68:
    )
    for i in range(2, 9):
        print(f"Send #{i}: {send}")
        s.sendall(send)
        data = s.recv(1024)
        print(f"Response: {data}")
        assert data[0] == 0x23
        assert data[36] == (0 if i < 8 else 3)  # Status-Class
        assert data[37] == 0  # Status-Detail