IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the
digest (16) that it solely supports at this point (MD5).
ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related
buffers (binary buffers and hex encodings alike), and (b) *processing*
binary digest buffers (comparing them, filling them, reading them).
In preparation for adding other hash algorithms, split purpose (a) from
purpose (b). For purpose (a) -- buffer allocation --, introduce
ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on
MD5_DIGEST_SIZE from <BaseCryptLib.h>.
Distinguishing these purposes is justified because purpose (b) --
processing -- must depend on the hashing algorithm negotiated between
initiator and target, while for purpose (a) -- allocation --, using the
maximum supported digest size is suitable. For now, because only MD5 is
supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE.
Note that the argument for using the digest size as the size of the
outgoing challenge (in case mutual authentication is desired by the
initiator) remains in place. Because of this, the above two purposes are
distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well.
This patch is functionally a no-op, just yet.
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/IScsiCHAP.h | 17 +++++++++------
NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++----------
2 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index d6a90fc27fc3..b8811b7580f0 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -1,86 +1,91 @@
/** @file
The header file of CHAP configuration.
Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_
#define ISCSI_AUTH_METHOD_CHAP "CHAP"
#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
#define ISCSI_KEY_CHAP_NAME "CHAP_N"
#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"
+//
+// Identifiers of supported CHAP hash algorithms:
+// https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
+//
#define ISCSI_CHAP_ALGORITHM_MD5 5
-///
-/// MD5_HASHSIZE
-///
-#define ISCSI_CHAP_RSP_LEN 16
+//
+// Byte count of the largest digest over the above-listed
+// ISCSI_CHAP_ALGORITHM_* hash algorithms.
+//
+#define ISCSI_CHAP_MAX_DIGEST_SIZE MD5_DIGEST_SIZE
#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4
#pragma pack(1)
typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
} ISCSI_CHAP_AUTH_CONFIG_NVDATA;
#pragma pack()
///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
UINT32 InIdentifier;
UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
- UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];
+ UINT8 CHAPResponse[ISCSI_CHAP_MAX_DIGEST_SIZE];
//
// Auth-data to be sent out for mutual authentication.
//
// While the challenge size is technically independent of the hashing
// algorithm, it is good practice to avoid hashing *fewer bytes* than the
// digest size. In other words, it's good practice to feed *at least as many
// bytes* to the hashing algorithm as the hashing algorithm will output.
//
UINT32 OutIdentifier;
- UINT8 OutChallenge[ISCSI_CHAP_RSP_LEN];
+ UINT8 OutChallenge[ISCSI_CHAP_MAX_DIGEST_SIZE];
} ISCSI_CHAP_AUTH_DATA;
/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@param[in] Conn The iSCSI connection.
@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.
**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
);
/**
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index bb84f4359d35..744824e63d23 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -96,90 +96,90 @@ IScsiCHAPCalculateResponse (
@param[in] AuthData iSCSI CHAP authentication data.
@param[in] TargetResponse The response from target.
@retval EFI_SUCCESS The response from target passed
authentication.
@retval EFI_SECURITY_VIOLATION The response from target was not expected
value.
@retval Others Other errors as indicated.
**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
- UINT8 VerifyRsp[ISCSI_CHAP_RSP_LEN];
+ UINT8 VerifyRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];
Status = EFI_SUCCESS;
SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN, // ChallengeLength
+ MD5_DIGEST_SIZE, // ChallengeLength
VerifyRsp
);
- if (CompareMem (VerifyRsp, TargetResponse, ISCSI_CHAP_RSP_LEN) != 0) {
+ if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) != 0) {
Status = EFI_SECURITY_VIOLATION;
}
return Status;
}
/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@param[in] Conn The iSCSI connection.
@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.
**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
UINT8 *Data;
UINT32 Len;
LIST_ENTRY *KeyValueList;
UINTN Algorithm;
CHAR8 *Identifier;
CHAR8 *Challenge;
CHAR8 *Name;
CHAR8 *Response;
- UINT8 TargetRsp[ISCSI_CHAP_RSP_LEN];
+ UINT8 TargetRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];
UINT32 RspLen;
UINTN Result;
ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);
ASSERT (Conn->RspQue.BufNum != 0);
Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
Len = Conn->RspQue.BufSize;
Data = AllocateZeroPool (Len);
if (Data == NULL) {
return EFI_OUT_OF_RESOURCES;
}
//
// Copy the data in case the data spans over multiple PDUs.
//
NetbufQueCopy (&Conn->RspQue, 0, Len, Data);
//
@@ -324,41 +324,41 @@ IScsiCHAPOnRspReceived (
case ISCSI_CHAP_STEP_FOUR:
ASSERT (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL);
//
// The forth step, CHAP_N=<N> CHAP_R=<R> is received from Target.
//
Name = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_NAME);
if (Name == NULL) {
goto ON_EXIT;
}
Response = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_RESPONSE
);
if (Response == NULL) {
goto ON_EXIT;
}
- RspLen = ISCSI_CHAP_RSP_LEN;
+ RspLen = MD5_DIGEST_SIZE;
Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
- if (EFI_ERROR (Status) || RspLen != ISCSI_CHAP_RSP_LEN) {
+ if (EFI_ERROR (Status) || RspLen != MD5_DIGEST_SIZE) {
Status = EFI_PROTOCOL_ERROR;
goto ON_EXIT;
}
//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;
default:
break;
}
ON_EXIT:
if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}
@@ -395,45 +395,45 @@ IScsiCHAPToSendReq (
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
UINT32 ChallengeLen;
EFI_STATUS BinToHexStatus;
ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);
Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
LoginReq = (ISCSI_LOGIN_REQUEST *) NetbufGetByte (Pdu, 0, 0);
if (LoginReq == NULL) {
return EFI_PROTOCOL_ERROR;
}
Status = EFI_SUCCESS;
- RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
+ RspLen = 2 * ISCSI_CHAP_MAX_DIGEST_SIZE + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}
- ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
+ ChallengeLen = 2 * ISCSI_CHAP_MAX_DIGEST_SIZE + 3;
Challenge = AllocateZeroPool (ChallengeLen);
if (Challenge == NULL) {
FreePool (Response);
return EFI_OUT_OF_RESOURCES;
}
switch (Conn->AuthStep) {
case ISCSI_AUTH_INITIAL:
//
// It's the initial Login Request. Fill in the key=value pairs mandatory
// for the initial Login Request.
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_INITIATOR_NAME,
mPrivate->InitiatorName
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
IScsiAddKeyValuePair (
@@ -466,59 +466,59 @@ IScsiCHAPToSendReq (
case ISCSI_CHAP_STEP_THREE:
//
// Third step, send the Login Request with CHAP_N=<N> CHAP_R=<R> or
// CHAP_N=<N> CHAP_R=<R> CHAP_I=<I> CHAP_C=<C> if target authentication is
// required too.
//
// CHAP_N=<N>
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_CHAP_NAME,
(CHAR8 *) &AuthData->AuthConfig->CHAPName
);
//
// CHAP_R=<R>
//
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
- ISCSI_CHAP_RSP_LEN,
+ MD5_DIGEST_SIZE,
Response,
&RspLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
- IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
+ IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, MD5_DIGEST_SIZE);
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN,
+ MD5_DIGEST_SIZE,
Challenge,
&ChallengeLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;
default:
Status = EFI_PROTOCOL_ERROR;
break;
}
--
2.19.1.3.g30247aa5d201
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76216): https://edk2.groups.io/g/devel/message/76216
Mute This Topic: https://groups.io/mt/83395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo, On 6/8/21 3:06 PM, Laszlo Ersek wrote: > IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the > digest (16) that it solely supports at this point (MD5). > ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related > buffers (binary buffers and hex encodings alike), and (b) *processing* > binary digest buffers (comparing them, filling them, reading them). > > In preparation for adding other hash algorithms, split purpose (a) from > purpose (b). For purpose (a) -- buffer allocation --, introduce > ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on > MD5_DIGEST_SIZE from <BaseCryptLib.h>. Matter of taste probably, I'd rather see this patch split in 2, as you identified. (b) first then (a). Regardless: Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > Distinguishing these purposes is justified because purpose (b) -- > processing -- must depend on the hashing algorithm negotiated between > initiator and target, while for purpose (a) -- allocation --, using the > maximum supported digest size is suitable. For now, because only MD5 is > supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE. > > Note that the argument for using the digest size as the size of the > outgoing challenge (in case mutual authentication is desired by the > initiator) remains in place. Because of this, the above two purposes are > distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well. > > This patch is functionally a no-op, just yet. > > 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/IScsiCHAP.h | 17 +++++++++------ > NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++---------- > 2 files changed, 22 insertions(+), 17 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76261): https://edk2.groups.io/g/devel/message/76261 Mute This Topic: https://groups.io/mt/83395026/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 06/09/21 12:43, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 6/8/21 3:06 PM, Laszlo Ersek wrote: >> IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the >> digest (16) that it solely supports at this point (MD5). >> ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related >> buffers (binary buffers and hex encodings alike), and (b) *processing* >> binary digest buffers (comparing them, filling them, reading them). >> >> In preparation for adding other hash algorithms, split purpose (a) from >> purpose (b). For purpose (a) -- buffer allocation --, introduce >> ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on >> MD5_DIGEST_SIZE from <BaseCryptLib.h>. > > Matter of taste probably, I'd rather see this patch split in 2, as you > identified. (b) first then (a). Regardless: > Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in one patch, and classifying each use one way or another at once, was the best for reviewer understanding. Basically it's a single "mental loop" over all uses, and in the "loop body", we have an "if" (classification) -- allocation vs. processing. What you propose is basically "two loops". In that approach, in the first patch (= the first "mental loop"), only "processing" uses would be updated; the "allocation sites" wouldn't be shown at all. I feel that this approach is counter-intuitive: From the body of the first patch, - the reviewer can check the *correctness* of the patch (that is, whether everything that I converted is indeed "processing"), - but they can't check the *completeness* of the patch (that is, whether there is a "processing" site that I should have converted, but missed). For the reviewer to verify the completeness of the first patch, they have to apply it (or check out the branch at that stage), and go over all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed something. And, if the reviewer has to check every single instance of ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the same work as if they had just reviewed this particular patch. I think your approach boils down to the following idea: The completeness of the first patch would be proved by the correctness of the second patch. That is, *after* you reviewed the second patch (and see that every site converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN instance remains), you could be sure that no processing site was missed in the first patch. Technically / mathematically, this is entirely true; I just prefer avoiding situations where you have to review patch (N+X) to see the validity (completeness) of patch (N). I quite dislike jumping between patches during review. Does my explanation make sense? If you still prefer the split, I'm OK to do it. Thanks! Laszlo > >> Distinguishing these purposes is justified because purpose (b) -- >> processing -- must depend on the hashing algorithm negotiated between >> initiator and target, while for purpose (a) -- allocation --, using the >> maximum supported digest size is suitable. For now, because only MD5 is >> supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE. >> >> Note that the argument for using the digest size as the size of the >> outgoing challenge (in case mutual authentication is desired by the >> initiator) remains in place. Because of this, the above two purposes are >> distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well. >> >> This patch is functionally a no-op, just yet. >> >> 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/IScsiCHAP.h | 17 +++++++++------ >> NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++---------- >> 2 files changed, 22 insertions(+), 17 deletions(-) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76275): https://edk2.groups.io/g/devel/message/76275 Mute This Topic: https://groups.io/mt/83395026/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
To cut to the chase on this patch: Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> On 09-Jun-21 15:46, Laszlo Ersek wrote: > On 06/09/21 12:43, Philippe Mathieu-Daudé wrote: >> Hi Laszlo, >> >> On 6/8/21 3:06 PM, Laszlo Ersek wrote: >>> IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the >>> digest (16) that it solely supports at this point (MD5). >>> ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related >>> buffers (binary buffers and hex encodings alike), and (b) *processing* >>> binary digest buffers (comparing them, filling them, reading them). >>> >>> In preparation for adding other hash algorithms, split purpose (a) from >>> purpose (b). For purpose (a) -- buffer allocation --, introduce >>> ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on >>> MD5_DIGEST_SIZE from <BaseCryptLib.h>. >> Matter of taste probably, I'd rather see this patch split in 2, as you >> identified. (b) first then (a). Regardless: >> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> > Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in > one patch, and classifying each use one way or another at once, was the > best for reviewer understanding. Basically it's a single "mental loop" > over all uses, and in the "loop body", we have an "if" (classification) > -- allocation vs. processing. > > What you propose is basically "two loops". In that approach, in the > first patch (= the first "mental loop"), only "processing" uses would be > updated; the "allocation sites" wouldn't be shown at all. I feel that > this approach is counter-intuitive: > > From the body of the first patch, > > - the reviewer can check the *correctness* of the patch (that is, > whether everything that I converted is indeed "processing"), > > - but they can't check the *completeness* of the patch (that is, whether > there is a "processing" site that I should have converted, but missed). > > For the reviewer to verify the completeness of the first patch, they > have to apply it (or check out the branch at that stage), and go over > all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed > something. And, if the reviewer has to check every single instance of > ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the > same work as if they had just reviewed this particular patch. > > I think your approach boils down to the following idea: > > The completeness of the first patch would be proved by the correctness > of the second patch. > > That is, *after* you reviewed the second patch (and see that every site > converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN > macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN > instance remains), you could be sure that no processing site was missed > in the first patch. > > Technically / mathematically, this is entirely true; I just prefer > avoiding situations where you have to review patch (N+X) to see the > validity (completeness) of patch (N). I quite dislike jumping between > patches during review. > > Does my explanation make sense? > > If you still prefer the split, I'm OK to do it. > > Thanks! > Laszlo > >>> Distinguishing these purposes is justified because purpose (b) -- >>> processing -- must depend on the hashing algorithm negotiated between >>> initiator and target, while for purpose (a) -- allocation --, using the >>> maximum supported digest size is suitable. For now, because only MD5 is >>> supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE. >>> >>> Note that the argument for using the digest size as the size of the >>> outgoing challenge (in case mutual authentication is desired by the >>> initiator) remains in place. Because of this, the above two purposes are >>> distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well. >>> >>> This patch is functionally a no-op, just yet. >>> >>> 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/IScsiCHAP.h | 17 +++++++++------ >>> NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++---------- >>> 2 files changed, 22 insertions(+), 17 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76387): https://edk2.groups.io/g/devel/message/76387 Mute This Topic: https://groups.io/mt/83395026/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.