Re: [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

On 08-Jun-21 15:06, Laszlo Ersek wrote:
> RFC 7143 explains that a single iSCSI session may use multiple TCP
> connections. The first connection established is called the leading
> connection. The login performed on the leading connection is called the
> leading login. Before the session is considered full-featured, the leading
> login must succeed. Further (non-leading) connections can be associated
> with the session later.
>
> (It's unclear to me from RFC 7143 whether the non-leading connections
> require individual (non-leading) logins as well, but that particular
> question is irrelevant from the perspective of this patch; see below.)
>
> The data model in IScsiDxe exhibits some confusion, regarding connection /
> session association:
>
> - On one hand, the "ISCSI_SESSION.Conns" field is a *set* (it has type
>    LIST_ENTRY), and accordingly, connections can be added to, and removed
>    from, a session, with the IScsiAttatchConnection() and
>    IScsiDetatchConnection() functions.
>
> - On the other hand, ISCSI_MAX_CONNS_PER_SESSION has value 1, therefore no
>    session will ever use more than 1 connection at a time (refer to
>    instances of "Session->MaxConnections" in
>    "NetworkPkg/IScsiDxe/IScsiProto.c").
>
> This one-to-many confusion between ISCSI_SESSION and ISCSI_CONNECTION is
> very visible in the CHAP logic, where the progress of the authentication
> is maintained *per connection*, in the "ISCSI_CONNECTION.AuthStep" field
> (with values such as ISCSI_AUTH_INITIAL, ISCSI_CHAP_STEP_ONE, etc), but
> the *data* for the authentication are maintained *per session*, in the
> "AuthType" and "AuthData" fields of ISCSI_SESSION. Clearly, this makes no
> sense if multiple connections are eligible for logging in.
>
> Knowing that IScsiDxe uses only one connection per session (put
> differently: knowing that any connection is a leading connection, and any
> login is a leading login), there is no functionality bug. But the data
> model is still broken: "AuthType", "AuthData", and "AuthStep" should be
> maintained at the *same* level -- be it "session-level" or "(leading)
> connection-level".
>
> Fixing this data model bug is more than what I'm signing up for. However,
> I do need to add one function, in preparation for multi-hash support:
> whenever a new login is attempted (put differently: whenever the leading
> login is re-attempted), which always happens with a fresh connection, the
> session-level authentication data needs to be rewound to a sane initial
> state.
>
> Introduce the IScsiSessionResetAuthData() function. Call it from the
> central -- session-level -- IScsiSessionLogin() function, just before the
> latter calls the -- connection-level -- IScsiConnLogin() function.
>
> Right now, do nothing in IScsiSessionResetAuthData(); so functionally
> speaking, the patch is a no-op.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   NetworkPkg/IScsiDxe/IScsiProto.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/NetworkPkg/IScsiDxe/IScsiProto.c b/NetworkPkg/IScsiDxe/IScsiProto.c
> index 6983f0fa5973..69d1b39dbb1f 100644
> --- a/NetworkPkg/IScsiDxe/IScsiProto.c
> +++ b/NetworkPkg/IScsiDxe/IScsiProto.c
> @@ -401,38 +401,55 @@ IScsiGetIp6NicInfo (
>     if (Ip6ModeData.GroupTable!= NULL) {
>       FreePool (Ip6ModeData.GroupTable);
>     }
>     if (Ip6ModeData.RouteTable!= NULL) {
>       FreePool (Ip6ModeData.RouteTable);
>     }
>     if (Ip6ModeData.NeighborCache!= NULL) {
>       FreePool (Ip6ModeData.NeighborCache);
>     }
>     if (Ip6ModeData.PrefixTable!= NULL) {
>       FreePool (Ip6ModeData.PrefixTable);
>     }
>     if (Ip6ModeData.IcmpTypeList!= NULL) {
>       FreePool (Ip6ModeData.IcmpTypeList);
>     }
>   
>     return Status;
>   }
>   
> +/**
> +  Re-set any stateful session-level authentication information that is used by
> +  the leading login / leading connection.
> +
> +  (Note that this driver only supports a single connection per session -- see
> +  ISCSI_MAX_CONNS_PER_SESSION.)
> +
> +  @param[in,out] Session  The iSCSI session.
> +**/
> +STATIC
> +VOID
> +IScsiSessionResetAuthData (
> +  IN OUT ISCSI_SESSION *Session
> +  )
> +{
> +}
> +
>   /**
>     Login the iSCSI session.
>   
>     @param[in]  Session           The iSCSI session.
>   
>     @retval EFI_SUCCESS           The iSCSI session login procedure finished.
>     @retval EFI_OUT_OF_RESOURCES  Failed to allocate memory.
>     @retval EFI_NO_MEDIA          There was a media error.
>     @retval Others                Other errors as indicated.
>   
>   **/
>   EFI_STATUS
>   IScsiSessionLogin (
>     IN ISCSI_SESSION  *Session
>     )
>   {
>     EFI_STATUS        Status;
>     ISCSI_CONNECTION  *Conn;
>     VOID              *Tcp;
> @@ -454,38 +471,39 @@ IScsiSessionLogin (
>     //
>     CopyMem (Session->Isid, Session->ConfigData->SessionConfigData.IsId, 6);
>   
>     RetryCount = 0;
>   
>     do {
>       //
>       // Create a connection for the session.
>       //
>       Conn = IScsiCreateConnection (Session);
>       if (Conn == NULL) {
>         return EFI_OUT_OF_RESOURCES;
>       }
>   
>       IScsiAttatchConnection (Session, Conn);
>   
>       //
>       // Login through the newly created connection.
>       //
> +    IScsiSessionResetAuthData (Session);
>       Status = IScsiConnLogin (Conn, Session->ConfigData->SessionConfigData.ConnectTimeout);
>       if (EFI_ERROR (Status)) {
>         IScsiConnReset (Conn);
>         IScsiDetatchConnection (Conn);
>         IScsiDestroyConnection (Conn);
>       }
>   
>       if (Status != EFI_TIMEOUT) {
>         break;
>       }
>   
>       RetryCount++;
>     } while (RetryCount <= Session->ConfigData->SessionConfigData.ConnectRetryCount);
>   
>     if (!EFI_ERROR (Status)) {
>       Session->State = SESSION_STATE_LOGGED_IN;
>   
>       if (!Conn->Ipv6Flag) {
>         ProtocolGuid = &gEfiTcp4ProtocolGuid;