Date   

Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

Min Xu
 

On 06/04/2021 12:12 AM, Laszlo wrote:

(22) EmuVariableFvbRuntimeDxe

Ouch, this is an unpleasant surprise.

First, if you know for a fact that pflash is not part of the *board* in any TDX
setup, then pulling

OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

into the firmware platform is useless, as it is mutually exclusive with

OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf

(via dynamic means -- a dynamic PCD).

Note that the FDF file places QemuFlashFvbServicesRuntimeDxe in APRIORI
DXE when SMM_REQUIRE is FALSE. This driver checks for pflash presence,
and lets EmuVariableFvbRuntimeDxe perform its in-RAM flash emulation
only in case pflash is not found.
Right, pflash is not part of the *board* in any TDX setup.
This is a limitation from the Qemu (the tdx-qemu) side. TDVF is copied into
RAM. Generic loader is the one for it.
In this case (pflash is not found), FvbServiceRuntimeDxe.inf will return
EFI_WRITE_PROTECTED in its FvbInitialize().
EmuVariableFvbRuntimeDxe will then perform its in-RAM flash emulation.

So this is again in favor of a separate platform -- if we know pflash is never
part of the board, then QemuFlashFvbServicesRuntimeDxe is never needed,
but you cannot remove it from the traditional DSC/FDF files.
Yes, if TDVF is a separate DSD/FDF, QemuFlashFvbServicesRuntimeDxe will be
removed from the image.

Second, EmuVariableFvbRuntimeDxe consumes the PlatformFvbLib class, for
the PlatformFvbDataWritten() API (among other things). This lib class is
implemented by two instances in OvmfPkg, PlatformFvbLibNull and
EmuVariableFvbLib. The latter instance allows Platform BDS to hook an event
(for signaling) via "PcdEmuVariableEvent" into the
EmuVariableFvbRuntimeDxe driver.

In old (very old) QEMU board configurations, namely those without pflash,
this (mis)feature is used by OVMF's PlatformBootManagerLib to write out all
variables to the EFI system partition in a regular file called \NvVars, with the
help of NvVarsFileLib, whenever EmuVariableFvbRuntimeDxe writes out an
emulated "flash" block. For this purpose, the traditional OVMF DSC files link
EmuVariableFvbLib into EmuVariableFvbRuntimeDxe.
Thanks for the explanation. It's very helpful for me to understand the code.

But it counts as an absolute disaster nowadays, and should not be revived in
any platform. If you don't have pflash in TDX guests, just accept that you
won't have non-volatile variables. And link PlatformFvbLibNull into
EmuVariableFvbRuntimeDxe. You're going to need a separate
PlatformBootManagerLib instance anyway.
I have a question here, that if PlatformFvbLibNull is linked into EmuVaiableFvRuntimeDxe,
Does it mean it cannot write to the in-RAM variable store?


(We should have removed EmuVariableFvbRuntimeDxe a long time ago from
the traditional OVMF platforms, i.e. made pflash a hard requirement, even
when SMM is not built into the platform -- but whenever I tried that, Jordan
always shot me down.)
I am afraid in TDVF we have to use EmuVariableFvRuntimeDxe to emulate the
in-RAM, as I explained pflash is not part of the *board* in TDX setup.
In the traditional EmuVariableFvRuntimeDxe, the FV contents will be initialized.
But TDVF has its own requirement, that the SB keys in CFV need to be copied
into the FV contents. That's why EmuVariableFvRuntimeDxe is updated in
TDVF project.


My point is: using EmuVariableFvbRuntimeDxe in the TDX platform may be
defensible per se, but we must be very clear that it will never provide a
standards-conformant service for non-volatile UEFI variables, and we must
keep as much of the \NvVars mess out of EmuVariableFvbRuntimeDxe as
possible. This will require a separate PlatformBootManagerLib instance for
TDX anyway (or maybe share PlatformBootManagerLibGrub with
"OvmfPkg/AmdSev/AmdSevX64.dsc").

Apart from the volatility aspect, let's assume we have this in-RAM emulated
"flash device", storing authenticated UEFI variables for Secure Boot purposes.
And we don't have SMM.

What protects this in-RAM variable store from tampering by the guest OS?
It's all guest RAM only, after all. What provides the privilege barrier between
the guest firmware and the guest OS?
A good question and we will answer it a bit later. Thanks for your patience.

Thanks
Laszlo


[PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values

Laszlo Ersek
 

IScsiDxe (that is, the initiator) receives two hex-encoded strings from
the iSCSI target:

- CHAP_C, where the target challenges the initiator,

- CHAP_R, where the target answers the challenge from the initiator (in
case the initiator wants mutual authentication).

Accordingly, we have two IScsiHexToBin() call sites:

- At the CHAP_C decoding site, check whether the decoding succeeds. The
decoded buffer ("AuthData->InChallenge") can accommodate 1024 bytes,
which is a permissible restriction on the target, per
<https://tools.ietf.org/html/rfc7143#section-12.1.3>. Shorter challenges
from the target are acceptable.

- At the CHAP_R decoding site, enforce that the decoding both succeed, and
provide exactly ISCSI_CHAP_RSP_LEN bytes. CHAP_R contains the digest
calculated by the target, therefore it must be of fixed size. We may
only call IScsiCHAPAuthTarget() if "TargetRsp" has been fully populated.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index dbe3c8ef46f9..7e930c0d1eab 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -274,43 +274,47 @@ IScsiCHAPOnRspReceived (

Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}

AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
- IScsiHexToBin (
- (UINT8 *) AuthData->InChallenge,
- &AuthData->InChallengeLength,
- Challenge
- );
+ Status = IScsiHexToBin (
+ (UINT8 *) AuthData->InChallenge,
+ &AuthData->InChallengeLength,
+ Challenge
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto ON_EXIT;
+ }
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);

//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;

case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
@@ -321,39 +325,43 @@ 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;
- IScsiHexToBin (TargetRsp, &RspLen, Response);
+ Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
+ if (EFI_ERROR (Status) || RspLen != ISCSI_CHAP_RSP_LEN) {
+ 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);
}

FreePool (Data);

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow

Laszlo Ersek
 

The IScsiHexToBin() function documents the EFI_BUFFER_TOO_SMALL return
condition, but never actually checks whether the decoded buffer fits into
the caller-provided room (i.e., the input value of "BinLength"), and
EFI_BUFFER_TOO_SMALL is never returned. The decoding of "HexStr" can
overflow "BinBuffer".

This is remotely exploitable, as shown in a subsequent patch, which adds
error checking to the IScsiHexToBin() call sites. This issue allows the
target to compromise the initiator.

