* [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
@ 2018-03-28 20:26 ` Laszlo Ersek
2018-03-29 1:34 ` Zeng, Star
2018-03-28 20:26 ` [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize Laszlo Ersek
` (5 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-28 20:26 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Eric Dong, Ruiyu Ni, Star Zeng
The variable driver doesn't distinguish "non-volatile non-authenticated"
variables from "volatile non-authenticated" variables, when checking
individual variable sizes against the permitted maximum.
PcdMaxVariableSize covers both kinds.
This prevents volatile non-authenticated variables from carrying large
data between UEFI drivers, despite having no flash impact. One example is
EFI_TLS_CA_CERTIFICATE_VARIABLE, which platforms might want to create as
volatile on every boot: the certificate list can be several hundred KB in
size.
Introduce PcdMaxVolatileVariableSize to represent the limit on individual
volatile non-authenticated variables. The default value is zero, which
makes Variable/RuntimeDxe fall back to PcdMaxVariableSize (i.e. the
current behavior). This is similar to the PcdMaxAuthVariableSize fallback.
Whenever the size limit is enforced, consult MaxVolatileVariableSize as
the last option, after checking
- MaxAuthVariableSize for VARIABLE_ATTRIBUTE_AT_AW,
- and MaxVariableSize for EFI_VARIABLE_NON_VOLATILE.
EFI_VARIABLE_HARDWARE_ERROR_RECORD is always handled separately; it always
takes priority over the three cases listed above.
Introduce the GetMaxVariableSize() helper to consider
PcdMaxVolatileVariableSize, in addition to
GetNonVolatileMaxVariableSize(). GetNonVolatileMaxVariableSize() is
currently called at three sites, and two of those need to start using
GetMaxVariableSize() instead:
- VariableServiceInitialize() [VariableSmm.c]: the SMM comms buffer must
accommodate all kinds of variables,
- VariableCommonInitialize() [Variable.c]: the preallocated scratch space
must also accommodate all kinds of variables,
- InitNonVolatileVariableStore() [Variable.c] can continue using
GetNonVolatileMaxVariableSize().
Don't modify the ReclaimForOS() function as it is specific to non-volatile
variables and should ignore PcdMaxVolatileVariableSize.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/MdeModulePkg.dec | 8 ++++
MdeModulePkg/MdeModulePkg.uni | 8 ++++
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 +++++
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 +++++++++++++++++---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
7 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 5d561ff48495..cc397185f7b9 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1043,6 +1043,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
# @Prompt Maximum authenticated variable size.
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x00|UINT32|0x30000009
+ ## The maximum size of a single non-authenticated volatile variable.
+ # The default value is 0 for compatibility: in that case, the maximum
+ # non-authenticated volatile variable size remains specified by
+ # PcdMaxVariableSize. Only the MdeModulePkg/Universal/Variable/RuntimeDxe
+ # driver supports this PCD.
+ # @Prompt Maximum non-authenticated volatile variable size.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x00|UINT32|0x3000000a
+
## The maximum size of single hardware error record variable.<BR><BR>
# In IA32/X64 platforms, this value should be larger than 1KB.<BR>
# In IA64 platforms, this value should be larger than 128KB.<BR>
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index f3fa616438b0..080b8a62c045 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -94,6 +94,14 @@
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxAuthVariableSize_HELP #language en-US "The maximum size of a single authenticated variable."
"The value is 0 as default for compatibility that maximum authenticated variable size is specified by PcdMaxVariableSize."
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_PROMPT #language en-US "The maximum size of a single non-authenticated volatile variable."
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_HELP #language en-US "The maximum size of a single non-authenticated volatile variable.<BR><BR>\n"
+ "The default value is 0 for compatibility: in that case, the maximum "
+ "non-authenticated volatile variable size remains specified by "
+ "PcdMaxVariableSize.<BR>\n"
+ "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.<BR>"
+
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_PROMPT #language en-US "Maximum HwErr variable size"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_HELP #language en-US "The maximum size of single hardware error record variable.<BR><BR>\n"
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index e840fc9bff40..2d0a172ece35 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -123,6 +123,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index 69966f0d37ee..dbb0674a46ad 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -125,6 +125,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
index b35e8ab91273..938eb5de61fa 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
@@ -101,6 +101,7 @@ typedef struct {
UINTN HwErrVariableTotalSize;
UINTN MaxVariableSize;
UINTN MaxAuthVariableSize;
+ UINTN MaxVolatileVariableSize;
UINTN ScratchBufferSize;
CHAR8 *PlatformLangCodes;
CHAR8 *LangCodes;
@@ -460,6 +461,17 @@ GetNonVolatileMaxVariableSize (
VOID
);
+/**
+ Get maximum variable size, covering both non-volatile and volatile variables.
+
+ @return Maximum variable size.
+
+**/
+UINTN
+GetMaxVariableSize (
+ VOID
+ );
+
/**
Initializes variable write service after FVB was ready.
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index c11842b5c3ba..5a9051648004 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2345,12 +2345,14 @@ UpdateVariable (
CopyMem (BufferForMerge, (UINT8 *) ((UINTN) CacheVariable->CurrPtr + DataOffset), DataSizeOfVariable (CacheVariable->CurrPtr));
//
- // Set Max Common/Auth Variable Data Size as default MaxDataSize.
+ // Set Max Auth/Non-Volatile/Volatile Variable Data Size as default MaxDataSize.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
MaxDataSize = mVariableModuleGlobal->MaxAuthVariableSize - DataOffset;
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
MaxDataSize = mVariableModuleGlobal->MaxVariableSize - DataOffset;
+ } else {
+ MaxDataSize = mVariableModuleGlobal->MaxVolatileVariableSize - DataOffset;
}
//
@@ -3218,16 +3220,20 @@ VariableServiceSetVariable (
} else {
//
// The size of the VariableName, including the Unicode Null in bytes plus
- // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
+ // the DataSize is limited to maximum size of Max(Auth|Volatile)VariableSize bytes.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
return EFI_INVALID_PARAMETER;
}
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
return EFI_INVALID_PARAMETER;
}
+ } else {
+ if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ()) {
+ return EFI_INVALID_PARAMETER;
+ }
}
}
@@ -3399,12 +3405,14 @@ VariableServiceQueryVariableInfoInternal (
}
//
- // Let *MaximumVariableSize be Max(Auth)VariableSize with the exception of the variable header size.
+ // Let *MaximumVariableSize be Max(Auth|Volatile)VariableSize with the exception of the variable header size.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
*MaximumVariableSize = mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ();
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
*MaximumVariableSize = mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ();
+ } else {
+ *MaximumVariableSize = mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ();
}
}
@@ -3657,6 +3665,30 @@ GetNonVolatileMaxVariableSize (
}
}
+/**
+ Get maximum variable size, covering both non-volatile and volatile variables.
+
+ @return Maximum variable size.
+
+**/
+UINTN
+GetMaxVariableSize (
+ VOID
+ )
+{
+ UINTN MaxVariableSize;
+
+ MaxVariableSize = GetNonVolatileMaxVariableSize();
+ //
+ // The condition below fails implicitly if PcdMaxVolatileVariableSize equals
+ // the default zero value.
+ //
+ if (MaxVariableSize < PcdGet32 (PcdMaxVolatileVariableSize)) {
+ MaxVariableSize = PcdGet32 (PcdMaxVolatileVariableSize);
+ }
+ return MaxVariableSize;
+}
+
/**
Init non-volatile variable store.
@@ -3810,6 +3842,10 @@ InitNonVolatileVariableStore (
mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
+ mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
+ PcdGet32 (PcdMaxVolatileVariableSize) :
+ mVariableModuleGlobal->MaxVariableSize
+ );
//
// Parse non-volatile variable data and get last variable offset.
@@ -4228,7 +4264,7 @@ VariableCommonInitialize (
//
// Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
//
- ScratchSize = GetNonVolatileMaxVariableSize ();
+ ScratchSize = GetMaxVariableSize ();
mVariableModuleGlobal->ScratchBufferSize = ScratchSize;
VolatileVariableStore = AllocateRuntimePool (PcdGet32 (PcdVariableStoreSize) + ScratchSize);
if (VolatileVariableStore == NULL) {
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index 8d73b6edee51..e495d971a08b 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -955,7 +955,7 @@ VariableServiceInitialize (
);
ASSERT_EFI_ERROR (Status);
- mVariableBufferPayloadSize = GetNonVolatileMaxVariableSize () +
+ mVariableBufferPayloadSize = GetMaxVariableSize () +
OFFSET_OF (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) - GetVariableHeaderSize ();
Status = gSmst->SmmAllocatePool (
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
2018-03-28 20:26 ` [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize Laszlo Ersek
@ 2018-03-29 1:34 ` Zeng, Star
2018-03-29 12:19 ` Laszlo Ersek
0 siblings, 1 reply; 16+ messages in thread
From: Zeng, Star @ 2018-03-29 1:34 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Ni, Ruiyu, Zeng, Star
Laszlo,
Good patch and I assume you have done some tests with the patch. :)
One very minor comment.
Do you think the MaxVolatileVariableSize field assignment is better to be in VariableCommonInitialize(), but not InitNonVolatileVariableStore()?
mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
PcdGet32 (PcdMaxVolatileVariableSize) :
mVariableModuleGlobal->MaxVariableSize
);
For example, have it right before the comments in VariableCommonInitialize().
//
// Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
//
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, March 29, 2018 4:27 AM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
The variable driver doesn't distinguish "non-volatile non-authenticated"
variables from "volatile non-authenticated" variables, when checking individual variable sizes against the permitted maximum.
PcdMaxVariableSize covers both kinds.
This prevents volatile non-authenticated variables from carrying large data between UEFI drivers, despite having no flash impact. One example is EFI_TLS_CA_CERTIFICATE_VARIABLE, which platforms might want to create as volatile on every boot: the certificate list can be several hundred KB in size.
Introduce PcdMaxVolatileVariableSize to represent the limit on individual volatile non-authenticated variables. The default value is zero, which makes Variable/RuntimeDxe fall back to PcdMaxVariableSize (i.e. the current behavior). This is similar to the PcdMaxAuthVariableSize fallback.
Whenever the size limit is enforced, consult MaxVolatileVariableSize as the last option, after checking
- MaxAuthVariableSize for VARIABLE_ATTRIBUTE_AT_AW,
- and MaxVariableSize for EFI_VARIABLE_NON_VOLATILE.
EFI_VARIABLE_HARDWARE_ERROR_RECORD is always handled separately; it always takes priority over the three cases listed above.
Introduce the GetMaxVariableSize() helper to consider PcdMaxVolatileVariableSize, in addition to GetNonVolatileMaxVariableSize(). GetNonVolatileMaxVariableSize() is currently called at three sites, and two of those need to start using
GetMaxVariableSize() instead:
- VariableServiceInitialize() [VariableSmm.c]: the SMM comms buffer must
accommodate all kinds of variables,
- VariableCommonInitialize() [Variable.c]: the preallocated scratch space
must also accommodate all kinds of variables,
- InitNonVolatileVariableStore() [Variable.c] can continue using
GetNonVolatileMaxVariableSize().
Don't modify the ReclaimForOS() function as it is specific to non-volatile variables and should ignore PcdMaxVolatileVariableSize.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
MdeModulePkg/MdeModulePkg.dec | 8 ++++
MdeModulePkg/MdeModulePkg.uni | 8 ++++
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 +++++
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 +++++++++++++++++---
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
7 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 5d561ff48495..cc397185f7b9 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1043,6 +1043,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
# @Prompt Maximum authenticated variable size.
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x00|UINT32|0x30000009
+ ## The maximum size of a single non-authenticated volatile variable.
+ # The default value is 0 for compatibility: in that case, the maximum
+ # non-authenticated volatile variable size remains specified by #
+ PcdMaxVariableSize. Only the
+ MdeModulePkg/Universal/Variable/RuntimeDxe
+ # driver supports this PCD.
+ # @Prompt Maximum non-authenticated volatile variable size.
+
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x00|UINT32|
+ 0x3000000a
+
## The maximum size of single hardware error record variable.<BR><BR>
# In IA32/X64 platforms, this value should be larger than 1KB.<BR>
# In IA64 platforms, this value should be larger than 128KB.<BR> diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index f3fa616438b0..080b8a62c045 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -94,6 +94,14 @@
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxAuthVariableSize_HELP #language en-US "The maximum size of a single authenticated variable."
"The value is 0 as default for compatibility that maximum authenticated variable size is specified by PcdMaxVariableSize."
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_PROMPT #language en-US "The maximum size of a single non-authenticated volatile variable."
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_HELP #language en-US "The maximum size of a single non-authenticated volatile variable.<BR><BR>\n"
+ "The default value is 0 for compatibility: in that case, the maximum "
+ "non-authenticated volatile variable size remains specified by "
+ "PcdMaxVariableSize.<BR>\n"
+ "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.<BR>"
+
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_PROMPT #language en-US "Maximum HwErr variable size"
#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_HELP #language en-US "The maximum size of single hardware error record variable.<BR><BR>\n"
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index e840fc9bff40..2d0a172ece35 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -123,6 +123,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
index 69966f0d37ee..dbb0674a46ad 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
@@ -125,6 +125,7 @@ [Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
index b35e8ab91273..938eb5de61fa 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
@@ -101,6 +101,7 @@ typedef struct {
UINTN HwErrVariableTotalSize;
UINTN MaxVariableSize;
UINTN MaxAuthVariableSize;
+ UINTN MaxVolatileVariableSize;
UINTN ScratchBufferSize;
CHAR8 *PlatformLangCodes;
CHAR8 *LangCodes;
@@ -460,6 +461,17 @@ GetNonVolatileMaxVariableSize (
VOID
);
+/**
+ Get maximum variable size, covering both non-volatile and volatile variables.
+
+ @return Maximum variable size.
+
+**/
+UINTN
+GetMaxVariableSize (
+ VOID
+ );
+
/**
Initializes variable write service after FVB was ready.
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index c11842b5c3ba..5a9051648004 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2345,12 +2345,14 @@ UpdateVariable (
CopyMem (BufferForMerge, (UINT8 *) ((UINTN) CacheVariable->CurrPtr + DataOffset), DataSizeOfVariable (CacheVariable->CurrPtr));
//
- // Set Max Common/Auth Variable Data Size as default MaxDataSize.
+ // Set Max Auth/Non-Volatile/Volatile Variable Data Size as default MaxDataSize.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
MaxDataSize = mVariableModuleGlobal->MaxAuthVariableSize - DataOffset;
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
MaxDataSize = mVariableModuleGlobal->MaxVariableSize - DataOffset;
+ } else {
+ MaxDataSize = mVariableModuleGlobal->MaxVolatileVariableSize
+ - DataOffset;
}
//
@@ -3218,16 +3220,20 @@ VariableServiceSetVariable (
} else {
//
// The size of the VariableName, including the Unicode Null in bytes plus
- // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
+ // the DataSize is limited to maximum size of Max(Auth|Volatile)VariableSize bytes.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
return EFI_INVALID_PARAMETER;
}
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
return EFI_INVALID_PARAMETER;
}
+ } else {
+ if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ()) {
+ return EFI_INVALID_PARAMETER;
+ }
}
}
@@ -3399,12 +3405,14 @@ VariableServiceQueryVariableInfoInternal (
}
//
- // Let *MaximumVariableSize be Max(Auth)VariableSize with the exception of the variable header size.
+ // Let *MaximumVariableSize be Max(Auth|Volatile)VariableSize with the exception of the variable header size.
//
if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
*MaximumVariableSize = mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ();
- } else {
+ } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
*MaximumVariableSize = mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ();
+ } else {
+ *MaximumVariableSize =
+ mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize
+ ();
}
}
@@ -3657,6 +3665,30 @@ GetNonVolatileMaxVariableSize (
}
}
+/**
+ Get maximum variable size, covering both non-volatile and volatile variables.
+
+ @return Maximum variable size.
+
+**/
+UINTN
+GetMaxVariableSize (
+ VOID
+ )
+{
+ UINTN MaxVariableSize;
+
+ MaxVariableSize = GetNonVolatileMaxVariableSize();
+ //
+ // The condition below fails implicitly if PcdMaxVolatileVariableSize
+equals
+ // the default zero value.
+ //
+ if (MaxVariableSize < PcdGet32 (PcdMaxVolatileVariableSize)) {
+ MaxVariableSize = PcdGet32 (PcdMaxVolatileVariableSize);
+ }
+ return MaxVariableSize;
+}
+
/**
Init non-volatile variable store.
@@ -3810,6 +3842,10 @@ InitNonVolatileVariableStore (
mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
+ mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
+ PcdGet32 (PcdMaxVolatileVariableSize) :
+ mVariableModuleGlobal->MaxVariableSize
+ );
//
// Parse non-volatile variable data and get last variable offset.
@@ -4228,7 +4264,7 @@ VariableCommonInitialize (
//
// Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
//
- ScratchSize = GetNonVolatileMaxVariableSize ();
+ ScratchSize = GetMaxVariableSize ();
mVariableModuleGlobal->ScratchBufferSize = ScratchSize;
VolatileVariableStore = AllocateRuntimePool (PcdGet32 (PcdVariableStoreSize) + ScratchSize);
if (VolatileVariableStore == NULL) {
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
index 8d73b6edee51..e495d971a08b 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
@@ -955,7 +955,7 @@ VariableServiceInitialize (
);
ASSERT_EFI_ERROR (Status);
- mVariableBufferPayloadSize = GetNonVolatileMaxVariableSize () +
+ mVariableBufferPayloadSize = GetMaxVariableSize () +
OFFSET_OF (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) - GetVariableHeaderSize ();
Status = gSmst->SmmAllocatePool (
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
2018-03-29 1:34 ` Zeng, Star
@ 2018-03-29 12:19 ` Laszlo Ersek
2018-03-30 0:54 ` Zeng, Star
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-29 12:19 UTC (permalink / raw)
To: Zeng, Star, edk2-devel-01; +Cc: Dong, Eric, Ni, Ruiyu
On 03/29/18 03:34, Zeng, Star wrote:
> Laszlo,
>
> Good patch and I assume you have done some tests with the patch. :)
Yes, I have. For example I re-ran the Secure Boot Logo Test (the
automated one) from Microsoft HCK, using a "controller" VM (Windows
Server 2008R2) and a "client" VM (Windows Server 2016). This test
massages variables quite heavily.
I've never used the UEFI SCT; if someone could help with that, that
would be great.
> One very minor comment.
>
> Do you think the MaxVolatileVariableSize field assignment is better to be in VariableCommonInitialize(), but not InitNonVolatileVariableStore()?
>
> mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> PcdGet32 (PcdMaxVolatileVariableSize) :
> mVariableModuleGlobal->MaxVariableSize
> );
That's a good point! InitNonVolatileVariableStore() is not the right
place, judged simply by the function name. I originally put it there for
keeping the "size" PCDs close together, but there's likely a better spot.
> For example, have it right before the comments in VariableCommonInitialize().
>
> //
> // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
> //
OK, I'll do that in v2.
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, March 29, 2018 4:27 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
>
> The variable driver doesn't distinguish "non-volatile non-authenticated"
> variables from "volatile non-authenticated" variables, when checking individual variable sizes against the permitted maximum.
> PcdMaxVariableSize covers both kinds.
>
> This prevents volatile non-authenticated variables from carrying large data between UEFI drivers, despite having no flash impact. One example is EFI_TLS_CA_CERTIFICATE_VARIABLE, which platforms might want to create as volatile on every boot: the certificate list can be several hundred KB in size.
>
> Introduce PcdMaxVolatileVariableSize to represent the limit on individual volatile non-authenticated variables. The default value is zero, which makes Variable/RuntimeDxe fall back to PcdMaxVariableSize (i.e. the current behavior). This is similar to the PcdMaxAuthVariableSize fallback.
>
> Whenever the size limit is enforced, consult MaxVolatileVariableSize as the last option, after checking
> - MaxAuthVariableSize for VARIABLE_ATTRIBUTE_AT_AW,
> - and MaxVariableSize for EFI_VARIABLE_NON_VOLATILE.
>
> EFI_VARIABLE_HARDWARE_ERROR_RECORD is always handled separately; it always takes priority over the three cases listed above.
>
> Introduce the GetMaxVariableSize() helper to consider PcdMaxVolatileVariableSize, in addition to GetNonVolatileMaxVariableSize(). GetNonVolatileMaxVariableSize() is currently called at three sites, and two of those need to start using
> GetMaxVariableSize() instead:
> - VariableServiceInitialize() [VariableSmm.c]: the SMM comms buffer must
> accommodate all kinds of variables,
> - VariableCommonInitialize() [Variable.c]: the preallocated scratch space
> must also accommodate all kinds of variables,
> - InitNonVolatileVariableStore() [Variable.c] can continue using
> GetNonVolatileMaxVariableSize().
>
> Don't modify the ReclaimForOS() function as it is specific to non-volatile variables and should ignore PcdMaxVolatileVariableSize.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/MdeModulePkg.dec | 8 ++++
> MdeModulePkg/MdeModulePkg.uni | 8 ++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 +++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 +++++++++++++++++---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
> 7 files changed, 74 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 5d561ff48495..cc397185f7b9 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1043,6 +1043,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
> # @Prompt Maximum authenticated variable size.
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x00|UINT32|0x30000009
>
> + ## The maximum size of a single non-authenticated volatile variable.
> + # The default value is 0 for compatibility: in that case, the maximum
> + # non-authenticated volatile variable size remains specified by #
> + PcdMaxVariableSize. Only the
> + MdeModulePkg/Universal/Variable/RuntimeDxe
> + # driver supports this PCD.
> + # @Prompt Maximum non-authenticated volatile variable size.
> +
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x00|UINT32|
> + 0x3000000a
> +
> ## The maximum size of single hardware error record variable.<BR><BR>
> # In IA32/X64 platforms, this value should be larger than 1KB.<BR>
> # In IA64 platforms, this value should be larger than 128KB.<BR> diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index f3fa616438b0..080b8a62c045 100644
> --- a/MdeModulePkg/MdeModulePkg.uni
> +++ b/MdeModulePkg/MdeModulePkg.uni
> @@ -94,6 +94,14 @@
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxAuthVariableSize_HELP #language en-US "The maximum size of a single authenticated variable."
> "The value is 0 as default for compatibility that maximum authenticated variable size is specified by PcdMaxVariableSize."
>
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_PROMPT #language en-US "The maximum size of a single non-authenticated volatile variable."
> +
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_HELP #language en-US "The maximum size of a single non-authenticated volatile variable.<BR><BR>\n"
> + "The default value is 0 for compatibility: in that case, the maximum "
> + "non-authenticated volatile variable size remains specified by "
> + "PcdMaxVariableSize.<BR>\n"
> + "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.<BR>"
> +
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_PROMPT #language en-US "Maximum HwErr variable size"
>
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_HELP #language en-US "The maximum size of single hardware error record variable.<BR><BR>\n"
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index e840fc9bff40..2d0a172ece35 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -123,6 +123,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 69966f0d37ee..dbb0674a46ad 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -125,6 +125,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> index b35e8ab91273..938eb5de61fa 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> @@ -101,6 +101,7 @@ typedef struct {
> UINTN HwErrVariableTotalSize;
> UINTN MaxVariableSize;
> UINTN MaxAuthVariableSize;
> + UINTN MaxVolatileVariableSize;
> UINTN ScratchBufferSize;
> CHAR8 *PlatformLangCodes;
> CHAR8 *LangCodes;
> @@ -460,6 +461,17 @@ GetNonVolatileMaxVariableSize (
> VOID
> );
>
> +/**
> + Get maximum variable size, covering both non-volatile and volatile variables.
> +
> + @return Maximum variable size.
> +
> +**/
> +UINTN
> +GetMaxVariableSize (
> + VOID
> + );
> +
> /**
> Initializes variable write service after FVB was ready.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index c11842b5c3ba..5a9051648004 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2345,12 +2345,14 @@ UpdateVariable (
> CopyMem (BufferForMerge, (UINT8 *) ((UINTN) CacheVariable->CurrPtr + DataOffset), DataSizeOfVariable (CacheVariable->CurrPtr));
>
> //
> - // Set Max Common/Auth Variable Data Size as default MaxDataSize.
> + // Set Max Auth/Non-Volatile/Volatile Variable Data Size as default MaxDataSize.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> MaxDataSize = mVariableModuleGlobal->MaxAuthVariableSize - DataOffset;
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> MaxDataSize = mVariableModuleGlobal->MaxVariableSize - DataOffset;
> + } else {
> + MaxDataSize = mVariableModuleGlobal->MaxVolatileVariableSize
> + - DataOffset;
> }
>
> //
> @@ -3218,16 +3220,20 @@ VariableServiceSetVariable (
> } else {
> //
> // The size of the VariableName, including the Unicode Null in bytes plus
> - // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
> + // the DataSize is limited to maximum size of Max(Auth|Volatile)VariableSize bytes.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> + } else {
> + if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ()) {
> + return EFI_INVALID_PARAMETER;
> + }
> }
> }
>
> @@ -3399,12 +3405,14 @@ VariableServiceQueryVariableInfoInternal (
> }
>
> //
> - // Let *MaximumVariableSize be Max(Auth)VariableSize with the exception of the variable header size.
> + // Let *MaximumVariableSize be Max(Auth|Volatile)VariableSize with the exception of the variable header size.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> *MaximumVariableSize = mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ();
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> *MaximumVariableSize = mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ();
> + } else {
> + *MaximumVariableSize =
> + mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize
> + ();
> }
> }
>
> @@ -3657,6 +3665,30 @@ GetNonVolatileMaxVariableSize (
> }
> }
>
> +/**
> + Get maximum variable size, covering both non-volatile and volatile variables.
> +
> + @return Maximum variable size.
> +
> +**/
> +UINTN
> +GetMaxVariableSize (
> + VOID
> + )
> +{
> + UINTN MaxVariableSize;
> +
> + MaxVariableSize = GetNonVolatileMaxVariableSize();
> + //
> + // The condition below fails implicitly if PcdMaxVolatileVariableSize
> +equals
> + // the default zero value.
> + //
> + if (MaxVariableSize < PcdGet32 (PcdMaxVolatileVariableSize)) {
> + MaxVariableSize = PcdGet32 (PcdMaxVolatileVariableSize);
> + }
> + return MaxVariableSize;
> +}
> +
> /**
> Init non-volatile variable store.
>
> @@ -3810,6 +3842,10 @@ InitNonVolatileVariableStore (
>
> mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
> mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
> + mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> + PcdGet32 (PcdMaxVolatileVariableSize) :
> + mVariableModuleGlobal->MaxVariableSize
> + );
>
> //
> // Parse non-volatile variable data and get last variable offset.
> @@ -4228,7 +4264,7 @@ VariableCommonInitialize (
> //
> // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
> //
> - ScratchSize = GetNonVolatileMaxVariableSize ();
> + ScratchSize = GetMaxVariableSize ();
> mVariableModuleGlobal->ScratchBufferSize = ScratchSize;
> VolatileVariableStore = AllocateRuntimePool (PcdGet32 (PcdVariableStoreSize) + ScratchSize);
> if (VolatileVariableStore == NULL) {
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 8d73b6edee51..e495d971a08b 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -955,7 +955,7 @@ VariableServiceInitialize (
> );
> ASSERT_EFI_ERROR (Status);
>
> - mVariableBufferPayloadSize = GetNonVolatileMaxVariableSize () +
> + mVariableBufferPayloadSize = GetMaxVariableSize () +
> OFFSET_OF (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) - GetVariableHeaderSize ();
>
> Status = gSmst->SmmAllocatePool (
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
2018-03-29 12:19 ` Laszlo Ersek
@ 2018-03-30 0:54 ` Zeng, Star
0 siblings, 0 replies; 16+ messages in thread
From: Zeng, Star @ 2018-03-30 0:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Dong, Eric, Ni, Ruiyu, Zeng, Star
No V2 needed for this case, Reviewed-by: Star Zeng <star.zeng@intel.com> with my comment handled.
Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, March 29, 2018 8:19 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: Re: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
On 03/29/18 03:34, Zeng, Star wrote:
> Laszlo,
>
> Good patch and I assume you have done some tests with the patch. :)
Yes, I have. For example I re-ran the Secure Boot Logo Test (the automated one) from Microsoft HCK, using a "controller" VM (Windows Server 2008R2) and a "client" VM (Windows Server 2016). This test massages variables quite heavily.
I've never used the UEFI SCT; if someone could help with that, that would be great.
> One very minor comment.
>
> Do you think the MaxVolatileVariableSize field assignment is better to be in VariableCommonInitialize(), but not InitNonVolatileVariableStore()?
>
> mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> PcdGet32 (PcdMaxVolatileVariableSize) :
> mVariableModuleGlobal->MaxVariableSize
> );
That's a good point! InitNonVolatileVariableStore() is not the right place, judged simply by the function name. I originally put it there for keeping the "size" PCDs close together, but there's likely a better spot.
> For example, have it right before the comments in VariableCommonInitialize().
>
> //
> // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
> //
OK, I'll do that in v2.
Thanks!
Laszlo
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, March 29, 2018 4:27 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce
> PcdMaxVolatileVariableSize
>
> The variable driver doesn't distinguish "non-volatile non-authenticated"
> variables from "volatile non-authenticated" variables, when checking individual variable sizes against the permitted maximum.
> PcdMaxVariableSize covers both kinds.
>
> This prevents volatile non-authenticated variables from carrying large data between UEFI drivers, despite having no flash impact. One example is EFI_TLS_CA_CERTIFICATE_VARIABLE, which platforms might want to create as volatile on every boot: the certificate list can be several hundred KB in size.
>
> Introduce PcdMaxVolatileVariableSize to represent the limit on individual volatile non-authenticated variables. The default value is zero, which makes Variable/RuntimeDxe fall back to PcdMaxVariableSize (i.e. the current behavior). This is similar to the PcdMaxAuthVariableSize fallback.
>
> Whenever the size limit is enforced, consult MaxVolatileVariableSize
> as the last option, after checking
> - MaxAuthVariableSize for VARIABLE_ATTRIBUTE_AT_AW,
> - and MaxVariableSize for EFI_VARIABLE_NON_VOLATILE.
>
> EFI_VARIABLE_HARDWARE_ERROR_RECORD is always handled separately; it always takes priority over the three cases listed above.
>
> Introduce the GetMaxVariableSize() helper to consider
> PcdMaxVolatileVariableSize, in addition to
> GetNonVolatileMaxVariableSize(). GetNonVolatileMaxVariableSize() is
> currently called at three sites, and two of those need to start using
> GetMaxVariableSize() instead:
> - VariableServiceInitialize() [VariableSmm.c]: the SMM comms buffer must
> accommodate all kinds of variables,
> - VariableCommonInitialize() [Variable.c]: the preallocated scratch space
> must also accommodate all kinds of variables,
> - InitNonVolatileVariableStore() [Variable.c] can continue using
> GetNonVolatileMaxVariableSize().
>
> Don't modify the ReclaimForOS() function as it is specific to non-volatile variables and should ignore PcdMaxVolatileVariableSize.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> MdeModulePkg/MdeModulePkg.dec | 8 ++++
> MdeModulePkg/MdeModulePkg.uni | 8 ++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 +++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 +++++++++++++++++---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
> 7 files changed, 74 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 5d561ff48495..cc397185f7b9
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1043,6 +1043,14 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
> # @Prompt Maximum authenticated variable size.
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x00|UINT32|0x30
> 000009
>
> + ## The maximum size of a single non-authenticated volatile variable.
> + # The default value is 0 for compatibility: in that case, the
> + maximum # non-authenticated volatile variable size remains specified
> + by # PcdMaxVariableSize. Only the
> + MdeModulePkg/Universal/Variable/RuntimeDxe
> + # driver supports this PCD.
> + # @Prompt Maximum non-authenticated volatile variable size.
> +
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x00|UINT3
> + 2|
> + 0x3000000a
> +
> ## The maximum size of single hardware error record variable.<BR><BR>
> # In IA32/X64 platforms, this value should be larger than 1KB.<BR>
> # In IA64 platforms, this value should be larger than 128KB.<BR>
> diff --git a/MdeModulePkg/MdeModulePkg.uni
> b/MdeModulePkg/MdeModulePkg.uni index f3fa616438b0..080b8a62c045
> 100644
> --- a/MdeModulePkg/MdeModulePkg.uni
> +++ b/MdeModulePkg/MdeModulePkg.uni
> @@ -94,6 +94,14 @@
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxAuthVariableSize_HELP #language en-US "The maximum size of a single authenticated variable."
> "The value is 0 as default for compatibility that maximum authenticated variable size is specified by PcdMaxVariableSize."
>
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_PROMPT #language en-US "The maximum size of a single non-authenticated volatile variable."
> +
> +#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxVolatileVariableSize_HELP #language en-US "The maximum size of a single non-authenticated volatile variable.<BR><BR>\n"
> + "The default value is 0 for compatibility: in that case, the maximum "
> + "non-authenticated volatile variable size remains specified by "
> + "PcdMaxVariableSize.<BR>\n"
> + "Only the MdeModulePkg/Universal/Variable/RuntimeDxe driver supports this PCD.<BR>"
> +
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_PROMPT #language en-US "Maximum HwErr variable size"
>
> #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdMaxHardwareErrorVariableSize_HELP #language en-US "The maximum size of single hardware error record variable.<BR><BR>\n"
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index e840fc9bff40..2d0a172ece35 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.in
> +++ f
> @@ -123,6 +123,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index 69966f0d37ee..dbb0674a46ad 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -125,6 +125,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxHardwareErrorVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdHwErrStorageSize ## CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> index b35e8ab91273..938eb5de61fa 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> @@ -101,6 +101,7 @@ typedef struct {
> UINTN HwErrVariableTotalSize;
> UINTN MaxVariableSize;
> UINTN MaxAuthVariableSize;
> + UINTN MaxVolatileVariableSize;
> UINTN ScratchBufferSize;
> CHAR8 *PlatformLangCodes;
> CHAR8 *LangCodes;
> @@ -460,6 +461,17 @@ GetNonVolatileMaxVariableSize (
> VOID
> );
>
> +/**
> + Get maximum variable size, covering both non-volatile and volatile variables.
> +
> + @return Maximum variable size.
> +
> +**/
> +UINTN
> +GetMaxVariableSize (
> + VOID
> + );
> +
> /**
> Initializes variable write service after FVB was ready.
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index c11842b5c3ba..5a9051648004 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2345,12 +2345,14 @@ UpdateVariable (
> CopyMem (BufferForMerge, (UINT8 *) ((UINTN)
> CacheVariable->CurrPtr + DataOffset), DataSizeOfVariable
> (CacheVariable->CurrPtr));
>
> //
> - // Set Max Common/Auth Variable Data Size as default MaxDataSize.
> + // Set Max Auth/Non-Volatile/Volatile Variable Data Size as default MaxDataSize.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> MaxDataSize = mVariableModuleGlobal->MaxAuthVariableSize - DataOffset;
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> MaxDataSize = mVariableModuleGlobal->MaxVariableSize -
> DataOffset;
> + } else {
> + MaxDataSize =
> + mVariableModuleGlobal->MaxVolatileVariableSize
> + - DataOffset;
> }
>
> //
> @@ -3218,16 +3220,20 @@ VariableServiceSetVariable (
> } else {
> //
> // The size of the VariableName, including the Unicode Null in bytes plus
> - // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
> + // the DataSize is limited to maximum size of Max(Auth|Volatile)VariableSize bytes.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> + } else {
> + if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVolatileVariableSize - GetVariableHeaderSize ()) {
> + return EFI_INVALID_PARAMETER;
> + }
> }
> }
>
> @@ -3399,12 +3405,14 @@ VariableServiceQueryVariableInfoInternal (
> }
>
> //
> - // Let *MaximumVariableSize be Max(Auth)VariableSize with the exception of the variable header size.
> + // Let *MaximumVariableSize be Max(Auth|Volatile)VariableSize with the exception of the variable header size.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> *MaximumVariableSize = mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ();
> - } else {
> + } else if ((Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
> *MaximumVariableSize = mVariableModuleGlobal->MaxVariableSize -
> GetVariableHeaderSize ();
> + } else {
> + *MaximumVariableSize =
> + mVariableModuleGlobal->MaxVolatileVariableSize -
> + mVariableModuleGlobal->GetVariableHeaderSize
> + ();
> }
> }
>
> @@ -3657,6 +3665,30 @@ GetNonVolatileMaxVariableSize (
> }
> }
>
> +/**
> + Get maximum variable size, covering both non-volatile and volatile variables.
> +
> + @return Maximum variable size.
> +
> +**/
> +UINTN
> +GetMaxVariableSize (
> + VOID
> + )
> +{
> + UINTN MaxVariableSize;
> +
> + MaxVariableSize = GetNonVolatileMaxVariableSize();
> + //
> + // The condition below fails implicitly if
> +PcdMaxVolatileVariableSize equals
> + // the default zero value.
> + //
> + if (MaxVariableSize < PcdGet32 (PcdMaxVolatileVariableSize)) {
> + MaxVariableSize = PcdGet32 (PcdMaxVolatileVariableSize);
> + }
> + return MaxVariableSize;
> +}
> +
> /**
> Init non-volatile variable store.
>
> @@ -3810,6 +3842,10 @@ InitNonVolatileVariableStore (
>
> mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
> mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32
> (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) :
> mVariableModuleGlobal->MaxVariableSize);
> + mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> + PcdGet32 (PcdMaxVolatileVariableSize) :
> + mVariableModuleGlobal->MaxVariableSize
> + );
>
> //
> // Parse non-volatile variable data and get last variable offset.
> @@ -4228,7 +4264,7 @@ VariableCommonInitialize (
> //
> // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
> //
> - ScratchSize = GetNonVolatileMaxVariableSize ();
> + ScratchSize = GetMaxVariableSize ();
> mVariableModuleGlobal->ScratchBufferSize = ScratchSize;
> VolatileVariableStore = AllocateRuntimePool (PcdGet32 (PcdVariableStoreSize) + ScratchSize);
> if (VolatileVariableStore == NULL) { diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 8d73b6edee51..e495d971a08b 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -955,7 +955,7 @@ VariableServiceInitialize (
> );
> ASSERT_EFI_ERROR (Status);
>
> - mVariableBufferPayloadSize = GetNonVolatileMaxVariableSize () +
> + mVariableBufferPayloadSize = GetMaxVariableSize () +
> OFFSET_OF
> (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) -
> GetVariableHeaderSize ();
>
> Status = gSmst->SmmAllocatePool (
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
2018-03-28 20:26 ` [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize Laszlo Ersek
@ 2018-03-28 20:26 ` Laszlo Ersek
2018-03-30 10:57 ` Ard Biesheuvel
2018-03-28 20:26 ` [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize" Laszlo Ersek
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-28 20:26 UTC (permalink / raw)
To: edk2-devel-01
Cc: Anthony Perard, Ard Biesheuvel, Gary Ching-Pang Lin,
Jordan Justen, Julien Grall
In commit 62f43f7c1947c, we set PcdVariableStoreSize to the same value as
PcdFlashNvStorageVariableSize (which in turn comes from VARS_LIVE_SIZE in
"OvmfPkg.fdf.inc").
This equality between both PCDs is a false requirement from
EmuVariableFvbRuntimeDxe. FVB drivers should use
PcdFlashNvStorageVariableSize for supporting non-volatile variables (even
if they happen to be kept in RAM only), along the other PcdFlashNvStorage*
PCDs. Whereas PcdVariableStoreSize is for variables that are volatile at
the gRT->SetVariable() / gRT->GetVariable() API level.
(PlatformPei too bases the preallocation for EmuVariableFvbRuntimeDxe on
PcdFlashNvStorageFtwSpareSize.)
Replace PcdVariableStoreSize in EmuVariableFvbRuntimeDxe with the
same-value PcdFlashNvStorageVariableSize. This means no change in
behavior, and it allows us to increase PcdVariableStoreSize in a later
patch without disturbing EmuVariableFvbRuntimeDxe (or PlatformPei).
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 3 +--
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 6 +++---
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
index 9f37938408a4..2aacf06c923d 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
@@ -58,12 +58,11 @@ [Protocols]
gEfiDevicePathProtocolGuid # PROTOCOL ALWAYS_PRODUCED
[FixedPcd]
- gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
+ gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
[Pcd]
- gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 2d106bb50bed..9480d879c935 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -665,7 +665,7 @@ InitializeFvAndVariableStoreHeaders (
// UINT32 Size;
(
- FixedPcdGet32 (PcdVariableStoreSize) -
+ FixedPcdGet32 (PcdFlashNvStorageVariableSize) -
OFFSET_OF (FVB_FV_HDR_AND_VARS_TEMPLATE, VarHdr)
),
@@ -733,7 +733,7 @@ FvbInitialize (
ASSERT (FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
EMU_FVB_BLOCK_SIZE == 0);
if (
- (PcdGet32 (PcdVariableStoreSize) +
+ (PcdGet32 (PcdFlashNvStorageVariableSize) +
PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
) >
EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE
@@ -788,7 +788,7 @@ FvbInitialize (
//
// Initialize the Fault Tolerant Write data area
//
- SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
+ SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdFlashNvStorageVariableSize));
PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
(UINT32)(UINTN) SubPtr);
ASSERT_RETURN_ERROR (PcdStatus);
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize
2018-03-28 20:26 ` [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize Laszlo Ersek
@ 2018-03-30 10:57 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-03-30 10:57 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Anthony Perard, Gary Ching-Pang Lin, Jordan Justen,
Julien Grall
On 28 March 2018 at 21:26, Laszlo Ersek <lersek@redhat.com> wrote:
> In commit 62f43f7c1947c, we set PcdVariableStoreSize to the same value as
> PcdFlashNvStorageVariableSize (which in turn comes from VARS_LIVE_SIZE in
> "OvmfPkg.fdf.inc").
>
> This equality between both PCDs is a false requirement from
> EmuVariableFvbRuntimeDxe. FVB drivers should use
> PcdFlashNvStorageVariableSize for supporting non-volatile variables (even
> if they happen to be kept in RAM only), along the other PcdFlashNvStorage*
> PCDs. Whereas PcdVariableStoreSize is for variables that are volatile at
> the gRT->SetVariable() / gRT->GetVariable() API level.
>
> (PlatformPei too bases the preallocation for EmuVariableFvbRuntimeDxe on
> PcdFlashNvStorageFtwSpareSize.)
>
> Replace PcdVariableStoreSize in EmuVariableFvbRuntimeDxe with the
> same-value PcdFlashNvStorageVariableSize. This means no change in
> behavior, and it allows us to increase PcdVariableStoreSize in a later
> patch without disturbing EmuVariableFvbRuntimeDxe (or PlatformPei).
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 3 +--
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 6 +++---
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
> index 9f37938408a4..2aacf06c923d 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf
> @@ -58,12 +58,11 @@ [Protocols]
> gEfiDevicePathProtocolGuid # PROTOCOL ALWAYS_PRODUCED
>
> [FixedPcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
> + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> index 2d106bb50bed..9480d879c935 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> @@ -665,7 +665,7 @@ InitializeFvAndVariableStoreHeaders (
>
> // UINT32 Size;
> (
> - FixedPcdGet32 (PcdVariableStoreSize) -
> + FixedPcdGet32 (PcdFlashNvStorageVariableSize) -
> OFFSET_OF (FVB_FV_HDR_AND_VARS_TEMPLATE, VarHdr)
> ),
>
> @@ -733,7 +733,7 @@ FvbInitialize (
> ASSERT (FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
> EMU_FVB_BLOCK_SIZE == 0);
> if (
> - (PcdGet32 (PcdVariableStoreSize) +
> + (PcdGet32 (PcdFlashNvStorageVariableSize) +
> PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
> ) >
> EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE
> @@ -788,7 +788,7 @@ FvbInitialize (
> //
> // Initialize the Fault Tolerant Write data area
> //
> - SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
> + SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdFlashNvStorageVariableSize));
> PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
> (UINT32)(UINTN) SubPtr);
> ASSERT_RETURN_ERROR (PcdStatus);
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize"
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
2018-03-28 20:26 ` [PATCH 1/4] MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize Laszlo Ersek
2018-03-28 20:26 ` [PATCH 2/4] OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize Laszlo Ersek
@ 2018-03-28 20:26 ` Laszlo Ersek
2018-03-30 10:58 ` Ard Biesheuvel
2018-03-28 20:26 ` [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot Laszlo Ersek
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-28 20:26 UTC (permalink / raw)
To: edk2-devel-01
Cc: Anthony Perard, Ard Biesheuvel, Gary Ching-Pang Lin,
Jordan Justen, Julien Grall
As a continuation of the last patch, clarify in the DSC files that we set
PcdVariableStoreSize to the same value as PcdFlashNvStorageVariableSize
just for convenience; the equality is not a technical requirement.
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 2 ++
OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
OvmfPkg/OvmfPkgX64.dsc | 2 ++
3 files changed, 6 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 92c8c560a067..7664b50ddef9 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -439,11 +439,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 6ecaa795b288..e5969090d437 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -444,11 +444,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c98a3657c6f6..7197c1984a7c 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -444,11 +444,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ # match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize"
2018-03-28 20:26 ` [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize" Laszlo Ersek
@ 2018-03-30 10:58 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-03-30 10:58 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Anthony Perard, Gary Ching-Pang Lin, Jordan Justen,
Julien Grall
On 28 March 2018 at 21:26, Laszlo Ersek <lersek@redhat.com> wrote:
> As a continuation of the last patch, clarify in the DSC files that we set
> PcdVariableStoreSize to the same value as PcdFlashNvStorageVariableSize
> just for convenience; the equality is not a technical requirement.
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 2 ++
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 ++
> OvmfPkg/OvmfPkgX64.dsc | 2 ++
> 3 files changed, 6 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 92c8c560a067..7664b50ddef9 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -439,11 +439,13 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 6ecaa795b288..e5969090d437 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -444,11 +444,13 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index c98a3657c6f6..7197c1984a7c 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -444,11 +444,13 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> + # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
>
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
` (2 preceding siblings ...)
2018-03-28 20:26 ` [PATCH 3/4] OvmfPkg: annotate "PcdVariableStoreSize := PcdFlashNvStorageVariableSize" Laszlo Ersek
@ 2018-03-28 20:26 ` Laszlo Ersek
2018-03-30 11:00 ` Ard Biesheuvel
2018-03-29 4:56 ` [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list " Palmer, Thomas
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-28 20:26 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Gary Ching-Pang Lin, Jordan Justen
Introduce TlsAuthConfigLib to read the list of trusted CA certificates
from fw_cfg and to store it to EFI_TLS_CA_CERTIFICATE_VARIABLE.
The fw_cfg file is formatted by the "p11-kit" and "update-ca-trust"
utilities on the host side, so that the host settings take effect in guest
HTTPS boot as well. QEMU forwards the file intact to the firmware. The
contents are sanity-checked by NetworkPkg/HttpDxe code that was added in
commit 0fd13678a681.
Link TlsAuthConfigLib via NULL resolution into TlsAuthConfigDxe. This sets
EFI_TLS_CA_CERTIFICATE_VARIABLE in time for both
NetworkPkg/TlsAuthConfigDxe (for possible HII interaction with the user)
and for NetworkPkg/HttpDxe (for the effective TLS configuration).
The file formatted by "p11-kit" can be large. On a RHEL-7 host, the the
Mozilla CA root certificate bundle -- installed with the "ca-certificates"
package -- is processed into a 182KB file. Thus, create
EFI_TLS_CA_CERTIFICATE_VARIABLE as a volatile & boot-time only variable.
Also, in TLS_ENABLE builds, set the cumulative limit for volatile
variables (PcdVariableStoreSize) to 512KB, and the individual limit for
the same (PcdMaxVolatileVariableSize) to 256KB.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 13 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 13 +-
OvmfPkg/OvmfPkgX64.dsc | 13 +-
OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf | 55 ++++++++
OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c | 133 ++++++++++++++++++++
5 files changed, 224 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 7664b50ddef9..c9eb248506c5 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -439,15 +439,23 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
+!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
+!endif
+!if $(TLS_ENABLE) == TRUE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
@@ -796,7 +804,10 @@ [Components]
!endif
!if $(TLS_ENABLE) == TRUE
NetworkPkg/TlsDxe/TlsDxe.inf
- NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
+ NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
+ <LibraryClasses>
+ NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
+ }
!endif
OvmfPkg/VirtioNetDxe/VirtioNet.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index e5969090d437..17aef2d4830f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -444,15 +444,23 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
+!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
+!endif
+!if $(TLS_ENABLE) == TRUE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
@@ -805,7 +813,10 @@ [Components.X64]
!endif
!if $(TLS_ENABLE) == TRUE
NetworkPkg/TlsDxe/TlsDxe.inf
- NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
+ NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
+ <LibraryClasses>
+ NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
+ }
!endif
OvmfPkg/VirtioNetDxe/VirtioNet.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 7197c1984a7c..8af763ea9e9e 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -444,15 +444,23 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
+!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!if $(TLS_ENABLE) == FALSE
# match PcdFlashNvStorageVariableSize purely for convenience
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
+!endif
+!if $(TLS_ENABLE) == TRUE
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
@@ -803,7 +811,10 @@ [Components]
!endif
!if $(TLS_ENABLE) == TRUE
NetworkPkg/TlsDxe/TlsDxe.inf
- NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
+ NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
+ <LibraryClasses>
+ NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
+ }
!endif
OvmfPkg/VirtioNetDxe/VirtioNet.inf
diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
new file mode 100644
index 000000000000..5f83582a8313
--- /dev/null
+++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
@@ -0,0 +1,55 @@
+## @file
+#
+# A hook-in library for NetworkPkg/TlsAuthConfigDxe, in order to set volatile
+# variables related to TLS configuration, before TlsAuthConfigDxe or HttpDxe
+# (which is a UEFI_DRIVER) consume them.
+#
+# Copyright (C) 2013, 2015, 2018, Red Hat, Inc.
+# Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution. The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+[Defines]
+ INF_VERSION = 1.26
+ BASE_NAME = TlsAuthConfigLib
+ FILE_GUID = 660AB627-4C5F-4D42-A3B6-BD021E9028BD
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = TlsAuthConfigLib|DXE_DRIVER
+ CONSTRUCTOR = TlsAuthConfigInit
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
+#
+
+[Sources]
+ TlsAuthConfigLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ NetworkPkg/NetworkPkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
+ QemuFwCfgLib
+ UefiRuntimeServicesTableLib
+
+[Guids]
+ gEfiTlsCaCertificateGuid ## PRODUCES ## Variable:L"TlsCaCertificate"
+
+[Depex]
+ gEfiVariableWriteArchProtocolGuid
diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
new file mode 100644
index 000000000000..b5b33bc4fc69
--- /dev/null
+++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
@@ -0,0 +1,133 @@
+/** @file
+
+ A hook-in library for NetworkPkg/TlsAuthConfigDxe, in order to set volatile
+ variables related to TLS configuration, before TlsAuthConfigDxe or HttpDxe
+ (which is a UEFI_DRIVER) consume them.
+
+ Copyright (C) 2013, 2015, 2018, Red Hat, Inc.
+ Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
+
+ This program and the accompanying materials are licensed and made available
+ under the terms and conditions of the BSD License which accompanies this
+ distribution. The full text of the license may be found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Uefi/UefiBaseType.h>
+#include <Uefi/UefiSpec.h>
+
+#include <Guid/TlsAuthentication.h>
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
+
+/**
+ Read the list of trusted CA certificates from the fw_cfg file
+ "etc/edk2/https/cacerts", and store it to
+ gEfiTlsCaCertificateGuid:EFI_TLS_CA_CERTIFICATE_VARIABLE.
+
+ The contents are validated (for well-formedness) by NetworkPkg/HttpDxe.
+**/
+STATIC
+VOID
+SetCaCerts (
+ VOID
+ )
+{
+ EFI_STATUS Status;
+ FIRMWARE_CONFIG_ITEM HttpsCaCertsItem;
+ UINTN HttpsCaCertsSize;
+ VOID *HttpsCaCerts;
+
+ Status = QemuFwCfgFindFile ("etc/edk2/https/cacerts", &HttpsCaCertsItem,
+ &HttpsCaCertsSize);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_VERBOSE, "%a:%a: not touching CA cert list\n",
+ gEfiCallerBaseName, __FUNCTION__));
+ return;
+ }
+
+ //
+ // Delete the current EFI_TLS_CA_CERTIFICATE_VARIABLE if it exists. This
+ // serves two purposes:
+ //
+ // (a) If the variable exists with EFI_VARIABLE_NON_VOLATILE attribute, we
+ // cannot make it volatile without deleting it first.
+ //
+ // (b) If we fail to recreate the variable later, deleting the current one is
+ // still justified if the fw_cfg file exists. Emptying the set of trusted
+ // CA certificates will fail HTTPS boot, which is better than trusting
+ // any certificate that's possibly missing from the fw_cfg file.
+ //
+ Status = gRT->SetVariable (
+ EFI_TLS_CA_CERTIFICATE_VARIABLE, // VariableName
+ &gEfiTlsCaCertificateGuid, // VendorGuid
+ 0, // Attributes
+ 0, // DataSize
+ NULL // Data
+ );
+ if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
+ //
+ // This is fatal.
+ //
+ DEBUG ((DEBUG_ERROR, "%a:%a: failed to delete %g:\"%s\"\n",
+ gEfiCallerBaseName, __FUNCTION__, &gEfiTlsCaCertificateGuid,
+ EFI_TLS_CA_CERTIFICATE_VARIABLE));
+ ASSERT_EFI_ERROR (Status);
+ CpuDeadLoop ();
+ }
+
+ if (HttpsCaCertsSize == 0) {
+ DEBUG ((DEBUG_VERBOSE, "%a:%a: applied empty CA cert list\n",
+ gEfiCallerBaseName, __FUNCTION__));
+ return;
+ }
+
+ HttpsCaCerts = AllocatePool (HttpsCaCertsSize);
+ if (HttpsCaCerts == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate HttpsCaCerts\n",
+ gEfiCallerBaseName, __FUNCTION__));
+ return;
+ }
+
+ QemuFwCfgSelectItem (HttpsCaCertsItem);
+ QemuFwCfgReadBytes (HttpsCaCertsSize, HttpsCaCerts);
+
+ Status = gRT->SetVariable (
+ EFI_TLS_CA_CERTIFICATE_VARIABLE, // VariableName
+ &gEfiTlsCaCertificateGuid, // VendorGuid
+ EFI_VARIABLE_BOOTSERVICE_ACCESS, // Attributes
+ HttpsCaCertsSize, // DataSize
+ HttpsCaCerts // Data
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a:%a: failed to set %g:\"%s\": %r\n",
+ gEfiCallerBaseName, __FUNCTION__, &gEfiTlsCaCertificateGuid,
+ EFI_TLS_CA_CERTIFICATE_VARIABLE, Status));
+ goto FreeHttpsCaCerts;
+ }
+
+ DEBUG ((DEBUG_VERBOSE, "%a:%a: stored CA cert list (%Lu byte(s))\n",
+ gEfiCallerBaseName, __FUNCTION__, (UINT64)HttpsCaCertsSize));
+
+FreeHttpsCaCerts:
+ FreePool (HttpsCaCerts);
+}
+
+RETURN_STATUS
+EFIAPI
+TlsAuthConfigInit (
+ VOID
+ )
+{
+ SetCaCerts ();
+
+ return RETURN_SUCCESS;
+}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot
2018-03-28 20:26 ` [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot Laszlo Ersek
@ 2018-03-30 11:00 ` Ard Biesheuvel
0 siblings, 0 replies; 16+ messages in thread
From: Ard Biesheuvel @ 2018-03-30 11:00 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Gary Ching-Pang Lin, Jordan Justen
On 28 March 2018 at 21:26, Laszlo Ersek <lersek@redhat.com> wrote:
> Introduce TlsAuthConfigLib to read the list of trusted CA certificates
> from fw_cfg and to store it to EFI_TLS_CA_CERTIFICATE_VARIABLE.
>
> The fw_cfg file is formatted by the "p11-kit" and "update-ca-trust"
> utilities on the host side, so that the host settings take effect in guest
> HTTPS boot as well. QEMU forwards the file intact to the firmware. The
> contents are sanity-checked by NetworkPkg/HttpDxe code that was added in
> commit 0fd13678a681.
>
> Link TlsAuthConfigLib via NULL resolution into TlsAuthConfigDxe. This sets
> EFI_TLS_CA_CERTIFICATE_VARIABLE in time for both
> NetworkPkg/TlsAuthConfigDxe (for possible HII interaction with the user)
> and for NetworkPkg/HttpDxe (for the effective TLS configuration).
>
> The file formatted by "p11-kit" can be large. On a RHEL-7 host, the the
> Mozilla CA root certificate bundle -- installed with the "ca-certificates"
> package -- is processed into a 182KB file. Thus, create
> EFI_TLS_CA_CERTIFICATE_VARIABLE as a volatile & boot-time only variable.
> Also, in TLS_ENABLE builds, set the cumulative limit for volatile
> variables (PcdVariableStoreSize) to 512KB, and the individual limit for
> the same (PcdMaxVolatileVariableSize) to 256KB.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> OvmfPkg/OvmfPkgIa32.dsc | 13 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 13 +-
> OvmfPkg/OvmfPkgX64.dsc | 13 +-
> OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf | 55 ++++++++
> OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c | 133 ++++++++++++++++++++
> 5 files changed, 224 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 7664b50ddef9..c9eb248506c5 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -439,15 +439,23 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> +!endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> +!endif
> +!if $(TLS_ENABLE) == TRUE
> + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
> +!endif
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> @@ -796,7 +804,10 @@ [Components]
> !endif
> !if $(TLS_ENABLE) == TRUE
> NetworkPkg/TlsDxe/TlsDxe.inf
> - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> + NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> + <LibraryClasses>
> + NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> + }
> !endif
> OvmfPkg/VirtioNetDxe/VirtioNet.inf
>
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index e5969090d437..17aef2d4830f 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -444,15 +444,23 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> +!endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> +!endif
> +!if $(TLS_ENABLE) == TRUE
> + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
> +!endif
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> @@ -805,7 +813,10 @@ [Components.X64]
> !endif
> !if $(TLS_ENABLE) == TRUE
> NetworkPkg/TlsDxe/TlsDxe.inf
> - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> + NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> + <LibraryClasses>
> + NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> + }
> !endif
> OvmfPkg/VirtioNetDxe/VirtioNet.inf
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 7197c1984a7c..8af763ea9e9e 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -444,15 +444,23 @@ [PcdsFixedAtBuild]
> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> !endif
> +!endif
> !if $(FD_SIZE_IN_KB) == 4096
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!if $(TLS_ENABLE) == FALSE
> # match PcdFlashNvStorageVariableSize purely for convenience
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> !endif
> +!endif
> +!if $(TLS_ENABLE) == TRUE
> + gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x40000
> +!endif
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> @@ -803,7 +811,10 @@ [Components]
> !endif
> !if $(TLS_ENABLE) == TRUE
> NetworkPkg/TlsDxe/TlsDxe.inf
> - NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
> + NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> + <LibraryClasses>
> + NULL|OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> + }
> !endif
> OvmfPkg/VirtioNetDxe/VirtioNet.inf
>
> diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> new file mode 100644
> index 000000000000..5f83582a8313
> --- /dev/null
> +++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
> @@ -0,0 +1,55 @@
> +## @file
> +#
> +# A hook-in library for NetworkPkg/TlsAuthConfigDxe, in order to set volatile
> +# variables related to TLS configuration, before TlsAuthConfigDxe or HttpDxe
> +# (which is a UEFI_DRIVER) consume them.
> +#
> +# Copyright (C) 2013, 2015, 2018, Red Hat, Inc.
> +# Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
> +#
> +# This program and the accompanying materials are licensed and made available
> +# under the terms and conditions of the BSD License which accompanies this
> +# distribution. The full text of the license may be found at
> +# http://opensource.org/licenses/bsd-license.php
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +##
> +
> +[Defines]
> + INF_VERSION = 1.26
> + BASE_NAME = TlsAuthConfigLib
> + FILE_GUID = 660AB627-4C5F-4D42-A3B6-BD021E9028BD
> + MODULE_TYPE = BASE
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = TlsAuthConfigLib|DXE_DRIVER
> + CONSTRUCTOR = TlsAuthConfigInit
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64
> +#
> +
> +[Sources]
> + TlsAuthConfigLib.c
> +
> +[Packages]
> + MdePkg/MdePkg.dec
> + NetworkPkg/NetworkPkg.dec
> + OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> + BaseLib
> + DebugLib
> + MemoryAllocationLib
> + QemuFwCfgLib
> + UefiRuntimeServicesTableLib
> +
> +[Guids]
> + gEfiTlsCaCertificateGuid ## PRODUCES ## Variable:L"TlsCaCertificate"
> +
> +[Depex]
> + gEfiVariableWriteArchProtocolGuid
> diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
> new file mode 100644
> index 000000000000..b5b33bc4fc69
> --- /dev/null
> +++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
> @@ -0,0 +1,133 @@
> +/** @file
> +
> + A hook-in library for NetworkPkg/TlsAuthConfigDxe, in order to set volatile
> + variables related to TLS configuration, before TlsAuthConfigDxe or HttpDxe
> + (which is a UEFI_DRIVER) consume them.
> +
> + Copyright (C) 2013, 2015, 2018, Red Hat, Inc.
> + Copyright (c) 2008 - 2012, Intel Corporation. All rights reserved.<BR>
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution. The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Uefi/UefiSpec.h>
> +
> +#include <Guid/TlsAuthentication.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/QemuFwCfgLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +
> +/**
> + Read the list of trusted CA certificates from the fw_cfg file
> + "etc/edk2/https/cacerts", and store it to
> + gEfiTlsCaCertificateGuid:EFI_TLS_CA_CERTIFICATE_VARIABLE.
> +
> + The contents are validated (for well-formedness) by NetworkPkg/HttpDxe.
> +**/
> +STATIC
> +VOID
> +SetCaCerts (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + FIRMWARE_CONFIG_ITEM HttpsCaCertsItem;
> + UINTN HttpsCaCertsSize;
> + VOID *HttpsCaCerts;
> +
> + Status = QemuFwCfgFindFile ("etc/edk2/https/cacerts", &HttpsCaCertsItem,
> + &HttpsCaCertsSize);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_VERBOSE, "%a:%a: not touching CA cert list\n",
> + gEfiCallerBaseName, __FUNCTION__));
> + return;
> + }
> +
> + //
> + // Delete the current EFI_TLS_CA_CERTIFICATE_VARIABLE if it exists. This
> + // serves two purposes:
> + //
> + // (a) If the variable exists with EFI_VARIABLE_NON_VOLATILE attribute, we
> + // cannot make it volatile without deleting it first.
> + //
> + // (b) If we fail to recreate the variable later, deleting the current one is
> + // still justified if the fw_cfg file exists. Emptying the set of trusted
> + // CA certificates will fail HTTPS boot, which is better than trusting
> + // any certificate that's possibly missing from the fw_cfg file.
> + //
> + Status = gRT->SetVariable (
> + EFI_TLS_CA_CERTIFICATE_VARIABLE, // VariableName
> + &gEfiTlsCaCertificateGuid, // VendorGuid
> + 0, // Attributes
> + 0, // DataSize
> + NULL // Data
> + );
> + if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
> + //
> + // This is fatal.
> + //
> + DEBUG ((DEBUG_ERROR, "%a:%a: failed to delete %g:\"%s\"\n",
> + gEfiCallerBaseName, __FUNCTION__, &gEfiTlsCaCertificateGuid,
> + EFI_TLS_CA_CERTIFICATE_VARIABLE));
> + ASSERT_EFI_ERROR (Status);
> + CpuDeadLoop ();
> + }
> +
> + if (HttpsCaCertsSize == 0) {
> + DEBUG ((DEBUG_VERBOSE, "%a:%a: applied empty CA cert list\n",
> + gEfiCallerBaseName, __FUNCTION__));
> + return;
> + }
> +
> + HttpsCaCerts = AllocatePool (HttpsCaCertsSize);
> + if (HttpsCaCerts == NULL) {
> + DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate HttpsCaCerts\n",
> + gEfiCallerBaseName, __FUNCTION__));
> + return;
> + }
> +
> + QemuFwCfgSelectItem (HttpsCaCertsItem);
> + QemuFwCfgReadBytes (HttpsCaCertsSize, HttpsCaCerts);
> +
> + Status = gRT->SetVariable (
> + EFI_TLS_CA_CERTIFICATE_VARIABLE, // VariableName
> + &gEfiTlsCaCertificateGuid, // VendorGuid
> + EFI_VARIABLE_BOOTSERVICE_ACCESS, // Attributes
> + HttpsCaCertsSize, // DataSize
> + HttpsCaCerts // Data
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a:%a: failed to set %g:\"%s\": %r\n",
> + gEfiCallerBaseName, __FUNCTION__, &gEfiTlsCaCertificateGuid,
> + EFI_TLS_CA_CERTIFICATE_VARIABLE, Status));
> + goto FreeHttpsCaCerts;
> + }
> +
> + DEBUG ((DEBUG_VERBOSE, "%a:%a: stored CA cert list (%Lu byte(s))\n",
> + gEfiCallerBaseName, __FUNCTION__, (UINT64)HttpsCaCertsSize));
> +
> +FreeHttpsCaCerts:
> + FreePool (HttpsCaCerts);
> +}
> +
> +RETURN_STATUS
> +EFIAPI
> +TlsAuthConfigInit (
> + VOID
> + )
> +{
> + SetCaCerts ();
> +
> + return RETURN_SUCCESS;
> +}
> --
> 2.14.1.3.gb7cf6e02401b
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
` (3 preceding siblings ...)
2018-03-28 20:26 ` [PATCH 4/4] OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot Laszlo Ersek
@ 2018-03-29 4:56 ` Palmer, Thomas
2018-03-29 11:57 ` Laszlo Ersek
2018-03-30 4:39 ` Gary Lin
2018-03-30 19:43 ` Laszlo Ersek
6 siblings, 1 reply; 16+ messages in thread
From: Palmer, Thomas @ 2018-03-29 4:56 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Ruiyu Ni, Eric Dong, Ard Biesheuvel, Jordan Justen, Lin, Gary,
Anthony Perard, Star Zeng
Laszlo,
(First, are you are plugfest? Let's chat.)
Second, what need do you see for having KB worth of CA at UEFI's disposal? If HTTPS feature is primarily for PXE booting OS's, then it is likely the IT administrator who setup the PXE server also has a single CA they want use for PXE. By allowing any and every CA to be installed (instead of having the user pick only the immediately needed CAs), we inadvertently open HTTPS to state-backed/well-financed malicious actors who can pay for quality SSL signing services. (The less CAs then the less that can go wrong).
This is not to prevent your patches going in, but would like to ensure manufacturers / admins know how to properly use the CA list
Regards,
Thomas Palmer
"I have only made this letter longer because I have not had the time to make it shorter" - Blaise Pascal
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, March 28, 2018 3:27 PM
To: edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Eric Dong <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Jordan Justen <jordan.l.justen@intel.com>; Gary Ching-Pang Lin <glin@suse.com>; Anthony Perard <anthony.perard@citrix.com>; Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
Repo: https://github.com/lersek/edk2.git
Branch: https_cacert_rhbz_1536624
The trusted CA certificates for HTTPS boot can be specified in EFI_TLS_CA_CERTIFICATE_VARIABLE. The platform may choose to create this variable as volatile and set it on every boot as appropriate. The OVMF feature is that the virtualization host passes down an fw_cfg blob that carries the CA certs trusted on the host side, and the OVMF HTTPS boot will verify web servers against that certificate bundle. (For (part of) the host side implementation, refer to
<https://github.com/p11-glue/p11-kit/pull/137.)
The challenge for edk2 is that the CA cert list from the host side is huge; on my laptop it is 182KB when formatted to the EFI_SIGNATURE_LIST sequence expected by NetworkPkg/HttpDxe. Storing this in a non-volatile EFI_TLS_CA_CERTIFICATE_VARIABLE is out of the question, but even when making EFI_TLS_CA_CERTIFICATE_VARIABLE volatile, there are two limits that need raising:
(1) the individual limit on volatile variables,
(2) the cumulative limit on volatile variables.
Regarding (1), the edk2 variable driver does not distinguish a limit for volatile non-auth vs. non-volatile non-auth variables. The first patch introduces "PcdMaxVolatileVariableSize" for this, in a backwards compatible way (i.e. platforms that don't care need not learn about it).
The new PCD lets a platform raise the individual limit just for volatile non-auth variables.
Regarding (2), OvmfPkg/EmuVariableFvbRuntimeDxe has a bug where it abuses the cumulative limit on volatile variables for the live size of the emulated non-volatile variable store. The difference is that "volatile variables" are volatile on the UEFI service API level
(gRT->SetVariable() etc), and the driver stack expects the FVB impls to use the non-volatile storage PCDs (regardless of the actual FVB backing store). Patch #2 fixes this (without change in behavior) in OvmfPkg/EmuVariableFvbRuntimeDxe.
Patch #3 adds a bit of documentation to the OVMF DSC files, as a continuation of patch #2.
Patch #4 implements the feature, raising both limits (liberated in earlier patches) and populating EFI_TLS_CA_CERTIFICATE_VARIABLE from fw_cfg.
I've done reasonable HTTPS boot testing and regression testing too (including "-bios" with OVMF and pflash with ArmVirtQemu). Indepdent testing would be highly appreciated (feature and regression alike).
This email is too long and so are the commit messages, but I'm too tired to trim them; apologies.
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks,
Laszlo
Laszlo Ersek (4):
MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize
OvmfPkg: annotate "PcdVariableStoreSize :=
PcdFlashNvStorageVariableSize"
OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot
MdeModulePkg/MdeModulePkg.dec | 8 ++
MdeModulePkg/MdeModulePkg.uni | 8 ++
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 ++++++--
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 ++
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 6 +-
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 3 +-
OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c | 133 ++++++++++++++++++++
OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf | 55 ++++++++
OvmfPkg/OvmfPkgIa32.dsc | 15 ++-
OvmfPkg/OvmfPkgIa32X64.dsc | 15 ++-
OvmfPkg/OvmfPkgX64.dsc | 15 ++-
14 files changed, 308 insertions(+), 16 deletions(-) create mode 100644 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
create mode 100644 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
--
2.14.1.3.gb7cf6e02401b
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
2018-03-29 4:56 ` [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list " Palmer, Thomas
@ 2018-03-29 11:57 ` Laszlo Ersek
2018-03-29 18:17 ` Palmer, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-29 11:57 UTC (permalink / raw)
To: Palmer, Thomas, edk2-devel-01
Cc: Ruiyu Ni, Eric Dong, Ard Biesheuvel, Jordan Justen, Lin, Gary,
Anthony Perard, Star Zeng
On 03/29/18 06:56, Palmer, Thomas wrote:
> Laszlo,
>
> (First, are you are plugfest? Let's chat.)
I like chatting :) but I'm not at the plugfest. I travel only once per
year, and that's to the KVM Forum.
> Second, what need do you see for having KB worth of CA at UEFI's
> disposal? If HTTPS feature is primarily for PXE booting OS's,
> then it is likely the IT administrator who setup the PXE server
> also has a single CA they want use for PXE. By allowing any
> and every CA to be installed (instead of having the user pick
> only the immediately needed CAs), we inadvertently open HTTPS to
> state-backed/well-financed malicious actors who can pay for
> quality SSL signing services. (The less CAs then the less that
> can go wrong).
In a virt setup, we have to split this question in two.
(1) Why do we want to configure the guest from the host side?
(2) Why do we want to push down so many CA certs to the guest?
The answer to (1) is quite obvious: configuring the guest from the host
side allows for better automation, larger scale deployment, integration
with management tools etc.
Regarding (2), the premise is that the virtualization host administrator
has a carefully curated set of trusted CA certificates. It does not
necessarily need to be the full CA bundle from Mozilla, it just may be.
The feature that folks from our crypto and virt management tools teams
are requesting is that the HTTPS configuration in the guest, for OVMF
netbooting, not require *separate* administration from the host CA
curation.
(The same applies to the trusted cipher suites as well, and I'm going to
post patches for that too.)
> This is not to prevent your patches going in, but would like to
> ensure manufacturers / admins know how to properly use the CA
> list
Oh, definitely. The idea here is *absolutely not* to encourage platform
vendors (physical or virtual) to heap shady CAs into
EFI_TLS_CA_CERTIFICATE_VARIABLE. This work is all about mechanism, not
policy; I'm just building the conduit through wich the virt host admin
can send down the CA list that they have *carefully* vetted for
host-side use already. If that list contains just one certificate, so be
it.
In my testing, the 182KB number comes from the default CA bundle from
Mozilla (the "ca-certificates" package) on my RHEL-7 laptop, which -- as
you can likely tell :) -- I haven't personally filtered down.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
2018-03-29 11:57 ` Laszlo Ersek
@ 2018-03-29 18:17 ` Palmer, Thomas
0 siblings, 0 replies; 16+ messages in thread
From: Palmer, Thomas @ 2018-03-29 18:17 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Ruiyu Ni, Eric Dong, Ard Biesheuvel, Jordan Justen, Lin, Gary,
Anthony Perard, Star Zeng
Sorry to miss you this time, maybe next.
Yes, I was fine the fixes but wanted to know "why". Thanks
Regards,
Thomas Palmer
“I have only made this letter longer because I have not had the time to make it shorter” - Blaise Pascal
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, March 29, 2018 6:57 AM
To: Palmer, Thomas <thomas.palmer@hpe.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>; Eric Dong <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Jordan Justen <jordan.l.justen@intel.com>; Lin, Gary <GLin@suse.com>; Anthony Perard <anthony.perard@citrix.com>; Star Zeng <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
On 03/29/18 06:56, Palmer, Thomas wrote:
> Laszlo,
>
> (First, are you are plugfest? Let's chat.)
I like chatting :) but I'm not at the plugfest. I travel only once per year, and that's to the KVM Forum.
> Second, what need do you see for having KB worth of CA at UEFI's
> disposal? If HTTPS feature is primarily for PXE booting OS's,
> then it is likely the IT administrator who setup the PXE server
> also has a single CA they want use for PXE. By allowing any
> and every CA to be installed (instead of having the user pick
> only the immediately needed CAs), we inadvertently open HTTPS to
> state-backed/well-financed malicious actors who can pay for
> quality SSL signing services. (The less CAs then the less that
> can go wrong).
In a virt setup, we have to split this question in two.
(1) Why do we want to configure the guest from the host side?
(2) Why do we want to push down so many CA certs to the guest?
The answer to (1) is quite obvious: configuring the guest from the host side allows for better automation, larger scale deployment, integration with management tools etc.
Regarding (2), the premise is that the virtualization host administrator has a carefully curated set of trusted CA certificates. It does not necessarily need to be the full CA bundle from Mozilla, it just may be.
The feature that folks from our crypto and virt management tools teams are requesting is that the HTTPS configuration in the guest, for OVMF netbooting, not require *separate* administration from the host CA curation.
(The same applies to the trusted cipher suites as well, and I'm going to post patches for that too.)
> This is not to prevent your patches going in, but would like to
> ensure manufacturers / admins know how to properly use the CA
> list
Oh, definitely. The idea here is *absolutely not* to encourage platform vendors (physical or virtual) to heap shady CAs into EFI_TLS_CA_CERTIFICATE_VARIABLE. This work is all about mechanism, not policy; I'm just building the conduit through wich the virt host admin can send down the CA list that they have *carefully* vetted for host-side use already. If that list contains just one certificate, so be it.
In my testing, the 182KB number comes from the default CA bundle from Mozilla (the "ca-certificates" package) on my RHEL-7 laptop, which -- as you can likely tell :) -- I haven't personally filtered down.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
` (4 preceding siblings ...)
2018-03-29 4:56 ` [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list " Palmer, Thomas
@ 2018-03-30 4:39 ` Gary Lin
2018-03-30 19:43 ` Laszlo Ersek
6 siblings, 0 replies; 16+ messages in thread
From: Gary Lin @ 2018-03-30 4:39 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel-01, Anthony Perard, Ard Biesheuvel, Eric Dong,
Jordan Justen, Julien Grall, Ruiyu Ni, Star Zeng
On Wed, Mar 28, 2018 at 10:26:47PM +0200, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: https_cacert_rhbz_1536624
>
This patch series is great and I like it :)
Configuring the CA list dynamically is really useful and flexible for
the administrator.
I read the code and tested it with my self-signed server, and it worked
as expected.
Reviewed-by: Gary Lin <glin@suse.com>
Tested-by: Gary Lin <glin@suse.com>
> The trusted CA certificates for HTTPS boot can be specified in
> EFI_TLS_CA_CERTIFICATE_VARIABLE. The platform may choose to create this
> variable as volatile and set it on every boot as appropriate. The OVMF
> feature is that the virtualization host passes down an fw_cfg blob that
> carries the CA certs trusted on the host side, and the OVMF HTTPS boot
> will verify web servers against that certificate bundle. (For (part of)
> the host side implementation, refer to
> <https://github.com/p11-glue/p11-kit/pull/137.)
>
> The challenge for edk2 is that the CA cert list from the host side is
> huge; on my laptop it is 182KB when formatted to the EFI_SIGNATURE_LIST
> sequence expected by NetworkPkg/HttpDxe. Storing this in a non-volatile
> EFI_TLS_CA_CERTIFICATE_VARIABLE is out of the question, but even when
> making EFI_TLS_CA_CERTIFICATE_VARIABLE volatile, there are two limits
> that need raising:
>
> (1) the individual limit on volatile variables,
> (2) the cumulative limit on volatile variables.
>
> Regarding (1), the edk2 variable driver does not distinguish a limit for
> volatile non-auth vs. non-volatile non-auth variables. The first patch
> introduces "PcdMaxVolatileVariableSize" for this, in a backwards
> compatible way (i.e. platforms that don't care need not learn about it).
> The new PCD lets a platform raise the individual limit just for volatile
> non-auth variables.
>
> Regarding (2), OvmfPkg/EmuVariableFvbRuntimeDxe has a bug where it
> abuses the cumulative limit on volatile variables for the live size of
> the emulated non-volatile variable store. The difference is that
> "volatile variables" are volatile on the UEFI service API level
> (gRT->SetVariable() etc), and the driver stack expects the FVB impls to
> use the non-volatile storage PCDs (regardless of the actual FVB backing
> store). Patch #2 fixes this (without change in behavior) in
> OvmfPkg/EmuVariableFvbRuntimeDxe.
>
> Patch #3 adds a bit of documentation to the OVMF DSC files, as a
> continuation of patch #2.
>
> Patch #4 implements the feature, raising both limits (liberated in
> earlier patches) and populating EFI_TLS_CA_CERTIFICATE_VARIABLE from
> fw_cfg.
>
> I've done reasonable HTTPS boot testing and regression testing too
> (including "-bios" with OVMF and pflash with ArmVirtQemu). Indepdent
> testing would be highly appreciated (feature and regression alike).
>
> This email is too long and so are the commit messages, but I'm too tired
> to trim them; apologies.
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (4):
> MdeModulePkg/Variable/RuntimeDxe: introduce PcdMaxVolatileVariableSize
> OvmfPkg/EmuVariableFvbRuntimeDxe: stop using PcdVariableStoreSize
> OvmfPkg: annotate "PcdVariableStoreSize :=
> PcdFlashNvStorageVariableSize"
> OvmfPkg/TlsAuthConfigLib: configure trusted CA certs for HTTPS boot
>
> MdeModulePkg/MdeModulePkg.dec | 8 ++
> MdeModulePkg/MdeModulePkg.uni | 8 ++
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 50 ++++++--
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 12 ++
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 1 +
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 2 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 1 +
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 6 +-
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf | 3 +-
> OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c | 133 ++++++++++++++++++++
> OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf | 55 ++++++++
> OvmfPkg/OvmfPkgIa32.dsc | 15 ++-
> OvmfPkg/OvmfPkgIa32X64.dsc | 15 ++-
> OvmfPkg/OvmfPkgX64.dsc | 15 ++-
> 14 files changed, 308 insertions(+), 16 deletions(-)
> create mode 100644 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
> create mode 100644 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
>
> --
> 2.14.1.3.gb7cf6e02401b
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot
2018-03-28 20:26 [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list for HTTPS boot Laszlo Ersek
` (5 preceding siblings ...)
2018-03-30 4:39 ` Gary Lin
@ 2018-03-30 19:43 ` Laszlo Ersek
6 siblings, 0 replies; 16+ messages in thread
From: Laszlo Ersek @ 2018-03-30 19:43 UTC (permalink / raw)
To: edk2-devel-01
Cc: Ruiyu Ni, Eric Dong, Ard Biesheuvel, Jordan Justen,
Gary Ching-Pang Lin, Anthony Perard, Star Zeng
On 03/28/18 22:26, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: https_cacert_rhbz_1536624
According to Star's feedback, I modified patch #1 as follows:
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 5a9051648004..6caf603b3d30 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -3842,10 +3842,6 @@ InitNonVolatileVariableStore (
>
> mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
> mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
> - mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> - PcdGet32 (PcdMaxVolatileVariableSize) :
> - mVariableModuleGlobal->MaxVariableSize
> - );
>
> //
> // Parse non-volatile variable data and get last variable offset.
> @@ -4261,6 +4257,10 @@ VariableCommonInitialize (
> }
> }
>
> + mVariableModuleGlobal->MaxVolatileVariableSize = ((PcdGet32 (PcdMaxVolatileVariableSize) != 0) ?
> + PcdGet32 (PcdMaxVolatileVariableSize) :
> + mVariableModuleGlobal->MaxVariableSize
> + );
> //
> // Allocate memory for volatile variable store, note that there is a scratch space to store scratch data.
> //
and I added the feedback tags like this:
> Reviewed-by: Gary Lin <glin@suse.com>
> Tested-by: Gary Lin <glin@suse.com>
> [lersek@redhat.com: set MaxVolatileVariableSize where Star suggested]
> Reviewed-by: Star Zeng <star.zeng@intel.com>
To the rest of the patches, I applied the feedback tags as usual.
Before pushing the set, I retested it with an OVMF HTTPS boot, and I
regression-retested it with an ArmVirtQemu disk boot.
Pushed as commit range 3d7ebd643431..9c7d0d499296.
Thanks everyone!
Laszlo
^ permalink raw reply [flat|nested] 16+ messages in thread