Introduce EFI_BAD_BUFFER_SIZE, in addition to the existent
EFI_BUFFER_TOO_SMALL, for reporting a special case of the buffer overflow,
plus actually catch the buffer overflow.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 3 +++
NetworkPkg/IScsiDxe/IScsiMisc.c | 20 +++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 404a482e57f3..fddef4f466dc 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -156,38 +156,41 @@ IScsiAsciiStrToIp (
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index f0f4992b07c7..406954786751 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -361,89 +361,103 @@ IScsiBinToHex (
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
+ UINTN BinLengthMin;
+ UINT32 BinLengthProvided;
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}

Length = AsciiStrLen (HexStr);

//
// Reject an empty hex string; reject a stray nibble.
//
if (Length == 0 || Length % 2 != 0) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // Check if the caller provides enough room for the decoded blob.
+ //
+ BinLengthMin = Length / 2;
+ if (BinLengthMin > MAX_UINT32) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ BinLengthProvided = *BinLength;
+ *BinLength = (UINT32)BinLengthMin;
+ if (BinLengthProvided < BinLengthMin) {
+ return EFI_BUFFER_TOO_SMALL;
+ }

for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
// Invalid Hex Char.
//
return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}
-
- *BinLength = (UINT32) ((Index + 1)/2);
-
return EFI_SUCCESS;
}


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
UINTN
IScsiNetNtoi (
IN CHAR8 *Str
)
{
if ((Str[0] == '0') && ((Str[1] == 'x') || (Str[1] == 'X'))) {
Str += 2;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing

Laszlo Ersek
 

The IScsiHexToBin() function has the following parser issues:

(1) If the *subject sequence* in "HexStr" is empty, the function returns
EFI_SUCCESS (with "BinLength" set to 0 on output). Such inputs should
be rejected.

(2) The function mis-handles a "HexStr" that ends with a stray nibble. For
example, if "HexStr" is "0xABC", the function decodes it to the bytes
{0xAB, 0x0C}, sets "BinLength" to 2 on output, and returns
EFI_SUCCESS. Such inputs should be rejected.

(3) If an invalid hex char is found in "HexStr", the function treats it as
end-of-hex-string, and returns EFI_SUCCESS. Such inputs should be
rejected.

All of the above cases are remotely triggerable, as shown in a subsequent
patch, which adds error checking to the IScsiHexToBin() call sites. While
the initiator is not immediately compromised, incorrectly parsing CHAP_R
from the target, in case of mutual authentication, is not great.

Extend the interface contract of IScsiHexToBin() with
EFI_INVALID_PARAMETER, for reporting issues (1) through (3), and implement
the new checks.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 28cf408cd5c5..404a482e57f3 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -155,38 +155,39 @@ IScsiAsciiStrToIp (

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 014700e87a5f..f0f4992b07c7 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -360,72 +360,80 @@ IScsiBinToHex (
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}

Length = AsciiStrLen (HexStr);

+ //
+ // Reject an empty hex string; reject a stray nibble.
+ //
+ if (Length == 0 || Length % 2 != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
- // Invalid Lun Char.
+ // Invalid Hex Char.
//
- break;
+ return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}

*BinLength = (UINT32) ((Index + 1)/2);

return EFI_SUCCESS;
}


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds

Laszlo Ersek
 

IScsiBinToHex() is called for encoding:

- the answer to the target's challenge; that is, CHAP_R;

- the challenge for the target, in case mutual authentication is enabled;
that is, CHAP_C.

The initiator controls the size of both blobs, the sizes of their hex
encodings are correctly calculated in "RspLen" and "ChallengeLen".
Therefore the IScsiBinToHex() calls never fail; assert that.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 27 +++++++++++---------
1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 9e192ce292e8..dbe3c8ef46f9 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -375,38 +375,39 @@ IScsiCHAPOnRspReceived (
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.

**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_LOGIN_REQUEST *LoginReq;
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;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}

ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Challenge = AllocateZeroPool (ChallengeLen);
@@ -455,63 +456,65 @@ IScsiCHAPToSendReq (
Conn->AuthStep = ISCSI_CHAP_STEP_TWO;
break;

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>
//
- IScsiBinToHex (
- (UINT8 *) AuthData->CHAPResponse,
- ISCSI_CHAP_RSP_LEN,
- Response,
- &RspLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->CHAPResponse,
+ ISCSI_CHAP_RSP_LEN,
+ 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);
- IScsiBinToHex (
- (UINT8 *) AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN,
- Challenge,
- &ChallengeLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->OutChallenge,
+ ISCSI_CHAP_RSP_LEN,
+ 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;
}

FreePool (Response);
FreePool (Challenge);

return Status;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block

Laszlo Ersek
 

We'll need further return values for IScsiHexToBin() in a subsequent
patch; make room for them in the leading comment block of the function.
While at it, rewrap the comment block to 80 characters width.

No functional changes.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 14 +++++++-------
NetworkPkg/IScsiDxe/IScsiMisc.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 231413993b08..28cf408cd5c5 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -149,46 +149,46 @@ IScsiAsciiStrToIp (

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
@retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.

+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
UINTN
IScsiNetNtoi (
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 42988e15cb06..014700e87a5f 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -354,46 +354,46 @@ IScsiBinToHex (
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';

for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.

+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies

Laszlo Ersek
 

Sort the library class dependencies in the #include directives and in the
INF file. Remove the DpcLib class from the #include directives -- it is
not listed in the INF file, and IScsiDxe doesn't call either DpcLib API
(QueueDpc(), DispatchDpc()). No functional changes.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 6 +++---
NetworkPkg/IScsiDxe/IScsiImpl.h | 17 ++++++++---------
2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 0ffb340ce05e..543c4083026a 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -49,53 +49,53 @@ [Sources]
IScsiDriver.c
IScsiDriver.h
IScsiExtScsiPassThru.c
IScsiIbft.c
IScsiIbft.h
IScsiInitiatorName.c
IScsiImpl.h
IScsiMisc.c
IScsiMisc.h
IScsiProto.c
IScsiProto.h

[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec

[LibraryClasses]
+ BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
- TcpIoLib
PrintLib
+ TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
+ UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib
- UefiHiiServicesLib
- BaseCryptLib

[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp4Config2ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp6ConfigProtocolGuid ## SOMETIMES_CONSUMES
gEfiTcp4ProtocolGuid ## TO_START
gEfiTcp6ProtocolGuid ## TO_START
gEfiTcp4ServiceBindingProtocolGuid ## TO_START
gEfiTcp6ServiceBindingProtocolGuid ## TO_START
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index 387ab9765e9e..d895c7feb947 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -19,53 +19,52 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/DevicePath.h>
#include <Protocol/HiiConfigAccess.h>

#include <Protocol/Ip6.h>
#include <Protocol/Dhcp4.h>
#include <Protocol/Dhcp6.h>
#include <Protocol/Dns4.h>
#include <Protocol/Dns6.h>
#include <Protocol/Tcp4.h>
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>

#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>

-#include <Library/HiiLib.h>
-#include <Library/UefiHiiServicesLib.h>
-#include <Library/DevicePathLib.h>
-#include <Library/DebugLib.h>
+#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
-#include <Library/DpcLib.h>
-#include <Library/NetLib.h>
-#include <Library/TcpIoLib.h>
-#include <Library/BaseCryptLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>

#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>

#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"

#include "IScsiIbft.h"
#include "IScsiMisc.h"
#include "IScsiDns.h"
#include "IScsiConfig.h"

#define ISCSI_AUTH_INITIAL 0

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()

Laszlo Ersek
 

Considering IScsiBinToHex():

if (((*HexLength) - 3) < BinLength * 2) {
*HexLength = BinLength * 2 + 3;
}
the following subexpressions are problematic:

(*HexLength) - 3
BinLength * 2
BinLength * 2 + 3

The first one may wrap under zero, the latter two may wrap over
MAX_UINT32.

Rewrite the calculation using SafeIntLib.

While at it, change the type of the "Index" variable from UINTN to UINT32.
The largest "Index"-based value that we calculate is

Index * 2 + 2 (with (Index == BinLength))

Because the patch makes

BinLength * 2 + 3

safe to calculate in UINT32, using UINT32 for

Index * 2 + 2 (with (Index == BinLength))

is safe too. Consistently using UINT32 improves readability.

This patch is best reviewed with "git show -W".

The integer overflows that this patch fixes are theoretical; a subsequent
patch in the series will audit the IScsiBinToHex() call sites, and show
that none of them can fail.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 1 +
NetworkPkg/IScsiDxe/IScsiImpl.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 19 +++++++++++++++----
4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 543c4083026a..1dde56d00ca2 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -58,38 +58,39 @@ [Sources]
IScsiProto.c
IScsiProto.h

[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec

[LibraryClasses]
BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
PrintLib
+ SafeIntLib
TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib

[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index d895c7feb947..ac3a25730efb 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -28,38 +28,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>

#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>

#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>

#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>

#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"

#include "IScsiIbft.h"
#include "IScsiMisc.h"
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 46c725aab3a4..231413993b08 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -134,38 +134,39 @@ IScsiMacAddrToStr (
**/
EFI_STATUS
IScsiAsciiStrToIp (
IN CHAR8 *Str,
IN UINT8 IpMode,
OUT EFI_IP_ADDRESS *Ip
);

/**
Convert the binary encoded buffer into a hexadecimal encoded string.

@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a binary
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index b8fef3ff6f5a..42988e15cb06 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -300,61 +300,72 @@ IScsiMacAddrToStr (
String = &Str[3 * Index - 1] ;
if (VlanId != 0) {
String += UnicodeSPrint (String, 6 * sizeof (CHAR16), L"\\%04x", (UINTN) VlanId);
}

*String = L'\0';
}

/**
Convert the binary encoded buffer into a hexadecimal encoded string.

@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
)
{
- UINTN Index;
+ UINT32 HexLengthMin;
+ UINT32 HexLengthProvided;
+ UINT32 Index;

if ((HexStr == NULL) || (BinBuffer == NULL) || (BinLength == 0)) {
return EFI_INVALID_PARAMETER;
}

- if (((*HexLength) - 3) < BinLength * 2) {
- *HexLength = BinLength * 2 + 3;
+ //
+ // Safely calculate: HexLengthMin := BinLength * 2 + 3.
+ //
+ if (RETURN_ERROR (SafeUint32Mult (BinLength, 2, &HexLengthMin)) ||
+ RETURN_ERROR (SafeUint32Add (HexLengthMin, 3, &HexLengthMin))) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ HexLengthProvided = *HexLength;
+ *HexLength = HexLengthMin;
+ if (HexLengthProvided < HexLengthMin) {
return EFI_BUFFER_TOO_SMALL;
}

- *HexLength = BinLength * 2 + 3;
//
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';

for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength"

Laszlo Ersek
 

The "ISCSI_CHAP_AUTH_DATA.OutChallenge" field is declared as a UINT8 array
with ISCSI_CHAP_AUTH_MAX_LEN (1024) elements. However, when the challenge
is generated and formatted, only ISCSI_CHAP_RSP_LEN (16) octets are used
in the array.

Change the array size to ISCSI_CHAP_RSP_LEN, and remove the (now unused)
ISCSI_CHAP_AUTH_MAX_LEN macro.

Remove the "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" field, which is
superfluous too.

Most importantly, explain in a new comment *why* tying the challenge size
to the digest size (ISCSI_CHAP_RSP_LEN) has always made sense. (See also
Linux kernel commit 19f5f88ed779, "scsi: target: iscsi: tie the challenge
length to the hash digest size", 2019-11-06.) For sure, the motivation
that the new comment now explains has always been there, and has always
been the same, for IScsiDxe; it's just that now we spell it out too.

No change in peer-visible behavior.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 9 ++++++---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 3 +--
2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 1fc1d96ea3f3..35d5d6ec29e3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -3,39 +3,38 @@

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"

#define ISCSI_CHAP_ALGORITHM_MD5 5

-#define ISCSI_CHAP_AUTH_MAX_LEN 1024
///
/// MD5_HASHSIZE
///
#define ISCSI_CHAP_RSP_LEN 16

#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];
@@ -43,41 +42,45 @@ typedef struct _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];

//
// 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_AUTH_MAX_LEN];
- UINT32 OutChallengeLength;
+ UINT8 OutChallenge[ISCSI_CHAP_RSP_LEN];
} 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 df3c2eb1200a..9e192ce292e8 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -106,39 +106,39 @@ IScsiCHAPCalculateResponse (
**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
UINT8 VerifyRsp[ISCSI_CHAP_RSP_LEN];

Status = EFI_SUCCESS;

SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- AuthData->OutChallengeLength,
+ ISCSI_CHAP_RSP_LEN, // ChallengeLength
VerifyRsp
);

if (CompareMem (VerifyRsp, TargetResponse, ISCSI_CHAP_RSP_LEN) != 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.
@@ -474,39 +474,38 @@ IScsiCHAPToSendReq (
IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
ISCSI_CHAP_RSP_LEN,
Response,
&RspLen
);
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);
- AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
ISCSI_CHAP_RSP_LEN,
Challenge,
&ChallengeLen
);
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


[PUBLIC edk2 PATCH v2 02/10] NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size

Laszlo Ersek
 

The ISCSI_CHAP_AUTH_MAX_LEN macro is defined with value 1024.

The usage of this macro currently involves a semantic (not functional)
bug, which we're going to fix in a subsequent patch, eliminating
ISCSI_CHAP_AUTH_MAX_LEN altogether.

For now, remove the macro's usage from all
"ISCSI_CHAP_AUTH_DATA.InChallenge" contexts. This is doable without
duplicating open-coded constants.

No changes in functionality.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 2 +-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 5e59fb678bd7..1fc1d96ea3f3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -33,39 +33,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#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[ISCSI_CHAP_AUTH_MAX_LEN];
+ UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];

//
// Auth-data to be sent out for mutual authentication.
//
UINT32 OutIdentifier;
UINT8 OutChallenge[ISCSI_CHAP_AUTH_MAX_LEN];
UINT32 OutChallengeLength;
} ISCSI_CHAP_AUTH_DATA;

/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index cbbc56ae5b43..df3c2eb1200a 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -273,39 +273,39 @@ IScsiCHAPOnRspReceived (
}

Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}

AuthData->InIdentifier = (UINT32) Result;
- AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
+ AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
IScsiHexToBin (
(UINT8 *) AuthData->InChallenge,
&AuthData->InChallengeLength,
Challenge
);
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);

//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters

Laszlo Ersek
 

Working with overlong lines is difficult for me; rewrap the CHAP-related
source files in IScsiDxe to 80 characters width. No functional changes.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 3 +-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 90 +++++++++++++++-----
2 files changed, 71 insertions(+), 22 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 140bba0dcd76..5e59fb678bd7 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -72,31 +72,32 @@ typedef struct _ISCSI_CHAP_AUTH_DATA {

@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
);
/**
This function fills the CHAP authentication information into the login PDU
during the security negotiation stage in the iSCSI connection login.

@param[in] Conn The iSCSI connection.
@param[in, out] Pdu The PDU to send out.

@retval EFI_SUCCESS All check passed and the phase-related CHAP
- authentication info is filled into the iSCSI PDU.
+ authentication info is filled into the iSCSI
+ PDU.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.

**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
);

#endif
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 355c6f129f68..cbbc56ae5b43 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -1,42 +1,45 @@
/** @file
- This file is for Challenge-Handshake Authentication Protocol (CHAP) Configuration.
+ This file is for Challenge-Handshake Authentication Protocol (CHAP)
+ Configuration.

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#include "IScsiImpl.h"

/**
Initiator calculates its own expected hash value.

@param[in] ChapIdentifier iSCSI CHAP identifier sent by authenticator.
@param[in] ChapSecret iSCSI CHAP secret of the authenticator.
@param[in] SecretLength The length of iSCSI CHAP secret.
@param[in] ChapChallenge The challenge message sent by authenticator.
@param[in] ChallengeLength The length of iSCSI CHAP challenge message.
@param[out] ChapResponse The calculation of the expected hash value.

- @retval EFI_SUCCESS The expected hash value was calculatedly successfully.
- @retval EFI_PROTOCOL_ERROR The length of the secret should be at least the
- length of the hash value for the hashing algorithm chosen.
+ @retval EFI_SUCCESS The expected hash value was calculatedly
+ successfully.
+ @retval EFI_PROTOCOL_ERROR The length of the secret should be at least
+ the length of the hash value for the hashing
+ algorithm chosen.
@retval EFI_PROTOCOL_ERROR MD5 hash operation fail.
@retval EFI_OUT_OF_RESOURCES Fail to allocate resource to complete MD5.

**/
EFI_STATUS
IScsiCHAPCalculateResponse (
IN UINT32 ChapIdentifier,
IN CHAR8 *ChapSecret,
IN UINT32 SecretLength,
IN UINT8 *ChapChallenge,
IN UINT32 ChallengeLength,
OUT UINT8 *ChapResponse
)
{
UINTN Md5ContextSize;
VOID *Md5Ctx;
CHAR8 IdByte[1];
EFI_STATUS Status;

@@ -78,40 +81,42 @@ IScsiCHAPCalculateResponse (
goto Exit;
}

if (Md5Final (Md5Ctx, ChapResponse)) {
Status = EFI_SUCCESS;
}

Exit:
FreePool (Md5Ctx);
return Status;
}

/**
The initiator checks the CHAP response replied by target against its own
calculation of the expected hash value.

@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 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];

Status = EFI_SUCCESS;

SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
@@ -177,187 +182,211 @@ IScsiCHAPOnRspReceived (
//
NetbufQueCopy (&Conn->RspQue, 0, Len, Data);

//
// Build the key-value list from the data segment of the Login Response.
//
KeyValueList = IScsiBuildKeyValueList ((CHAR8 *) Data, Len);
if (KeyValueList == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto ON_EXIT;
}

Status = EFI_PROTOCOL_ERROR;

switch (Conn->AuthStep) {
case ISCSI_AUTH_INITIAL:
//
// The first Login Response.
//
- Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_TARGET_PORTAL_GROUP_TAG);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_TARGET_PORTAL_GROUP_TAG
+ );
if (Value == NULL) {
goto ON_EXIT;
}

Result = IScsiNetNtoi (Value);
if (Result > 0xFFFF) {
goto ON_EXIT;
}

Session->TargetPortalGroupTag = (UINT16) Result;

- Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_AUTH_METHOD);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_AUTH_METHOD
+ );
if (Value == NULL) {
goto ON_EXIT;
}
//
- // Initiator mandates CHAP authentication but target replies without "CHAP", or
- // initiator suggets "None" but target replies with some kind of auth method.
+ // Initiator mandates CHAP authentication but target replies without
+ // "CHAP", or initiator suggets "None" but target replies with some kind of
+ // auth method.
//
if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
if (AsciiStrCmp (Value, ISCSI_KEY_VALUE_NONE) != 0) {
goto ON_EXIT;
}
} else if (Session->AuthType == ISCSI_AUTH_TYPE_CHAP) {
if (AsciiStrCmp (Value, ISCSI_AUTH_METHOD_CHAP) != 0) {
goto ON_EXIT;
}
} else {
goto ON_EXIT;
}

//
// Transit to CHAP step one.
//
Conn->AuthStep = ISCSI_CHAP_STEP_ONE;
Status = EFI_SUCCESS;
break;

case ISCSI_CHAP_STEP_TWO:
//
// The Target replies with CHAP_A=<A> CHAP_I=<I> CHAP_C=<C>
//
- Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_ALGORITHM);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_ALGORITHM
+ );
if (Value == NULL) {
goto ON_EXIT;
}

Algorithm = IScsiNetNtoi (Value);
if (Algorithm != ISCSI_CHAP_ALGORITHM_MD5) {
//
// Unsupported algorithm is chosen by target.
//
goto ON_EXIT;
}

- Identifier = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_IDENTIFIER);
+ Identifier = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_IDENTIFIER
+ );
if (Identifier == NULL) {
goto ON_EXIT;
}

- Challenge = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_CHALLENGE);
+ Challenge = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_CHALLENGE
+ );
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}

AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
- IScsiHexToBin ((UINT8 *) AuthData->InChallenge, &AuthData->InChallengeLength, Challenge);
+ IScsiHexToBin (
+ (UINT8 *) AuthData->InChallenge,
+ &AuthData->InChallengeLength,
+ Challenge
+ );
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);

//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;

case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
//
Status = EFI_SUCCESS;
break;

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);
+ Response = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_RESPONSE
+ );
if (Response == NULL) {
goto ON_EXIT;
}

RspLen = ISCSI_CHAP_RSP_LEN;
IScsiHexToBin (TargetRsp, &RspLen, Response);

//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;

default:
break;
}

ON_EXIT:

if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}

FreePool (Data);

return Status;
}


/**
This function fills the CHAP authentication information into the login PDU
during the security negotiation stage in the iSCSI connection login.

@param[in] Conn The iSCSI connection.
@param[in, out] Pdu The PDU to send out.

@retval EFI_SUCCESS All check passed and the phase-related CHAP
- authentication info is filled into the iSCSI PDU.
+ authentication info is filled into the iSCSI
+ PDU.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.

**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_LOGIN_REQUEST *LoginReq;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
@@ -376,95 +405,114 @@ IScsiCHAPToSendReq (
RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}

ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 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_INITIATOR_NAME,
+ mPrivate->InitiatorName
+ );
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_TARGET_NAME,
Session->ConfigData->SessionConfigData.TargetName
);

if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
Value = ISCSI_KEY_VALUE_NONE;
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
} else {
Value = ISCSI_AUTH_METHOD_CHAP;
}

IScsiAddKeyValuePair (Pdu, ISCSI_KEY_AUTH_METHOD, Value);

break;

case ISCSI_CHAP_STEP_ONE:
//
- // First step, send the Login Request with CHAP_A=<A1,A2...> key-value pair.
+ // First step, send the Login Request with CHAP_A=<A1,A2...> key-value
+ // pair.
//
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", ISCSI_CHAP_ALGORITHM_MD5);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_ALGORITHM, ValueStr);

Conn->AuthStep = ISCSI_CHAP_STEP_TWO;
break;

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);
+ IScsiAddKeyValuePair (
+ Pdu,
+ ISCSI_KEY_CHAP_NAME,
+ (CHAR8 *) &AuthData->AuthConfig->CHAPName
+ );
//
// CHAP_R=<R>
//
- IScsiBinToHex ((UINT8 *) AuthData->CHAPResponse, ISCSI_CHAP_RSP_LEN, Response, &RspLen);
+ IScsiBinToHex (
+ (UINT8 *) AuthData->CHAPResponse,
+ ISCSI_CHAP_RSP_LEN,
+ Response,
+ &RspLen
+ );
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);
AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
- IScsiBinToHex ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN, Challenge, &ChallengeLen);
+ IScsiBinToHex (
+ (UINT8 *) AuthData->OutChallenge,
+ ISCSI_CHAP_RSP_LEN,
+ Challenge,
+ &ChallengeLen
+ );
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;
}

FreePool (Response);
FreePool (Challenge);

return Status;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs

Laszlo Ersek
 

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Repo: https://pagure.io/lersek/edk2.git
Branch: iscsi_overflow_bz3356

The main goal of this series is to fix a remotely exploitable buffer
overflow in the IScsiHexToBin() function.

This posting corresponds to:

https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c22

meaning that it corresponds to the v2 patches attached to, and tested
in,

https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c17

and that it carries Phil's and Maciej's R-b's that were given up to
comment#22.

Today is the Public Date for this embargoed security issue; I intend to
merge the patches tomorrow, based on Maciej's (already given) R-b.
(Simultaneously with this posting, I'm opening up the BZ publicly.) No
further review is required; the one day delay on the list is just to
give the community a (brief) opportunity to speak up, before the patches
are merged.

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>

Thanks,
Laszlo

Laszlo Ersek (10):
NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size
NetworkPkg/IScsiDxe: clean up
"ISCSI_CHAP_AUTH_DATA.OutChallengeLength"
NetworkPkg/IScsiDxe: clean up library class dependencies
NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()
NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds
NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block
NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing
NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow
NetworkPkg/IScsiDxe: check IScsiHexToBin() return values

NetworkPkg/IScsiDxe/IScsiCHAP.c | 108 +++++++++++++++-----
NetworkPkg/IScsiDxe/IScsiCHAP.h | 14 ++-
NetworkPkg/IScsiDxe/IScsiDxe.inf | 7 +-
NetworkPkg/IScsiDxe/IScsiImpl.h | 18 ++--
NetworkPkg/IScsiDxe/IScsiMisc.c | 65 +++++++++---
NetworkPkg/IScsiDxe/IScsiMisc.h | 19 ++--
6 files changed, 166 insertions(+), 65 deletions(-)

--
2.19.1.3.g30247aa5d201


Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Dov Murik
 

On 08/06/2021 13:59, Laszlo Ersek wrote:
Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:


On 04/06/2021 14:26, Laszlo Ersek wrote:
On 06/04/21 12:30, Dov Murik wrote:
...


[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between
two places. (Well, it is split between *three* places in fact, but
I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
the command line is fetched in (both) QemuLoadImageLib instances.
This requires that all these modules be littered with hashing as
well, which I find *really bad*. Even if we factor out the actual
logic, I strongly dislike having *just hooks* for hashing in
multiple modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
don't expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly
from fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?
I understand there's agreement here, and that both this suggested
change (use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas. How do you envision this
process in the mailing list? Seperate patch serieses with
dependency? One long patch series with both changes? What goes
first?
Good point. I do have a kind of patch order laid out in my mind, but
I didn't think of whether we should have the patches in one patch
series, or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and
(b). Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device
path, so the first step would be calling gBS->LocateDevicePath(), for
finding SimpleFs on the unique device path. Once you have the
SimpleFs interface, you can call OpenVolume, then open the "cmdline"
file using the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon.
I started working on that, and managed to remove all QemuFwCfg* calls
in the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c). That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic
"initrd" file. I used Library/FileHandleLib.h; I hope that's fine.
The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO pointer
and freeing it etc (for getting the size of "cmdline" and "initrd").
But maybe it's better not to add another dependency.



In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

However, there's another path (which I don't reach with my test
setup), which is the call to QemuLoadLegacyImage, which has a lot of
calls to QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?
The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).


(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().


I'm sorry for missing this aspect previously! I really hope we can go
with (1)!
I'll check.

But if we go with (1) -- do you (and Ard) prefer:

(a) leave X86QemuLoadImageLib as it is in master;

-or-

(b) modify X86QemuLoadImageLib the "main" path to use the
QemuKernelLoaderFs (what I started doing) and leave the "legacy" path
with QemuFwCfg

?


Or, in other words, is the refactoring to read the cmdline from
QemuKernelLoaderFs (across both QemuLoadImageLib implementations)
beneficial even if we don't add the verification "hooks"?


-Dov

Thanks,
Laszlo


Re: [edk2-platforms][PATCH v2 19/32] AmpereAltraPkg: Add Random Number Generator Support

Leif Lindholm
 

On Wed, May 26, 2021 at 17:07:11 +0700, Nhi Pham wrote:
From: Vu Nguyen <vunguyen@os.amperecomputing.com>

This change is to produce RNG protocol which is required by several
modules.

Cc: Thang Nguyen <thang@os.amperecomputing.com>
Cc: Chuong Tran <chuong@os.amperecomputing.com>
Cc: Phong Vo <phong@os.amperecomputing.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>

Signed-off-by: Vu Nguyen <vunguyen@os.amperecomputing.com>
---
Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc | 5 +
Platform/Ampere/JadePkg/Jade.fdf | 5 +
Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf | 43 +++++
Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.c | 164 ++++++++++++++++++++
Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.uni | 10 ++
Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxeExtra.uni | 9 ++
6 files changed, 236 insertions(+)

diff --git a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
index 33f5fe7af544..930bbb5d385b 100755
--- a/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
+++ b/Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dsc.inc
@@ -682,6 +682,11 @@ [Components.common]
MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
Silicon/Ampere/AmpereAltraPkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf

+ #
+ # Random Number Generator Support
+ #
+ Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf
+
#
# Bds
#
diff --git a/Platform/Ampere/JadePkg/Jade.fdf b/Platform/Ampere/JadePkg/Jade.fdf
index 6820a197568b..ef24e6f1f8e0 100755
--- a/Platform/Ampere/JadePkg/Jade.fdf
+++ b/Platform/Ampere/JadePkg/Jade.fdf
@@ -304,6 +304,11 @@ [FV.FvMain]
#
INF Drivers/ASpeed/ASpeedGopBinPkg/ASpeedAst2500GopDxe.inf

+ #
+ # Random Number Generator Support
+ #
+ INF Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf
+
#
# UEFI application (Shell Embedded Boot Loader)
#
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf
new file mode 100644
index 000000000000..d3d2c20436a0
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.inf
@@ -0,0 +1,43 @@
+## @file
+#
+# Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+ INF_VERSION = 0x0001001B
+ BASE_NAME = RngDxe
+ FILE_GUID = 4FCC006E-C740-4027-BC97-787907C8D286
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ ENTRY_POINT = RngDriverEntry
+ MODULE_UNI_FILE = RngDxe.uni
+
+[Sources.common]
+ RngDxe.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ Silicon/Ampere/AmpereAltraPkg/AmpereAltraPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ TrngLib
+ UefiBootServicesTableLib
+ UefiDriverEntryPoint
+ UefiLib
+
+[Guids]
+ gEfiRngAlgorithmRaw ## SOMETIMES_PRODUCES ## GUID # Unique ID of the algorithm for RNG
+
+[Protocols]
+ gEfiRngProtocolGuid ## PRODUCES
+
+[UserExtensions.TianoCore."ExtraFiles"]
+ RngDxeExtra.uni
+
+[Depex]
+ TRUE
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.c b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.c
new file mode 100644
index 000000000000..bb8140cfeb2f
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.c
@@ -0,0 +1,164 @@
+/** @file
+
+ Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/TrngLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/Rng.h>
+
+/**
+ Returns information about the random number generation implementation.
+
+ @param[in] This A pointer to the EFI_RNG_PROTOCOL instance.
+ @param[in,out] RNGAlgorithmListSize On input, the size in bytes of RNGAlgorithmList.
+ On output with a return code of EFI_SUCCESS, the size
+ in bytes of the data returned in RNGAlgorithmList. On output
+ with a return code of EFI_BUFFER_TOO_SMALL,
+ the size of RNGAlgorithmList required to obtain the list.
+ @param[out] RNGAlgorithmList A caller-allocated memory buffer filled by the driver
+ with one EFI_RNG_ALGORITHM element for each supported
+ RNG algorithm. The list must not change across multiple
+ calls to the same driver. The first algorithm in the list
+ is the default algorithm for the driver.
+
+ @retval EFI_SUCCESS The RNG algorithm list was returned successfully.
+ @retval EFI_INVALID_PARAMETER One or more of the parameters are incorrect.
+ @retval EFI_BUFFER_TOO_SMALL The buffer RNGAlgorithmList is too small to hold the result.
+
+**/
+EFI_STATUS
+EFIAPI
+RngGetInfo (
+ IN EFI_RNG_PROTOCOL *This,
+ IN OUT UINTN *RNGAlgorithmListSize,
+ OUT EFI_RNG_ALGORITHM *RNGAlgorithmList
+ )
+{
+ if (This == NULL || RNGAlgorithmListSize == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
+ *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ if (RNGAlgorithmList == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
+ CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Produces and returns an RNG value using either the default or specified RNG algorithm.
+
+ @param[in] This A pointer to the EFI_RNG_PROTOCOL instance.
+ @param[in] RNGAlgorithm A pointer to the EFI_RNG_ALGORITHM that identifies the RNG
+ algorithm to use. May be NULL in which case the function will
+ use its default RNG algorithm.
+ @param[in] RNGValueLength The length in bytes of the memory buffer pointed to by
+ RNGValue. The driver shall return exactly this numbers of bytes.
+ @param[out] RNGValue A caller-allocated memory buffer filled by the driver with the
+ resulting RNG value.
+
+ @retval EFI_SUCCESS The RNG value was returned successfully.
+ @retval EFI_UNSUPPORTED The algorithm specified by RNGAlgorithm is not supported by
+ this driver.
+ @retval EFI_DEVICE_ERROR An RNG value could not be retrieved due to a hardware or
+ firmware error.
+ @retval EFI_INVALID_PARAMETER RNGValue is NULL or RNGValueLength is zero.
+
+**/
+EFI_STATUS
+EFIAPI
+RngGetRNG (
+ IN EFI_RNG_PROTOCOL *This,
+ IN EFI_RNG_ALGORITHM *RNGAlgorithm, OPTIONAL
+ IN UINTN RNGValueLength,
+ OUT UINT8 *RNGValue
+ )
+{
+ EFI_STATUS Status;
+
+ if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // We only support the raw algorithm, so reject requests for anything else
+ //
+ if (RNGAlgorithm != NULL &&
+ !CompareGuid (RNGAlgorithm, &gEfiRngAlgorithmRaw))
+ {
{ on preceding line.

With that:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

/
Leif

+ return EFI_UNSUPPORTED;
+ }
+
+ Status = GenerateRandomNumbers (RNGValue, RNGValueLength);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "%a:%d Failed to generate a random number. \n",
+ __FUNCTION__,
+ __LINE__
+ ));
+ return Status;
+ }
+
+ return EFI_SUCCESS;
+}
+
+/*
+ * The Random Number Generator (RNG) protocol
+ */
+EFI_RNG_PROTOCOL mRng = {
+ RngGetInfo,
+ RngGetRNG
+};
+
+/**
+ The user Entry Point for the Random Number Generator (RNG) driver.
+
+ @param[in] ImageHandle The firmware allocated handle for the EFI image.
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS The entry point is executed successfully.
+ @retval EFI_NOT_SUPPORTED Platform does not support RNG.
+ @retval Other Some error occurs when executing this entry point.
+
+**/
+EFI_STATUS
+EFIAPI
+RngDriverEntry (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ EFI_STATUS Status;
+ EFI_HANDLE Handle;
+
+ //
+ // Install UEFI RNG (Random Number Generator) Protocol
+ //
+ Handle = NULL;
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &Handle,
+ &gEfiRngProtocolGuid,
+ &mRng,
+ NULL
+ );
+
+ return Status;
+}
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.uni b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.uni
new file mode 100644
index 000000000000..cd9dde97a236
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxe.uni
@@ -0,0 +1,10 @@
+//
+// Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+
+
+#string STR_MODULE_ABSTRACT #language en-US "Produces UEFI Random Number Generator protocol"
+
+#string STR_MODULE_DESCRIPTION #language en-US "This module will produce UEFI Random Number Generator protocol."
diff --git a/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxeExtra.uni b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxeExtra.uni
new file mode 100644
index 000000000000..9a3696b25442
--- /dev/null
+++ b/Silicon/Ampere/AmpereAltraPkg/Drivers/RngDxe/RngDxeExtra.uni
@@ -0,0 +1,9 @@
+//
+// Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+
+#string STR_PROPERTIES_MODULE_NAME
+#language en-US
+"Ampere UEFI Random Number Generator DXE"
--
2.17.1


Re: [PATCH V0 0/4] Enable Dynamic ACPI for LS1046AFRWY

Leif Lindholm
 

Hi Vikas,

Please resubmit this set, following the instructions set out by
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers
as I requested for your previous set.

Best Regards,

Leif

On Tue, Jun 01, 2021 at 19:20:30 +0530, Vikas Singh wrote:
This patch series basically aims to extend the Dynamic ACPI
framework towards NXP's LS1046AFRWY platform.

Refer- https://edk2.groups.io/g/devel/message/71709

The change set in the series is in below order -

(1)Introducing a new platform specific macro "PLAT_SOC_NAME"
This macro will be consumed by Configuration Manager(CM).
Platforms who extends CM services for themselves must notify
their SoC details to CM. Additionally also update the lx2160ardb
platform header with PLAT_SOC_NAME, this will be consumed by CM.

(2)Introduced a function to get SoC's System Version Register(SVR)
This function will fetch SVR for LS1046A SoC based platforms.
In current patch series, this function will be used by LS1046aFrwy.

(3)Extending Configuration Manager (CM) and its services to leverage
the Dynamic ACPI support for NXP's LS1046aFrwy platform.

(4)Introduced an OEM specific firmware acpi table generator
Also add Dsdt.asl as a place holder having only platform's clock
related dsdt properties for now and can accommodate other IP specific
dsdt tables(acpi properties) for LS1046AFRWY in future patch series.

Vikas Singh (4):
Platform/NXP: Add generic log in CM to print SoC version
Silicon/NXP: Add support of SVR handling for LS1046FRWY
Platform/NXP/LS1046aFrwyPkg: Extend Dynamic ACPI support
Platform/NXP/LS1046aFrwyPkg: Add OEM specific DSDT generator

.../ConfigurationManager.c | 10 +-
.../AcpiTablesInclude/Dsdt/Clk.asl | 60 +++++++
.../AcpiTablesInclude/Dsdt/Dsdt.asl | 15 ++
.../AcpiTablesInclude/PlatformAcpiDsdtLib.inf | 39 +++++
.../PlatformAcpiDsdtLib/RawDsdtGenerator.c | 138 +++++++++++++++
.../AcpiTablesInclude/PlatformAcpiLib.h | 23 +++
.../NXP/LS1046aFrwyPkg/Include/Platform.h | 159 ++++++++++++++++++
.../NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.dsc | 29 ++++
.../NXP/LS1046aFrwyPkg/LS1046aFrwyPkg.fdf | 13 ++
Platform/NXP/LX2160aRdbPkg/Include/Platform.h | 5 +-
Silicon/NXP/LS1046A/LS1046A.dsc.inc | 10 ++
Silicon/NXP/LS1046A/Library/SocLib/SocLib.c | 16 ++
12 files changed, 507 insertions(+), 10 deletions(-)
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Clk.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/Dsdt/Dsdt.asl
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib.inf
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiDsdtLib/RawDsdtGenerator.c
create mode 100644 Platform/NXP/LS1046aFrwyPkg/AcpiTablesInclude/PlatformAcpiLib.h
create mode 100644 Platform/NXP/LS1046aFrwyPkg/Include/Platform.h

--
2.25.1


Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Laszlo Ersek
 

Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:


On 04/06/2021 14:26, Laszlo Ersek wrote:
On 06/04/21 12:30, Dov Murik wrote:
...


[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between
two places. (Well, it is split between *three* places in fact, but
I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
the command line is fetched in (both) QemuLoadImageLib instances.
This requires that all these modules be littered with hashing as
well, which I find *really bad*. Even if we factor out the actual
logic, I strongly dislike having *just hooks* for hashing in
multiple modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
don't expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly
from fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?
I understand there's agreement here, and that both this suggested
change (use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas. How do you envision this
process in the mailing list? Seperate patch serieses with
dependency? One long patch series with both changes? What goes
first?
Good point. I do have a kind of patch order laid out in my mind, but
I didn't think of whether we should have the patches in one patch
series, or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and
(b). Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device
path, so the first step would be calling gBS->LocateDevicePath(), for
finding SimpleFs on the unique device path. Once you have the
SimpleFs interface, you can call OpenVolume, then open the "cmdline"
file using the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon.
I started working on that, and managed to remove all QemuFwCfg* calls
in the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c). That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic
"initrd" file. I used Library/FileHandleLib.h; I hope that's fine.
The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

However, there's another path (which I don't reach with my test
setup), which is the call to QemuLoadLegacyImage, which has a lot of
calls to QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?
The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).


(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().


I'm sorry for missing this aspect previously! I really hope we can go
with (1)!

Thanks,
Laszlo


Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Dov Murik
 

On 04/06/2021 14:26, Laszlo Ersek wrote:
On 06/04/21 12:30, Dov Murik wrote:
...


[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between two
places. (Well, it is split between *three* places in fact, but I'm
going to ignore LinuxInitrdDynamicShellCommand for now, because the
AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
the command line is fetched in (both) QemuLoadImageLib instances.
This requires that all these modules be littered with hashing as
well, which I find *really bad*. Even if we factor out the actual
logic, I strongly dislike having *just hooks* for hashing in multiple
modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
don't expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly
from fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?
I understand there's agreement here, and that both this suggested
change (use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas. How do you envision this
process in the mailing list? Seperate patch serieses with dependency?
One long patch series with both changes? What goes first?
Good point. I do have a kind of patch order laid out in my mind, but I
didn't think of whether we should have the patches in one patch series,
or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and (b).
Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device path,
so the first step would be calling gBS->LocateDevicePath(), for finding
SimpleFs on the unique device path. Once you have the SimpleFs
interface, you can call OpenVolume, then open the "cmdline" file using
the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon.
I started working on that, and managed to remove all QemuFwCfg* calls in
the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c). That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic "initrd"
file. I used Library/FileHandleLib.h; I hope that's fine.

However, there's another path (which I don't reach with my test setup),
which is the call to QemuLoadLegacyImage, which has a lot of calls to
QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?


Thanks for the guidance,

-Dov


Re: [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD

Laszlo Ersek
 

On 06/07/21 19:33, Brijesh Singh wrote:

On 6/7/21 7:48 AM, Laszlo Ersek wrote:
On 06/07/21 14:26, Laszlo Ersek wrote:
On 05/27/21 01:11, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OjlLNpUW8U%2FykMMU7JwjddEZd9Zi%2BsHNK%2FqoQCwW3vo%3D&amp;reserved=0

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.
For pure SEV?

When SEV-SNP is enabled, the secrets page contains the VM platform
communication keys. The guest BIOS and OS can use this key to communicate
with the SEV firmware to get attesation report. See the SEV-SNP firmware
spec for more details for the content of the secrets page.

When SEV and SEV-ES is enabled, the secrets page contains the information
provided by the guest owner after the attestation. See the SEV
LAUNCH_SECRET command for more details.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.fdf | 5 +++++
OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 +
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 15 ++++++++++++++-
4 files changed, 22 insertions(+), 1 deletion(-)
How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc"
platform, where remote attestation is not a goal?

What you describe makes sense to me, but only for the remote-attested
"OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes
SecretPei and SecretDxe, and sets the necessary PCDs.)

Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei"
module, the commit message does not explain sufficiently why the secrets
page must be reserved for good. The "SEV-SNP firmware spec" reference is
vague at best; I'm permanently lost between the dozen PDF files I have
downloaded locally from the AMD website. Please include a specific
document number, revision number, and chapter/section identifier.

Honestly I'm getting a *rushed* vibe on this whole series. Why is that?

Assume that I'm dumb. You won't be far from the truth. Then hold my hand
through all this?
Here's the v2 discussion:

- https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F9804ecb5-8afd-c56e-4982-d1a6ebad3de8%40redhat.com&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=K8FRcks19dQ4BM4DBOh%2F7uO4hNvIsM0eqdNvwUQzDUU%3D&amp;reserved=0
- https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F74797&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489236720%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8rfpRAEvBdWex0BQctCbbGnHb691gcKSIEvVA3ZKDkg%3D&amp;reserved=0
- https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00112.html&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NAL8jAfiq1EApkDBOBjgL7b3NIsmjginZSDxB1NDCk8%3D&amp;reserved=0

That discussion refers to a different use case, raised by Dov. That use
case might justify reserving the area even for plain SEV. It's out of
scope for now, AIUI.

(

And even for that separate use case, James showed down-thread that *not*
reserving the page forever in the firmware is more flexible.

- https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2Faed7d3490fe6edee74440ed8e4cd5364fb2ba4af.camel%40linux.ibm.com&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=2UV6KcGYb9CoKzgIU%2FscCX2l%2F5pKaSkFYshP%2BPSWHSM%3D&amp;reserved=0
- https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F74801&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=jnHpYxYkijt2LtcH772m88%2BLNH3Zjfn3Zqc3uuttL1M%3D&amp;reserved=0
- https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flistman.redhat.com%2Farchives%2Fedk2-devel-archive%2F2021-May%2Fmsg00116.html&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc7a508dbd4af461b413208d929b2a231%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586669489246713%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=XzePjtTDS8blsXhBNOg52uo81uFhoYpgcNMvU4RupSI%3D&amp;reserved=0

)

AFAICT, the only effect of the v2 sub-thread on the patch has been that
we now use the Reserved memory type rather than AcpiNVS (when SEV-SNP is
in use). I have two comments on that:

- It's good that we're not mixing in the other use case raised by Dov
(i.e., enabling the guest-kernel to read secrets from the injected
page even under plain SEV).

- It's still unclear to me why the reservation needs to be permanent
under SEV-SNP.

As highlighted in the previous email, in the case of SEV, the secrets
page contains the private data provided by the guest owner to the guest.
Whereas, in SEV-SNP, the secrets page includes the key and other
metadata used by the guest (OVMF or kernel) to construct a message for
the PSP. The secrets page contains some information (e.g key and
sequence number) that must persist across kexec boots. If we mark the
SEV-SNP secrets page as "Boot Data," I believe it gets free'd on
ExitBootService(). In the kexec'ed kernel, we need to retrieve the
secret page to get the key and message counters to construct the next
PSP quest request command.
All great information, especially the kexec bits; why not explicitly
document all this?

Thanks
Laszlo



I have not looked into detail on how EFI configuration table and other
data is preserved during the kexec boot, but I thought making the
secrets reserved should ensure that memory is not free'd on
ExitBootServices() and we can reach it after the kexec. I can
investigate a bit more.

Dov/James,
 
Does kexec works with the SEV secrets page?

-Brijesh




Re: [Patch V3 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

Patrick Rudolph
 

On Fri, Jun 4, 2021 at 11:42 AM Zhiguang Liu <zhiguang.liu@intel.com> wrote:

V1:
The default EfiSmbiosProtocol operates on an empty SMBIOS table.
The SMBIOS tables are provided by the bootloader on UefiPayloadPkg.
Scan for existing tables in SmbiosDxe and load them if they seem valid.

This fixes the settings menu not showing any hardware information, instead
only "0 MB RAM" was displayed.

Tests showed that the OS can still see the SMBIOS tables.

V2:
SmbiosDxe will get the SMBIOS from a guid Hob.
Aslo will keep the SmbiosHandle if it is available.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h | 4 +++-
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 5 ++++-
3 files changed, 325 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
index 3cdb0b1ed7..3579c4d890 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -2,7 +2,7 @@
This code produces the Smbios protocol. It also responsible for constructing
SMBIOS table into system table.

-Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -148,6 +148,31 @@ SMBIOS_TABLE_3_0_ENTRY_POINT Smbios30EntryPointStructureData = {
//
0
};
+
+/**
+ Validates a SMBIOS table entry point.
+
+ @param TableEntry The SmBios table entry to validate.
+ @param TableAddress On exit, point to the smbios table addres.
+ @param TableMaximumSize On exit, point to the maximum size of the table.
+
+ @retval TRUE SMBIOS table entry point is valid.
+ @retval FALSE SMBIOS table entry point is malformed.
+
+**/
+typedef
+BOOLEAN
+(* IS_SMBIOS_TABLE_VALID) (
+ IN VOID *TableEntry,
+ OUT VOID **TableAddress,
+ OUT UINTN *TableMaximumSize
+ );
+typedef struct {
+ EFI_GUID *Guid;
+ IS_SMBIOS_TABLE_VALID IsValid;
+} IS_SMBIOS_TABLE_VALID_ENTRY;
+
+
/**

Get the full size of SMBIOS structure including optional strings that follow the formatted structure.
@@ -1408,6 +1433,296 @@ SmbiosTableConstruction (
}
}

+/**
+ Validates a SMBIOS 2.0 table entry point.
+
+ @param TableEntry The SmBios table entry to validate.
+ @param TableAddress On exit, point to the smbios table addres.
+ @param TableMaximumSize On exit, point to the maximum size of the table.
+
+ @retval TRUE SMBIOS table entry point is valid.
+ @retval FALSE SMBIOS table entry point is malformed.
+
+**/
+STATIC
+BOOLEAN
+IsValidSmbios20Table (
+ IN VOID *TableEntry,
+ OUT VOID **TableAddress,
+ OUT UINTN *TableMaximumSize
+ )
+{
+ UINT8 Checksum;
+ SMBIOS_TABLE_ENTRY_POINT *SmbiosTable;
+ SmbiosTable = (SMBIOS_TABLE_ENTRY_POINT *) TableEntry;
+
+ if (CompareMem (SmbiosTable->AnchorString, "_SM_", 4) != 0) {
+ return FALSE;
+ }
+
+ if (CompareMem (SmbiosTable->IntermediateAnchorString, "_DMI_", 5) != 0) {
+ return FALSE;
+ }
+
+ //
+ // The actual value of the EntryPointLength should be 1Fh.
+ // However, it was incorrectly stated in version 2.1 of smbios specification.
+ // Therefore, 0x1F and 0x1E are both accepted.
+ //
+ if (SmbiosTable->EntryPointLength != 0x1E && SmbiosTable->EntryPointLength != sizeof (SMBIOS_TABLE_ENTRY_POINT)) {
+ return FALSE;
+ }
+
+ //
+ // MajorVersion should not be less than 2.
+ //
+ if (SmbiosTable->MajorVersion < 2) {
+ return FALSE;
+ }
+
+ //
+ // The whole struct check sum should be zero
+ //
+ Checksum = CalculateSum8 (
+ (UINT8 *) SmbiosTable,
+ SmbiosTable->EntryPointLength
+ );
+ if (Checksum != 0) {
+ return FALSE;
+ }
+
+ //
+ // The Intermediate Entry Point Structure check sum should be zero.
+ //
+ Checksum = CalculateSum8 (
+ (UINT8 *) SmbiosTable + OFFSET_OF (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString),
+ SmbiosTable->EntryPointLength - OFFSET_OF (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString)
+ );
+ if (Checksum != 0) {
+ return FALSE;
+ }
+
+ *TableAddress = (VOID *) (UINTN) SmbiosTable->TableAddress;
+ *TableMaximumSize = SmbiosTable->TableLength;
+ return TRUE;
+}
+
+/**
+ Validates a SMBIOS 3.0 table entry point.
+
+ @param TableEntry The SmBios table entry to validate.
+ @param TableAddress On exit, point to the smbios table addres.
+ @param TableMaximumSize On exit, point to the maximum size of the table.
+
+ @retval TRUE SMBIOS table entry point is valid.
+ @retval FALSE SMBIOS table entry point is malformed.
+
+**/
+STATIC
+BOOLEAN
+IsValidSmbios30Table (
+ IN VOID *TableEntry,
+ OUT VOID **TableAddress,
+ OUT UINTN *TableMaximumSize
+ )
+{
+ UINT8 Checksum;
+ SMBIOS_TABLE_3_0_ENTRY_POINT *SmbiosTable;
+ SmbiosTable = (SMBIOS_TABLE_3_0_ENTRY_POINT *) TableEntry;
+
+ if (CompareMem (SmbiosTable->AnchorString, "_SM3_", 5) != 0) {
+ return FALSE;
+ }
+ if (SmbiosTable->EntryPointLength < sizeof (SMBIOS_TABLE_3_0_ENTRY_POINT)) {
+ return FALSE;
+ }
+ if (SmbiosTable->MajorVersion < 3) {
+ return FALSE;
+ }
+
+ //
+ // The whole struct check sum should be zero
+ //
+ Checksum = CalculateSum8 (
+ (UINT8 *) SmbiosTable,
+ SmbiosTable->EntryPointLength
+ );
+ if (Checksum != 0) {
+ return FALSE;
+ }
+
+ *TableAddress = (VOID *) (UINTN) SmbiosTable->TableAddress;
+ *TableMaximumSize = SmbiosTable->TableMaximumSize;
+ return TRUE;
+}
+
+/**
+ Parse an existing SMBIOS table and insert it using SmbiosAdd.
+
+ @param ImageHandle The EFI_HANDLE to this driver.
+ @param Smbios The SMBIOS table to parse.
+ @param Length The length of the SMBIOS table.
+
+ @retval EFI_SUCCESS SMBIOS table was parsed and installed.
+ @retval EFI_OUT_OF_RESOURCES Record was not added due to lack of system resources.
+ @retval EFI_INVALID_PARAMETER Smbios is not a correct smbios table
+
+**/
+STATIC
+EFI_STATUS
+ParseAndAddExistingSmbiosTable (
+ IN EFI_HANDLE ImageHandle,
+ IN SMBIOS_STRUCTURE_POINTER Smbios,
+ IN UINTN Length
+ )
+{
+ EFI_STATUS Status;
+ CHAR8 *String;
+ EFI_SMBIOS_HANDLE SmbiosHandle;
+ SMBIOS_STRUCTURE_POINTER SmbiosEnd;
+
+ SmbiosEnd.Raw = Smbios.Raw + Length;
+
+ if (Smbios.Raw >= SmbiosEnd.Raw || Smbios.Raw == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ do {
+ //
+ // Make sure not to access memory beyond SmbiosEnd
+ //
+ if (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw ||
+ Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw) {
+ return EFI_INVALID_PARAMETER;
+ }
+ //
+ // Check for end marker
+ //
+ if (Smbios.Hdr->Type == SMBIOS_TYPE_END_OF_TABLE) {
+ break;
+ }
+ //
+ // Make sure not to access memory beyond SmbiosEnd
+ // Each structure shall be terminated by a double-null (0000h).
+ //
+ if (Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) > SmbiosEnd.Raw ||
+ Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) < Smbios.Raw) {
+ return EFI_INVALID_PARAMETER;
+ }
+ //
+ // Install the table
+ //
+ SmbiosHandle = Smbios.Hdr->Handle;
+ Status = SmbiosAdd (
+ &mPrivateData.Smbios,
+ ImageHandle,
+ &SmbiosHandle,
+ Smbios.Hdr
+ );
+
+ ASSERT_EFI_ERROR (Status);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ //
+ // Go to the next SMBIOS structure. Each SMBIOS structure may include 2 parts:
+ // 1. Formatted section; 2. Unformatted string section. So, 2 steps are needed
+ // to skip one SMBIOS structure.
+ //
+
+ //
+ // Step 1: Skip over formatted section.
+ //
+ String = (CHAR8 *) (Smbios.Raw + Smbios.Hdr->Length);
+
+ //
+ // Step 2: Skip over unformatted string section.
+ //
+ do {
+ //
+ // Each string is terminated with a NULL(00h) BYTE and the sets of strings
+ // is terminated with an additional NULL(00h) BYTE.
+ //
+ for ( ; *String != 0; String++) {
+ if ((UINTN) String >= (UINTN) SmbiosEnd.Raw - sizeof (UINT8)) {
+ return EFI_INVALID_PARAMETER;
+ }
+ }
+
+ if (*(UINT8 *) ++String == 0) {
+ //
+ // Pointer to the next SMBIOS structure.
+ //
+ Smbios.Raw = (UINT8 *) ++String;
+ break;
+ }
+ } while (TRUE);
+ } while (Smbios.Raw < SmbiosEnd.Raw);
+
+ return EFI_SUCCESS;
+}
+
+
+IS_SMBIOS_TABLE_VALID_ENTRY mIsSmbiosTableValid[] = {
+ {&gPldSmbios3TableGuid, IsValidSmbios30Table },
+ {&gPldSmbiosTableGuid, IsValidSmbios20Table }
+};
+
+/**
+ Retrieve SMBIOS from Hob.
+ @param ImageHandle Module's image handle
+
+ @retval EFI_SUCCESS Smbios from Hob is installed.
+ @return EFI_NOT_FOUND Not found Smbios from Hob.
+ @retval Other No Smbios from Hob is installed.
+
+**/
+EFI_STATUS
+RetrieveSmbiosFromHob (
+ IN EFI_HANDLE ImageHandle
+ )
+{
+ EFI_STATUS Status;
+ UINTN Index;
+ SMBIOS_STRUCTURE_POINTER Smbios;
+ EFI_HOB_GUID_TYPE *GuidHob;
+ PLD_SMBIOS_TABLE *SmBiosTableAdress;
+ PLD_GENERIC_HEADER *GenericHeader;
+ VOID *TableAddress;
+ UINTN TableMaximumSize;
+
+ Status = EFI_NOT_FOUND;
+
+ for (Index = 0; Index < ARRAY_SIZE (mIsSmbiosTableValid); Index++) {
+ GuidHob = GetFirstGuidHob (mIsSmbiosTableValid[Index].Guid);
+ if (GuidHob == NULL) {
+ continue;
+ }
+ GenericHeader = (PLD_GENERIC_HEADER *) GET_GUID_HOB_DATA (GuidHob);
+ if ((sizeof (PLD_GENERIC_HEADER) <= GET_GUID_HOB_DATA_SIZE (GuidHob)) && (GenericHeader->Length <= GET_GUID_HOB_DATA_SIZE (GuidHob))) {
+ if (GenericHeader->Revision == PLD_SMBIOS_TABLE_REVISION) {
+ //
+ // PLD_SMBIOS_TABLE structure is used when Revision equals to PLD_SMBIOS_TABLE_REVISION
+ //
+ SmBiosTableAdress = (PLD_SMBIOS_TABLE *) GET_GUID_HOB_DATA (GuidHob);
+ if (GenericHeader->Length >= PLD_SIZEOF_THROUGH_FIELD (PLD_SMBIOS_TABLE, SmBiosEntryPoint)) {
+ if (mIsSmbiosTableValid[Index].IsValid ((VOID *) (UINTN )SmBiosTableAdress->SmBiosEntryPoint, &TableAddress, &TableMaximumSize)) {
+ Smbios.Raw = TableAddress;
+ Status = ParseAndAddExistingSmbiosTable (ImageHandle, Smbios, TableMaximumSize);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "RetrieveSmbiosFromHob: Failed to parse preinstalled tables from gPldSmbios3TableGuid Guid Hob\n"));
+ Status = EFI_UNSUPPORTED;
+ } else {
+ return EFI_SUCCESS;
+ }
+ }
+ }
+ }
+ }
+ }
+ return Status;
+}
+
/**

Driver to produce Smbios protocol and pre-allocate 1 page for the final SMBIOS table.
@@ -1451,5 +1766,6 @@ SmbiosDriverEntryPoint (
&mPrivateData.Smbios
);

- return Status;
That doesn't compile as Status is written, but never read.

+ RetrieveSmbiosFromHob (ImageHandle);
+ return EFI_SUCCESS;
}
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
index f97c85ae40..a260cf695e 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
@@ -1,7 +1,7 @@
/** @file
This code supports the implementation of the Smbios protocol

-Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -24,6 +24,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/PcdLib.h>
+#include <Library/HobLib.h>
+#include <UniversalPayload/SmbiosTable.h>

#define SMBIOS_INSTANCE_SIGNATURE SIGNATURE_32 ('S', 'B', 'i', 's')
typedef struct {
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
index f6c036e1dc..63f468936d 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
@@ -1,7 +1,7 @@
## @file
# This driver initializes and installs the SMBIOS protocol, constructs SMBIOS table into system configuration table.
#
-# Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -41,6 +41,7 @@
UefiDriverEntryPoint
DebugLib
PcdLib
+ HobLib

[Protocols]
gEfiSmbiosProtocolGuid ## PRODUCES
@@ -48,6 +49,8 @@
[Guids]
gEfiSmbiosTableGuid ## SOMETIMES_PRODUCES ## SystemTable
gEfiSmbios3TableGuid ## SOMETIMES_PRODUCES ## SystemTable
+ gPldSmbios3TableGuid ## CONSUMES ## HOB
+ gPldSmbiosTableGuid ## SOMETIMES_CONSUMES ## HOB

[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion ## CONSUMES
--
2.30.0.windows.2


Re: [PATCH RFC v3 05/22] OvmfPkg: reserve Secrets page in MEMFD

Laszlo Ersek
 

On 06/07/21 17:58, Brijesh Singh wrote:

On 6/7/21 7:26 AM, Laszlo Ersek wrote:
On 05/27/21 01:11, Brijesh Singh wrote:
BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C32a95d87f0984b88080708d929af878f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586656154129803%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=JyrMLVE%2BMNq%2B1sUTI7WnbxkjloKi81PcISiLvz2geLg%3D&amp;reserved=0

When AMD SEV is enabled in the guest VM, a hypervisor need to insert a
secrets page.
For pure SEV?
The secrets page is applicable to all the SEV's (SEV, SEV-ES and
SEV-SNP) but there is some difference see below.



When SEV-SNP is enabled, the secrets page contains the VM platform
communication keys. The guest BIOS and OS can use this key to communicate
with the SEV firmware to get attesation report. See the SEV-SNP firmware
spec for more details for the content of the secrets page.

When SEV and SEV-ES is enabled, the secrets page contains the information
provided by the guest owner after the attestation. See the SEV
LAUNCH_SECRET command for more details.

Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
OvmfPkg/OvmfPkgX64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.fdf | 5 +++++
OvmfPkg/AmdSev/SecretPei/SecretPei.inf | 1 +
OvmfPkg/AmdSev/SecretPei/SecretPei.c | 15 ++++++++++++++-
4 files changed, 22 insertions(+), 1 deletion(-)
How is all of the above related to the "OvmfPkg/OvmfPkgX64.dsc"
platform, where remote attestation is not a goal?

What you describe makes sense to me, but only for the remote-attested
"OvmfPkg/AmdSev/AmdSevX64.dsc" platform. (Which already includes
SecretPei and SecretDxe, and sets the necessary PCDs.)

Then, even if we limit this patch only to the "OvmfPkg/AmdSev/SecretPei"
module, the commit message does not explain sufficiently why the secrets
page must be reserved for good. The "SEV-SNP firmware spec" reference is
vague at best; I'm permanently lost between the dozen PDF files I have
downloaded locally from the AMD website. Please include a specific
document number, revision number, and chapter/section identifier.

There is a fundamental difference between SEV and SEV-SNP attestation
flow. In the case of SEV and SEV-ES, the attestation happens before the
VM is booted, and the secrets page contains the data provided by the
guest owner after the attestation is complete. The hypervisor injects
that data into the guest memory before booting it.  However, with
SEV-SNP, the guest uses the data from the secrets page to build a
message for the PSP. The guest can send the following message to the PSP:

1. Expand the filtered CPUID list
2. Query attestation report
2. Derive a key
3. VM export, import, and absorb -- migration specific command

See chapter 7 [1] for all possible commands that a guest can send to PSP
through the guest message request. I understand that it is confusing,
but the secrets page is *not* same as SEV/SEV-ES. But since SEV-SNP spec
calls it secrets, so I used the same name. 
I thought the secrets page was entirely opaque to the guest firmware;
i.e., all the guest firmware would do with it is (a) cover it with an
allocation in SecretPei, (b) forward it to the guest OS via a UEFI
system config table in SecretDxe.

This patch uses the same PCD names ("launch secret", where I understand
the SEV-SNP case *not* to be a *launch* secret; is that right?), plus it
uses the same drivers. That's way too confusing.


So what is this "SNP secrets" page supposed to contain:

- both the previously defined SEV/SEV-ES level launch secret, and the
SNP-specific VMPCK (?)

- how are these secret bits separated from each other in the page?

- does the guest (firmware and/or OS) *write* to the new locations in
the page, possibly for secure message construction?


Either way, I think the proposed repurposing of the page, for the sake
of SNP secrets (VMPCK and maybe even secure message construction?),
breaks the current declarations of the PCDs, in "OvmfPkg.dec":

## The base address and size of the SEV Launch Secret Area provisioned
# after remote attestation. If this is set in the .fdf, the platform
# is responsible for protecting the area from DXE phase overwrites.
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43


In SEV-SNP, the secrets page is not tight up with just the remote
attestation.
This is the most important statement. We need the SNP secrets page even
without remote attestation. OvmfPkgX64.dsc does not deal with remote
attestation.

But then (putting all the PCD naming confusion aside), if a driver is
promoted to "common use", from the AmdSevX64 platform to multiple
OvmfPkg platforms, then it should be lifted to the top-level OvmfPkg
directory.

Later, the AmdSev.dsc can include a library to perform the
SEV-SNP-specific attestation. The library can use the SNP secrets page
to get the key and message counter use for constructing the guest
message to query the attestation report.

I hope it clarifies it.

[1] https://www.amd.com/system/files/TechDocs/56860.pdf


Honestly I'm getting a *rushed* vibe on this whole series. Why is that?
I am not sure why you are getting this feel, please let me know where I
can help to clarify but the series is *rushed* at all. Its building on
existing support. It's possible that we are getting mixed with the
fundamental difference between the SEV and SEV-SNP attestation flow and
recent patches from Dov to expand the attestation to cover other aspects
of the boot flow.

In case of SEV-SNP, some folks may prefer to do all the attestation in
the OVMF and others may prefer to do the attestation in the guest OS. We
should try to not restrict one approach over the other.



Assume that I'm dumb. You won't be far from the truth. Then hold my hand
through all this?

Please let me know if the above explanation helps or I should expand more.
You should please (a) expand your *commit messages*, (b) add a *wall* of
text in the "OvmfPkg.dec" file, where the PCDs in questions are
declared. When I grep the OvmfPkg subdirectory in two years for
"PcdSevLaunchSecretBase", I'd like to find the DEC file's comments to be
consistent with the actual uses of the PCD, and I'd like git-blame to
tell me something useful about those lines, too.


One problem is that I'm supposed to internalize about 50 pages from yet
from another technical specification, in order to get the basics of a
single patch. I can't even follow the *set* of AMD documents I should
have a local copy of. How am I supposed to interleave all that with, for
example, reviewing a 57 slide TDX design presentation?

Honestly, this has gone off the rails. The pressure that Confidential
Computing has generated for me as an OvmfPkg co-maintainer over the
course of a few months exceeds what I've been under for nearly a
*decade*, including all prior work with SEV and SEV-ES.

This makes me incredibly unhappy.

Laszlo




Laszlo


diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 999738dc39cd..ea08e1fabc65 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -716,6 +716,7 @@ [Components]
OvmfPkg/SmmAccess/SmmAccessPei.inf
!endif
UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+ OvmfPkg/AmdSev/SecretPei/SecretPei.inf

!if $(TPM_ENABLE) == TRUE
OvmfPkg/Tcg/TpmMmioSevDecryptPei/TpmMmioSevDecryptPei.inf
@@ -966,6 +967,7 @@ [Components]
OvmfPkg/PlatformDxe/Platform.inf
OvmfPkg/AmdSevDxe/AmdSevDxe.inf
OvmfPkg/IoMmuDxe/IoMmuDxe.inf
+ OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

!if $(SMM_REQUIRE) == TRUE
OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index d6be798fcadd..9126b8eb5014 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -88,6 +88,9 @@ [FD.MEMFD]
0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00D000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

@@ -179,6 +182,7 @@ [FV.PEIFV]
INF SecurityPkg/Tcg/TcgPei/TcgPei.inf
INF SecurityPkg/Tcg/Tcg2Pei/Tcg2Pei.inf
!endif
+INF OvmfPkg/AmdSev/SecretPei/SecretPei.inf

################################################################################

@@ -314,6 +318,7 @@ [FV.DXEFV]
INF ShellPkg/Application/Shell/Shell.inf

INF MdeModulePkg/Logo/LogoDxe.inf
+INF OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf

#
# Network modules
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
index 08be156c4bc0..9265f8adee12 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.inf
@@ -26,6 +26,7 @@ [LibraryClasses]
HobLib
PeimEntryPoint
PcdLib
+ MemEncryptSevLib

[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/AmdSev/SecretPei/SecretPei.c b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
index ad491515dd5d..51eb094555aa 100644
--- a/OvmfPkg/AmdSev/SecretPei/SecretPei.c
+++ b/OvmfPkg/AmdSev/SecretPei/SecretPei.c
@@ -7,6 +7,7 @@
#include <PiPei.h>
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
+#include <Library/MemEncryptSevLib.h>

EFI_STATUS
EFIAPI
@@ -15,10 +16,22 @@ InitializeSecretPei (
IN CONST EFI_PEI_SERVICES **PeiServices
)
{
+ UINTN Type;
+
+ //
+ // The location of the secret page should be marked reserved so that guest OS
+ // does not treated as a system RAM.
+ //
+ if (MemEncryptSevSnpIsEnabled ()) {
+ Type = EfiReservedMemoryType;
+ } else {
+ Type = EfiBootServicesData;
+ }
+
BuildMemoryAllocationHob (
PcdGet32 (PcdSevLaunchSecretBase),
PcdGet32 (PcdSevLaunchSecretSize),
- EfiBootServicesData
+ Type
);

return EFI_SUCCESS;

7901 - 7920 of 84035