Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V

[Dhaval Sharma]

Based on the above discussion, and some more thoughts I am thinking it is
okay to at least replace ASSERT from CMO code and let other platform code
place its own guards to avoid calling this code when it is known that
platform does not support such operations. If there are no objections to
this we can go ahead.

On Tue, Jan 9, 2024 at 9:50 PM Warkentin, Andrei 
wrote:

> For now, this is really something that ought to be hidden by DmaLib
> abstraction (Map/Unmap). This would allow the driver to be minimally aware
> of how the IP is integrated into the SoC.
>
> A
>
> > -Original Message-
> > From: Sunil V L 
> > Sent: Monday, January 8, 2024 11:32 PM
> > To: Pedro Falcato 
> > Cc: devel@edk2.groups.io; dha...@rivosinc.com; yorange
> > ; Warkentin, Andrei
> > ; Ard Biesheuvel  >;
> > Leif Lindholm 
> > Subject: Re: [edk2-devel] [PATCH v10 4/5] MdePkg: Utilize Cache
> > Management Operations Implementation For RISC-V
> >
> > On Mon, Jan 08, 2024 at 09:53:46PM +, Pedro Falcato wrote:
> > > On Mon, Jan 8, 2024 at 4:23 PM Dhaval Sharma 
> > wrote:
> > > >
> > > > Hi yangcheng/Pedro,
> > >
> > > +CC a bunch of relevant people
> > >
> > > Hi, (FYI you did not CC me)
> > >
> > > Looking at yangcheng's example:
> > >
> > >   Status = PrepareDmaData (gpIdmacDesc, Length, Buffer); <-- We write
> > > to the IDMAC desc
> > >   if (EFI_ERROR (Status)) {
> > > goto out;
> > >   }
> > >
> > >   WriteBackDataCacheRange (gpIdmacDesc, DescPages * EFI_PAGE_SIZE);
> > > <-- Make sure it's DMA-coherent
> > >   StartDma (Length); <-- We've flushed the cache, everything is now in
> > > DRAM and DMA-coherent, start DMA
> > >
> > > which screams of "bad abstractions" because you don't actually need to
> > > write data back, if the device and platform are DMA coherent.
> > >
> > > So what we want here really depends. My local "Volume I: RISC-V
> > > Unprivileged ISA V20191213" says, section A.5:
> > >
> > > "Table A.5 provides a mapping of Linux memory ordering macros onto
> > > RISC-V memory instructions.
> > > The Linux fences dma rmb() and dma wmb() map onto FENCE R,R and FENCE
> > > W,W, respectively, since the RISC-V Unix Platform requires coherent
> > > DMA, but would be mapped onto FENCE RI,RI and FENCE WO,WO,
> > > respectively, on a platform with non-coherent DMA.
> > > Platforms with non-
> > > coherent DMA may also require a mechanism by which cache lines can be
> > > flushed and/or invalidated.
> > > Such mechanisms will be device-specific and/or standardized in a
> > > future extension to the ISA."
> > >
> > > The (current date) RISCV Platform Spec also says: "Memory accesses by
> > > I/O masters can be coherent or non-coherent with respect to all
> > > hart-related caches."
> > > which is brilliantly useless.
> > >
> > > so I think the best solution here is to:
> > >
> > > 1) Add a new PCD for platform DMA coherency, and test that on
> > > WriteBackDataCacheRange's ASSERT (if (!Coherent) ASSERT() else
> > > return;)
> > > 2) Add a more abstracting API that doesn't necessarily map to
> > > WriteBackDataCache when all we wanted was to assert DMA coherency
> > >
> > > but, alas, I've seen a lot less funky platforms than many of you, and
> > > DMA/cache-coherency is not really my thing, so I'll defer to others..
> > >
> > My preference is just remove the assertion and add the debug verbose
> > message instead of changing drivers/introduce new interfaces. It is a
> nop in
> > linux as well if CMO is not present.
> >
> > Thanks,
> > Sunil
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113652): https://edk2.groups.io/g/devel/message/113652
Mute This Topic: https://groups.io/mt/103150435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v6 25/36] OvmfPkg/LoongArchVirt: Add stable timer driver

[maobibo]

On 2024/1/5 下午5:45, Chao Li wrote:

Add a CPU timer driver named StableTimerDxe, which proviedes
EFI_TIMER_ARCH_PROTOCOL for LoongArch.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4584

Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Gerd Hoffmann 
Cc: Bibo Mao 
Cc: Dongyan Qian 
Signed-off-by: Chao Li 
Co-authored-by: Baoqi Zhang 
---
  .../Drivers/StableTimerDxe/Timer.c| 381 ++
  .../Drivers/StableTimerDxe/Timer.h| 127 ++
  .../Drivers/StableTimerDxe/TimerDxe.inf   |  41 ++
  3 files changed, 549 insertions(+)
  create mode 100644 OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/Timer.c
  create mode 100644 OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/Timer.h
  create mode 100644 OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/TimerDxe.inf

diff --git a/OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/Timer.c 
b/OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/Timer.c
new file mode 100644
index 00..0e0f10970a
--- /dev/null
+++ b/OvmfPkg/LoongArchVirt/Drivers/StableTimerDxe/Timer.c
@@ -0,0 +1,381 @@
+/** @file
+  Timer Architectural Protocol as defined in the DXE CIS
+
+  Copyright (c) 2024 Loongson Technology Corporation Limited. All rights 
reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "Timer.h"
+
+//
+// The handle onto which the Timer Architectural Protocol will be installed
+//
+EFI_HANDLE  mTimerHandle = NULL;
+EFI_EVENT   EfiExitBootServicesEvent = (EFI_EVENT)NULL;
+
+//
+// The Timer Architectural Protocol that this driver produces
+//
+EFI_TIMER_ARCH_PROTOCOL  mTimer = {
+  TimerDriverRegisterHandler,
+  TimerDriverSetTimerPeriod,
+  TimerDriverGetTimerPeriod,
+  TimerDriverGenerateSoftInterrupt
+};
+
+//
+// Pointer to the CPU Architectural Protocol instance
+//
+EFI_CPU_ARCH_PROTOCOL  *mCpu;
+
+//
+// The notification function to call on every timer interrupt.
+// A bug in the compiler prevents us from initializing this here.
+//
+EFI_TIMER_NOTIFY  mTimerNotifyFunction;
+
+/**
+  Sets the counter value for timer.
+
+  @param CountThe 16-bit counter value to program into stable timer.
+
+  @retval VOID
+**/
+VOID
+SetPitCount (
+  IN UINT64  Count
+  )
+{
+  if (Count <= 4) {
+return;
+  }
+
+  Count &= LOONGARCH_CSR_TMCFG_TIMEVAL;
+  Count |= LOONGARCH_CSR_TMCFG_EN | LOONGARCH_CSR_TMCFG_PERIOD;
+  CsrWrite (LOONGARCH_CSR_TMCFG, Count);
+}
+
+/**
+  Timer Interrupt Handler.
+
+  @param InterruptTypeThe type of interrupt that occurred
+  @param SystemContextA pointer to the system context when the interrupt 
occurred
+
+  @retval VOID
+**/
+VOID
+EFIAPI
+TimerInterruptHandler (
+  IN EFI_EXCEPTION_TYPE  InterruptType,
+  IN EFI_SYSTEM_CONTEXT  SystemContext
+  )
+{
+  EFI_TPL  OriginalTPL;
+
+  OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+
+  //
+  // Clear interrupt.
+  //
+  CsrWrite (LOONGARCH_CSR_TINTCLR, 0x1);
+
+  if (mTimerNotifyFunction != NULL) {
+//
+// @bug : This does not handle missed timer interrupts
+//
+mTimerNotifyFunction (mTimerPeriod);
+  }
+
+  gBS->RestoreTPL (OriginalTPL);
+}
+
+/**
+  This function registers the handler NotifyFunction so it is called every time
+  the timer interrupt fires.  It also passes the amount of time since the last
+  handler call to the NotifyFunction.  If NotifyFunction is NULL, then the
+  handler is unregistered.  If the handler is registered, then EFI_SUCCESS is
+  returned.  If the CPU does not support registering a timer interrupt handler,
+  then EFI_UNSUPPORTED is returned.  If an attempt is made to register a 
handler
+  when a handler is already registered, then EFI_ALREADY_STARTED is returned.
+  If an attempt is made to unregister a handler when a handler is not 
registered,
+  then EFI_INVALID_PARAMETER is returned.  If an error occurs attempting to
+  register the NotifyFunction with the timer interrupt, then EFI_DEVICE_ERROR
+  is returned.
+
+  @param This The EFI_TIMER_ARCH_PROTOCOL instance.
+  @param NotifyFunction   The function to call when a timer interrupt fires.  
This
+  function executes at TPL_HIGH_LEVEL.  The DXE Core 
will
+  register a handler for the timer interrupt, so it 
can know
+  how much time has passed.  This information is used 
to
+  signal timer based events.  NULL will unregister the 
handler.
+
+  @retvalEFI_SUCCESSThe timer handler was registered.
+  @retvalEFI_UNSUPPORTEDThe platform does not support timer 
interrupts.
+  @retvalEFI_ALREADY_STARTEDNotifyFunction is not NULL, and a 
handler is already
+registered.
+  @retvalEFI_INVALID_PARAMETER  NotifyFunction is NULL, and a handler 
was not
+previously registered.
+  @retvalEFI_DEVICE_ERROR   

[edk2-devel] [edk2-redfish-client][PATCH 2/3] RedfishClientPkg/RedfishFeatureUtilityLib: Add two helper functions

[Chang, Abner via groups.io]

From: Abner Chang 

- Add RedfishRemoveUnchangeableProperties () to remove
  unchangeable Redfish properties such as @OData.id.
- Add DestoryRedfishCharArray () to free Redfish string
  array.

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
---
 .../RedfishFeatureUtilityLib.inf  |  1 +
 .../Library/RedfishFeatureUtilityLib.h| 35 
 .../RedfishFeatureUtilityLib.c| 79 +++
 3 files changed, 115 insertions(+)

diff --git 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.inf
 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.inf
index 63330c8e9d..d8f3da3732 100644
--- 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.inf
+++ 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.inf
@@ -37,6 +37,7 @@
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
+  ConverterCommonLib
   DebugLib
   MemoryAllocationLib
   RedfishLib
diff --git a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h 
b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
index 0f8aede5c4..1b6d3f4cf8 100644
--- a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
+++ b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
@@ -821,6 +821,23 @@ AddRedfishCharArray (
   IN  UINTN ArraySize
   );
 
+/**
+
+  Destroy Redfish string array
+
+  @param[in]Head  The head of string array.
+  @param[in]ArraySize The size of StringArray.
+
+  @retval EFI_SUCCESS   String array is destroyed successfully.
+  @retval OthersError happens
+
+**/
+EFI_STATUS
+DestoryRedfishCharArray (
+  IN  RedfishCS_char_Array  *Head,
+  IN  UINTN ArraySize
+  );
+
 /**
 
   Create numeric array and append to array node in Redfish JSON convert format.
@@ -1024,4 +1041,22 @@ ValidateRedfishStringArrayValues (
   OUT BOOLEAN  *ValueChanged
   );
 
+/**
+  This function removes the unchangeable Redfish properties from input 
JsonString.
+  New JSON string is returned in JsonString and the memory of original pointer 
to input
+  JsonString was freed. Caller is responsible to free the memory pointed by 
output
+  JsonString.
+
+  @param[in,out]  JsonString  On input, this is the pointer to original JSON 
string.
+  On output, this is the new pointer to the 
updated JSON string.
+
+  @retval  EFI_SUCCESS  The unchangeable Redfish properties were removed from 
original JSON string.
+  @retval  Others   There are problems to remove unchangeable Redfish 
properties.
+
+**/
+EFI_STATUS
+RedfishRemoveUnchangeableProperties (
+  IN OUT  CHAR8  **JsonString
+  );
+
 #endif
diff --git 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
index aa723264e8..b16a811bb1 100644
--- 
a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
+++ 
b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
@@ -3412,6 +3412,40 @@ AddRedfishCharArray (
   return EFI_SUCCESS;
 }
 
+/**
+
+  Destroy Redfish string array
+
+  @param[in]Head  The head of string array.
+  @param[in]ArraySize The size of StringArray.
+
+  @retval EFI_SUCCESS   String array is destroyed successfully.
+  @retval OthersError happens
+
+**/
+EFI_STATUS
+DestoryRedfishCharArray (
+  IN  RedfishCS_char_Array  *Head,
+  IN  UINTN ArraySize
+  )
+{
+  UINTN Index;
+  RedfishCS_char_Array  *NextPointer;
+
+  if ((Head == NULL) || (ArraySize == 0)) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  for (Index = 0; Index < ArraySize; Index++) {
+NextPointer = Head->Next;
+if (Head != NULL) {
+  FreePool (Head);
+}
+Head = NextPointer;
+  }
+  return EFI_SUCCESS;
+}
+
 /**
 
   Create numeric array and append to array node in Redfish JSON convert format.
@@ -3935,6 +3969,51 @@ ValidateRedfishStringArrayValues (
   return EFI_SUCCESS;
 }
 
+/**
+  This function removes the unchangeable Redfish properties from input 
JsonString.
+  New JSON string is returned in JsonString and the memory of original pointer 
to input
+  JsonString was freed. Caller is responsible to free the memory pointed by 
output
+  JsonString.
+
+  @param[in,out]  JsonString  On input, this is the pointer to original JSON 
string.
+  On output, this is the new pointer to the 
updated JSON string.
+
+  @retval  EFI_SUCCESS  The unchangeable Redfish properties were removed from 
original JSON string.
+  @retval  Others   There are problems to remove unchangeable Redfish 
properties.
+
+**/
+EFI_STATUS
+RedfishRemoveUnchangeableProperties (
+  IN OUT  CHAR8  **JsonString
+  )
+{
+  RedfishCS_status  Status;
+  CHAR8 *UpdatedJsonString;
+
+  if 

[edk2-devel] [edk2-redfish-client][PATCH 3/3] RedfishClientPkg/ConverterLib: Function to remove Redfish unchangeable properties

[Chang, Abner via groups.io]

From: Abner Chang 

Update RedfishCsCommon.c to add a function to remove Redfish
unchangeable properties.

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
---
 .../ConverterLib/include/RedfishCsCommon.h| 20 +++
 .../ConverterLib/src/RedfishCsCommon.c| 55 +++
 2 files changed, 75 insertions(+)

diff --git a/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h 
b/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h
index e454ab0b73..f5278015aa 100644
--- a/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h
+++ b/RedfishClientPkg/ConverterLib/include/RedfishCsCommon.h
@@ -104,6 +104,26 @@ DestoryCsMemory (
   RedfishCS_void  *rootCs
   );
 
+/**
+  This function removes the unchangeable Redfish properties from JSON raw text
+  The content in JsonString is left unmodified, the caller has to give enoungh
+  memory pointed by NewJsonBuffer in the size of BufferSize.
+
+  JsonString Input JSON raw string
+  NewJsonBuffer  Pointer to memory for the updated JSON raw string in
+ size of BuufferSize.
+  BuufferSizeThe buffer size of NewJsonBuffer
+
+  Return RedfishCS_status.
+
+**/
+RedfishCS_status
+RemoveUnchangeableProperties (
+   RedfishCS_char   *JsonString,
+   RedfishCS_char   *NewJsonBuffer,
+   RedfishCS_uint32  BuufferSize
+   );
+
 typedef struct _RedfishCS_char_Array  RedfishCS_char_Array;
 typedef struct _RedfishCS_int64_Array RedfishCS_int64_Array;
 typedef struct _RedfishCS_bool_Array  RedfishCS_bool_Array;
diff --git a/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c 
b/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c
index fd31e5296b..c6996d7d5d 100644
--- a/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c
+++ b/RedfishClientPkg/ConverterLib/src/RedfishCsCommon.c
@@ -1461,3 +1461,58 @@ CsEmptyPropLinkToJson (
 
   return RedfishCS_status_success;
 }
+
+/**
+  This function removes the unchangeable Redfish properties from JSON raw text
+  The content in JsonString is left unmodified, the caller has to give enoungh
+  memory pointed by NewJsonBuffer in the size of BufferSize.
+
+  JsonString Input JSON raw string
+  NewJsonBuffer  Pointer to memory for the updated JSON raw string in
+ size of BuufferSize.
+  BuufferSizeThe buffer size of NewJsonBuffer
+
+  Return RedfishCS_status.
+
+**/
+RedfishCS_status
+RemoveUnchangeableProperties (
+   RedfishCS_char   *JsonString,
+   RedfishCS_char   *NewJsonBuffer,
+   RedfishCS_uint32  BuufferSize
+   )
+{
+  json_t*JsonObj;
+  RedfishCS_char*TempChar;
+  RedfishCS_status  Status;
+
+  if ((JsonString == NULL) || (NewJsonBuffer == NULL)) {
+return RedfishCS_status_invalid_parameter;
+  }
+
+  JsonObj = json_loads (JsonString, 0, NULL);
+  if (JsonObj == NULL) {
+return RedfishCS_status_unknown_error;
+  }
+
+  json_object_del (JsonObj, "@odata.type");
+  json_object_del (JsonObj, "@odata.id");
+  json_object_del (JsonObj, "Id");
+  json_object_del (JsonObj, "Name");
+
+  TempChar = json_dumps ((json_t *)JsonObj, JSON_INDENT (2));
+  if (TempChar != NULL) {
+if ((strlen (TempChar) + 1) > BuufferSize) {
+  Status = RedfishCS_status_insufficient_memory;
+} else {
+  memcpy (NewJsonBuffer, TempChar, strlen (TempChar) + 1);
+  free (TempChar);
+  Status = RedfishCS_status_success;
+}
+  } else {
+Status = RedfishCS_status_unknown_error;
+  }
+  json_decref(JsonObj);
+  return Status;
+}
+
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113649): https://edk2.groups.io/g/devel/message/113649
Mute This Topic: https://groups.io/mt/103676920/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-redfish-client][PATCH 1/3] RedfishClientPkg/jansson: Define json_object_del

[Chang, Abner via groups.io]

From: Abner Chang 

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
---
 RedfishClientPkg/PrivateInclude/jansson.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/RedfishClientPkg/PrivateInclude/jansson.h 
b/RedfishClientPkg/PrivateInclude/jansson.h
index 900c7e39c6..6b6391428f 100644
--- a/RedfishClientPkg/PrivateInclude/jansson.h
+++ b/RedfishClientPkg/PrivateInclude/jansson.h
@@ -44,6 +44,7 @@ typedef EDKII_JSON_TYPE   json_type;
 #define json_array_append_new(json_t_array, json_t)  
JsonArrayAppendValue(json_t_array, json_t)
 #define json_object_set_new(json_t, key, value)  
JsonObjectSetValue(json_t, key, value)
 #define json_decref(json_t)  
JsonDecreaseReference(json_t)
+#define json_object_del(json_t, key) JsonObjectDelete(json_t, 
key)
 #define json_integer_value(json_t)   
JsonValueGetInteger(json_t)
 #define json_is_object(json_t)   JsonValueIsObject(json_t)
 #define json_is_array(json_t)JsonValueIsArray(json_t)
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113647): https://edk2.groups.io/g/devel/message/113647
Mute This Topic: https://groups.io/mt/103676918/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [RESEND PATCH] RedfishPkg/JsonLib: Add JSON delete object function

[Chang, Abner via groups.io]

From: Abner Chang 

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
---
 RedfishPkg/Include/Library/JsonLib.h | 17 +
 RedfishPkg/Library/JsonLib/JsonLib.c | 24 
 2 files changed, 41 insertions(+)

diff --git a/RedfishPkg/Include/Library/JsonLib.h 
b/RedfishPkg/Include/Library/JsonLib.h
index 8f31d934148..ea252291a0e 100644
--- a/RedfishPkg/Include/Library/JsonLib.h
+++ b/RedfishPkg/Include/Library/JsonLib.h
@@ -656,6 +656,23 @@ JsonObjectSetValue (
   INEDKII_JSON_VALUE   Json
   );
 
+/**
+  The function is used to delete a JSON key from the given JSON bject,
+
+  @param[in]   JsonObjThe provided JSON object.
+  @param[in]   KeyThe key of the JSON value to be deleted.
+
+  @retval  EFI_ABORTEDSome error occur and operation aborted.
+  @retval  EFI_SUCCESSThe JSON value has been deleted from 
this JSON object.
+
+**/
+EFI_STATUS
+EFIAPI
+JsonObjectDelete (
+  INEDKII_JSON_OBJECT  JsonObj,
+  INCONST CHAR8*Key
+  );
+
 /**
   The function is used to get the number of elements in a JSON array. Returns 
or 0 if JsonArray
   is NULL or not a JSON array.
diff --git a/RedfishPkg/Library/JsonLib/JsonLib.c 
b/RedfishPkg/Library/JsonLib/JsonLib.c
index 6c3373d205c..b7be4d61766 100644
--- a/RedfishPkg/Library/JsonLib/JsonLib.c
+++ b/RedfishPkg/Library/JsonLib/JsonLib.c
@@ -810,6 +810,30 @@ JsonObjectSetValue (
   }
 }
 
+/**
+  The function is used to delete a JSON key from the given JSON bject
+
+  @param[in]   JsonObjThe provided JSON object.
+  @param[in]   KeyThe key of the JSON value to be deleted.
+
+  @retval  EFI_ABORTEDSome error occur and operation aborted.
+  @retval  EFI_SUCCESSThe JSON value has been deleted from 
this JSON object.
+
+**/
+EFI_STATUS
+EFIAPI
+JsonObjectDelete (
+  INEDKII_JSON_OBJECT  JsonObj,
+  INCONST CHAR8*Key
+  )
+{
+  if (json_object_del ((json_t *)JsonObj, (const char *)Key) != 0) {
+return EFI_ABORTED;
+  } else {
+return EFI_SUCCESS;
+  }
+}
+
 /**
   The function is used to get the number of elements in a JSON array. Returns 
or 0 if JsonArray
   is NULL or not a JSON array.
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113646): https://edk2.groups.io/g/devel/message/113646
Mute This Topic: https://groups.io/mt/103676885/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH] RedfishPkg/JsonLib: Add JSON delete object function

[Chang, Abner via groups.io]

From: Abner Chang 

Add JSON delete object function to remove JSON object
from the given JSON paylaod.

Signed-off-by: Abner Chang 
Cc: Nickle Wang 
Cc: Igor Kulchytskyy 
---
 RedfishPkg/Include/Library/JsonLib.h | 17 +
 RedfishPkg/Library/JsonLib/JsonLib.c | 24 
 2 files changed, 41 insertions(+)

diff --git a/RedfishPkg/Include/Library/JsonLib.h 
b/RedfishPkg/Include/Library/JsonLib.h
index 8f31d934148..ea252291a0e 100644
--- a/RedfishPkg/Include/Library/JsonLib.h
+++ b/RedfishPkg/Include/Library/JsonLib.h
@@ -656,6 +656,23 @@ JsonObjectSetValue (
   INEDKII_JSON_VALUE   Json
   );
 
+/**
+  The function is used to delete a JSON key from the given JSON bject,
+
+  @param[in]   JsonObjThe provided JSON object.
+  @param[in]   KeyThe key of the JSON value to be deleted.
+
+  @retval  EFI_ABORTEDSome error occur and operation aborted.
+  @retval  EFI_SUCCESSThe JSON value has been deleted from 
this JSON object.
+
+**/
+EFI_STATUS
+EFIAPI
+JsonObjectDelete (
+  INEDKII_JSON_OBJECT  JsonObj,
+  INCONST CHAR8*Key
+  );
+
 /**
   The function is used to get the number of elements in a JSON array. Returns 
or 0 if JsonArray
   is NULL or not a JSON array.
diff --git a/RedfishPkg/Library/JsonLib/JsonLib.c 
b/RedfishPkg/Library/JsonLib/JsonLib.c
index 6c3373d205c..b7be4d61766 100644
--- a/RedfishPkg/Library/JsonLib/JsonLib.c
+++ b/RedfishPkg/Library/JsonLib/JsonLib.c
@@ -810,6 +810,30 @@ JsonObjectSetValue (
   }
 }
 
+/**
+  The function is used to delete a JSON key from the given JSON bject
+
+  @param[in]   JsonObjThe provided JSON object.
+  @param[in]   KeyThe key of the JSON value to be deleted.
+
+  @retval  EFI_ABORTEDSome error occur and operation aborted.
+  @retval  EFI_SUCCESSThe JSON value has been deleted from 
this JSON object.
+
+**/
+EFI_STATUS
+EFIAPI
+JsonObjectDelete (
+  INEDKII_JSON_OBJECT  JsonObj,
+  INCONST CHAR8*Key
+  )
+{
+  if (json_object_del ((json_t *)JsonObj, (const char *)Key) != 0) {
+return EFI_ABORTED;
+  } else {
+return EFI_SUCCESS;
+  }
+}
+
 /**
   The function is used to get the number of elements in a JSON array. Returns 
or 0 if JsonArray
   is NULL or not a JSON array.
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113645): https://edk2.groups.io/g/devel/message/113645
Mute This Topic: https://groups.io/mt/103676803/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Andrew Fish via groups.io]

Sorry need some more time to digest this…. First thoughts.

1) The actual performance issue we hit was the explosion of 
CoreValidateHandle() calls as the number of protocols got large for some diags. 
The newer handles tended to be at the end of the list if I remember correctly. 
  a) It looks like CoreValidateHandle() is the only place  gHandleList was 
walked, as the handle info is crossed referenced in the protocol database. So 
that is why we changed that.
2) If the issue at hand is MM why not just drop the optimizations?
  b) If we have so many MM protocols and handles that seems like its own 
problem? 
3) The issue is patching the grammar in place, why can’t we just make a copy 
for the dispatcher grammer, and operate on the copy. Maybe via a copy on 1st 
update strategy? 

Thanks,

Andrew Fish

> On Jan 10, 2024, at 5:45 AM, Laszlo Ersek  wrote:
> 
> (+ Andrew)
> 
> On 1/10/24 14:09, Laszlo Ersek wrote:
> 
>> I think that keeping the depex section read-only is valuable, so I'd
>> rule out #2. I'd also not start with option #1 -- copying the depex to
>> heap memory, just so this optimization can succeed. I'd simply start by
>> removing the optimization, and measuring how much driver dispatch slows
>> down in practice, on various platforms.
>> 
>> Can you try this? (I have only build-tested and "uncrustified" it.)
>> 
>> The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus it
>> CONST-ifies the Iterator pointer (which points into the DEPEX section),
>> so that the compiler catch any possible accesses at *build time* that
>> would write to the write-protected DEPEX memory area.
> 
> On a tangent: the optimization in question highlights a more general
> problem, namely that the DXE (and possibly MM/SMM) protocol databases
> are considered slow, for some access patterns.
> 
> Edk2 implements those protocol databases with linked lists, where lookup
> costs O(n) operations (average and worst cases). And protocol lookups
> are quite frequent (for example, in depex evaluations, they could be
> considered "particularly frequent").
> 
> (1) The "Tasks" wiki page mentions something *similar* (but not the
> same); see
> 
> https://github.com/tianocore/tianocore.github.io/wiki/Tasks#datahub--gcd-scalability
> 
> The description is: "The DXE core's DataHub and GCD (Global Coherency
> Domain) layers don't scale well as the number of data items gets large,
> since they are based on simple linked lists. Find better data structures."
> 
> The same might apply more or less to the protocol database implementation.
> 
> (2) More to the point, Andrew Fish reported earlier that at Apple, they
> had rewritten the DXE protocol database, using the red-black tree
> OrderedCollectionLib that I had contributed previously to edk2 -- and
> they saw significant performance improvements.
> 
> So upstreaming that feature to edk2 could be very valuable. (Red-black
> trees have O(log(n)) time cost (worst case) for lookup, insertion and
> deletion, and O(n) cost for iterating through the whole data structure.)
> 
> Let me see if I can find the bugzilla ticket...
> 
> Ah, I got it. Apologies, I misremembered: Andrew's comment was not about
> the protocol database, but about the handle database. Here it is:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=988#c7
> 
> (the BZ is still CONFIRMED btw...)
> 
> Still, I think it must be related in a way. Namely, an EFI handle exists
> if and only if at least one protocol interface is installed on it. If
> you uninstall the last protocol interface from a handle, then the handle
> is destroyed -- in fact that's the *only* way to destroy a handle, to my
> understanding. See EFI_BOOT_SERVICES.UninstallProtocolInterface() in the
> UEFI spec: "If the last protocol interface is removed from a handle, the
> handle is freed and is no longer valid". Furthermore, calling
> InstallProtocolInterface() and InstallMultipleProtocolInterfaces() is
> how one *creates* new handles.
> 
> So given how handles and protocol interfaces are conceptually
> interlinked, an rbtree-based protocol DB might have to maintain multiple
> rbtrees internally (for the ability to search the database quickly with
> different types of "keys"). I don't have a design ready in my mind at
> all (I'm not that familiar with the *current*, list-based implementation
> to begin with!). Upstreaming Apple's (experimental?) code could prove
> very helpful.
> 
> Laszlo
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113644): https://edk2.groups.io/g/devel/message/113644
Mute This Topic: https://groups.io/mt/103594587/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Liming Gao 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c b/MdeModulePkg/Core/Pei/Hob/Hob.c
index c4882a23cd..985da50995 100644
--- a/MdeModulePkg/Core/Pei/Hob/Hob.c
+++ b/MdeModulePkg/Core/Pei/Hob/Hob.c
@@ -85,7 +85,7 @@ PeiCreateHob (
   //
   // Check Length to avoid data overflow.
   //
-  if (0x1 - Length <= 0x7) {
+  if (MAX_UINT16 - Length < 0x7) {
 return EFI_INVALID_PARAMETER;
   }
 
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113643): https://edk2.groups.io/g/devel/message/113643
Mute This Topic: https://groups.io/mt/103675965/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/4] EmbeddedPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Leif Lindholm 
Reviewed-by: Ard Biesheuvel 
Cc: Abner Chang 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 EmbeddedPkg/Library/PrePiHobLib/Hob.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/EmbeddedPkg/Library/PrePiHobLib/Hob.c 
b/EmbeddedPkg/Library/PrePiHobLib/Hob.c
index 8eb175aa96..cbc35152cc 100644
--- a/EmbeddedPkg/Library/PrePiHobLib/Hob.c
+++ b/EmbeddedPkg/Library/PrePiHobLib/Hob.c
@@ -110,6 +110,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
@@ -160,6 +167,9 @@ BuildResourceDescriptorHob (
 
   Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof 
(EFI_HOB_RESOURCE_DESCRIPTOR));
   ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->ResourceType  = ResourceType;
   Hob->ResourceAttribute = ResourceAttribute;
@@ -401,6 +411,10 @@ BuildModuleHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_MODULE));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   CopyGuid (&(Hob->MemoryAllocationHeader.Name), 
);
   Hob->MemoryAllocationHeader.MemoryBaseAddress = MemoryAllocationModule;
@@ -449,6 +463,11 @@ BuildGuidHob (
   ASSERT (DataLength <= (0x - sizeof (EFI_HOB_GUID_TYPE)));
 
   Hob = CreateHob (EFI_HOB_TYPE_GUID_EXTENSION, (UINT16)(sizeof 
(EFI_HOB_GUID_TYPE) + DataLength));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return NULL;
+  }
+
   CopyGuid (>Name, Guid);
   return Hob + 1;
 }
@@ -512,6 +531,10 @@ BuildFvHob (
   EFI_HOB_FIRMWARE_VOLUME  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV, sizeof (EFI_HOB_FIRMWARE_VOLUME));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -543,6 +566,10 @@ BuildFv2Hob (
   EFI_HOB_FIRMWARE_VOLUME2  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV2, sizeof (EFI_HOB_FIRMWARE_VOLUME2));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -584,6 +611,10 @@ BuildFv3Hob (
   EFI_HOB_FIRMWARE_VOLUME3  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV3, sizeof (EFI_HOB_FIRMWARE_VOLUME3));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress  = BaseAddress;
   Hob->Length   = Length;
@@ -639,6 +670,10 @@ BuildCpuHob (
   EFI_HOB_CPU  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_CPU, sizeof (EFI_HOB_CPU));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->SizeOfMemorySpace = SizeOfMemorySpace;
   Hob->SizeOfIoSpace = SizeOfIoSpace;
@@ -676,6 +711,10 @@ BuildStackHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_STACK));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   CopyGuid (&(Hob->AllocDescriptor.Name), );
   Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
@@ -756,6 +795,10 @@ BuildMemoryAllocationHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   ZeroMem (&(Hob->AllocDescriptor.Name), sizeof (EFI_GUID));
   Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113642): https://edk2.groups.io/g/devel/message/113642
Mute This Topic: https://groups.io/mt/103675964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Reviewed-by: Ard Biesheuvel 
Cc: Sami Mujawar 
Cc: Ray Ni 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 .../Arm/StandaloneMmCoreHobLib.c  | 35 +++
 1 file changed, 35 insertions(+)

diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..59473e28fe 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
@@ -34,6 +34,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
@@ -89,6 +96,10 @@ BuildModuleHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_MODULE));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   CopyGuid (&(Hob->MemoryAllocationHeader.Name), 
);
   Hob->MemoryAllocationHeader.MemoryBaseAddress = MemoryAllocationModule;
@@ -129,6 +140,9 @@ BuildResourceDescriptorHob (
 
   Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof 
(EFI_HOB_RESOURCE_DESCRIPTOR));
   ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->ResourceType  = ResourceType;
   Hob->ResourceAttribute = ResourceAttribute;
@@ -167,6 +181,11 @@ BuildGuidHob (
   ASSERT (DataLength <= (0x - sizeof (EFI_HOB_GUID_TYPE)));
 
   Hob = CreateHob (EFI_HOB_TYPE_GUID_EXTENSION, (UINT16)(sizeof 
(EFI_HOB_GUID_TYPE) + DataLength));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return NULL;
+  }
+
   CopyGuid (>Name, Guid);
   return Hob + 1;
 }
@@ -226,6 +245,10 @@ BuildFvHob (
   EFI_HOB_FIRMWARE_VOLUME  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV, sizeof (EFI_HOB_FIRMWARE_VOLUME));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -255,6 +278,10 @@ BuildFv2Hob (
   EFI_HOB_FIRMWARE_VOLUME2  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV2, sizeof (EFI_HOB_FIRMWARE_VOLUME2));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -282,6 +309,10 @@ BuildCpuHob (
   EFI_HOB_CPU  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_CPU, sizeof (EFI_HOB_CPU));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->SizeOfMemorySpace = SizeOfMemorySpace;
   Hob->SizeOfIoSpace = SizeOfIoSpace;
@@ -319,6 +350,10 @@ BuildMemoryAllocationHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   ZeroMem (&(Hob->AllocDescriptor.Name), sizeof (EFI_GUID));
   Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113641): https://edk2.groups.io/g/devel/message/113641
Mute This Topic: https://groups.io/mt/103675962/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/4] UefiPayloadPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Reviewed-by: Gua Guo 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 .../Library/PayloadEntryHobLib/Hob.c  | 43 +++
 .../FitUniversalPayloadEntry.c|  8 ++--
 .../UefiPayloadEntry/UniversalPayloadEntry.c  |  8 ++--
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c 
b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
index 2c3acbbc19..51c2e28d7d 100644
--- a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
+++ b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
@@ -110,6 +110,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
@@ -160,6 +167,9 @@ BuildResourceDescriptorHob (
 
   Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof 
(EFI_HOB_RESOURCE_DESCRIPTOR));
   ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->ResourceType  = ResourceType;
   Hob->ResourceAttribute = ResourceAttribute;
@@ -330,6 +340,10 @@ BuildModuleHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_MODULE));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   CopyGuid (&(Hob->MemoryAllocationHeader.Name), 
);
   Hob->MemoryAllocationHeader.MemoryBaseAddress = MemoryAllocationModule;
@@ -378,6 +392,11 @@ BuildGuidHob (
   ASSERT (DataLength <= (0x - sizeof (EFI_HOB_GUID_TYPE)));
 
   Hob = CreateHob (EFI_HOB_TYPE_GUID_EXTENSION, (UINT16)(sizeof 
(EFI_HOB_GUID_TYPE) + DataLength));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return NULL;
+  }
+
   CopyGuid (>Name, Guid);
   return Hob + 1;
 }
@@ -441,6 +460,10 @@ BuildFvHob (
   EFI_HOB_FIRMWARE_VOLUME  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV, sizeof (EFI_HOB_FIRMWARE_VOLUME));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -472,6 +495,10 @@ BuildFv2Hob (
   EFI_HOB_FIRMWARE_VOLUME2  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV2, sizeof (EFI_HOB_FIRMWARE_VOLUME2));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress = BaseAddress;
   Hob->Length  = Length;
@@ -513,6 +540,10 @@ BuildFv3Hob (
   EFI_HOB_FIRMWARE_VOLUME3  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_FV3, sizeof (EFI_HOB_FIRMWARE_VOLUME3));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->BaseAddress  = BaseAddress;
   Hob->Length   = Length;
@@ -546,6 +577,10 @@ BuildCpuHob (
   EFI_HOB_CPU  *Hob;
 
   Hob = CreateHob (EFI_HOB_TYPE_CPU, sizeof (EFI_HOB_CPU));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   Hob->SizeOfMemorySpace = SizeOfMemorySpace;
   Hob->SizeOfIoSpace = SizeOfIoSpace;
@@ -583,6 +618,10 @@ BuildStackHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION_STACK));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   CopyGuid (&(Hob->AllocDescriptor.Name), );
   Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
@@ -664,6 +703,10 @@ BuildMemoryAllocationHob (
 );
 
   Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof 
(EFI_HOB_MEMORY_ALLOCATION));
+  ASSERT (Hob != NULL);
+  if (Hob == NULL) {
+return;
+  }
 
   ZeroMem (&(Hob->AllocDescriptor.Name), sizeof (EFI_GUID));
   Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
diff --git a/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c 
b/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c
index d2e7df4fbe..eb0b325369 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c
@@ -207,10 +207,12 @@ AddNewHob (
   }
 
   NewHob.Header = CreateHob (Hob->Header->HobType, Hob->Header->HobLength);
-
-  if (NewHob.Header != NULL) {
-CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - 
sizeof (EFI_HOB_GENERIC_HEADER));
+  ASSERT (NewHob.Header != NULL);
+  if (NewHob.Header == NULL) {
+return;
   }
+
+  CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - sizeof 
(EFI_HOB_GENERIC_HEADER));
 }
 
 /**
diff --git 

[edk2-devel] [PATCH v3 0/4] Bz4166: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

PR: https://github.com/tianocore/edk2/pull/5252

V3
1. UefiPayloadPkg/Hob: Integer : Add error handle

2. StandaloneMmPkg/Hob: Integer Overflow in : Add error handle

3. EmbeddedPkg/Hob: Integer Overflow in CreateHob() : Add error handle

V2
1. UefiPayloadPkg/Hob: Integer : Add Reviewed-by and Authored-by

2. StandaloneMmPkg/Hob: Integer Overflow in : Add Reviewed-by and Authored-by

3. EmbeddedPkg/Hob: Integer Overflow in CreateHob() : Add Reviewed-by and 
Authored-by

4. MdeModulePkg/Hob: Integer Overflow in CreateHob() : Add Authored-by

V1

1. UefiPayloadPkg/Hob: Integer

2. StandaloneMmPkg/Hob: Integer Overflow in

3. EmbeddedPkg/Hob: Integer Overflow in CreateHob()

4. MdeModulePkg/Hob: Integer Overflow in CreateHob()

Cc: Ard Biesheuvel 

Cc: Gerd Hoffmann 

Cc: John Mathew 

Cc: Vincent Zimmer 

Cc: Sami Mujawar 

Gua Guo (4):
  UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
  StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
  EmbeddedPkg/Hob: Integer Overflow in CreateHob()
  MdeModulePkg/Hob: Integer Overflow in CreateHob()

 EmbeddedPkg/Library/PrePiHobLib/Hob.c | 43 +++
 MdeModulePkg/Core/Pei/Hob/Hob.c   |  2 +-
 .../Arm/StandaloneMmCoreHobLib.c  | 35 +++
 .../Library/PayloadEntryHobLib/Hob.c  | 43 +++
 .../FitUniversalPayloadEntry.c|  8 ++--
 .../UefiPayloadEntry/UniversalPayloadEntry.c  |  8 ++--
 6 files changed, 132 insertions(+), 7 deletions(-)

--
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113639): https://edk2.groups.io/g/devel/message/113639
Mute This Topic: https://groups.io/mt/103675959/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Pedro Falcato]

On Thu, Jan 11, 2024 at 8:46 AM Laszlo Ersek  wrote:
>
> On 1/10/24 22:50, Pedro Falcato wrote:
> > On Wed, Jan 10, 2024 at 1:45 PM Laszlo Ersek 
> > wrote:
> >>
> >> (+ Andrew)
> >>
> >> On 1/10/24 14:09, Laszlo Ersek wrote:
> >>
> >>> I think that keeping the depex section read-only is valuable, so I'd
> >>> rule out #2. I'd also not start with option #1 -- copying the depex
> >>> to heap memory, just so this optimization can succeed. I'd simply
> >>> start by removing the optimization, and measuring how much driver
> >>> dispatch slows down in practice, on various platforms.
> >>>
> >>> Can you try this? (I have only build-tested and "uncrustified" it.)
> >>>
> >>> The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus
> >>> it CONST-ifies the Iterator pointer (which points into the DEPEX
> >>> section), so that the compiler catch any possible accesses at *build
> >>> time* that would write to the write-protected DEPEX memory area.
> >>
> >> On a tangent: the optimization in question highlights a more general
> >> problem, namely that the DXE (and possibly MM/SMM) protocol databases
> >> are considered slow, for some access patterns.
> >>
> >> Edk2 implements those protocol databases with linked lists, where
> >> lookup costs O(n) operations (average and worst cases). And protocol
> >> lookups are quite frequent (for example, in depex evaluations, they
> >> could be considered "particularly frequent").
> >>
> >> (1) The "Tasks" wiki page mentions something *similar* (but not the
> >> same); see
> >>
> >> https://github.com/tianocore/tianocore.github.io/wiki/Tasks#datahub--gcd-scalability
> >>
> >> The description is: "The DXE core's DataHub and GCD (Global Coherency
> >> Domain) layers don't scale well as the number of data items gets
> >> large, since they are based on simple linked lists. Find better data
> >> structures."
> >
> > How large do they usually get? What's the worst case?
>
> No idea.
>
> >
> >>
> >> The same might apply more or less to the protocol database
> >> implementation.
> >>
> >> (2) More to the point, Andrew Fish reported earlier that at Apple,
> >> they had rewritten the DXE protocol database, using the red-black
> >> tree OrderedCollectionLib that I had contributed previously to edk2
> >> -- and they saw significant performance improvements.
> >>
> >> So upstreaming that feature to edk2 could be very valuable.
> >> (Red-black trees have O(log(n)) time cost (worst case) for lookup,
> >> insertion and deletion, and O(n) cost for iterating through the whole
> >> data structure.)
> >
> > FWIW, can we do better than an RB tree? They're notoriously cache
> > unfriendly...
>
> Sure, if someone contributes a different data structure that is suitable
> for the task :)

Hey, I happen to have written one!
https://github.com/heatd/Onyx/blob/master/kernel/kernel/radix.cpp
It just needs some C'ifying, then some EFI'ing on top, but I'm fairly
confident in its stability.

>
> RB trees may be cache unfriendly, but the functionality they provide
> with O(log(n)) worst case performance is amazing. You can use them as
> read-write associate arrays, sorted lists supporting forward and
> backward traversal (in fact tools for sorting), priority queues, etc.
>
> When I contributed the code, edk2 didn't have any associative array
> type, so something generic that would address the widest range of use
> cases looked like a good idea. (And indeed the library has been well
> applied in several of those use cases since, in various parts of edk2 --
> for sorting, uniqueness-checking, async interface token tracking &
> lookups.)

Definitely, I use it in Ext4Dxe too, it's great! (Although I do have a
small nit in that it allocates nodes itself, and there's no way around
it, but oh well...)

>
> This is not an argument against a more suitable data structure of
> course. Just pointing out that the RB tree has worked well thus far.
> E.g., under the BZ link below, Andrew mentions a diagnostic tool that
> creates 3000 handles. Looking up an element in a 3000-element list would
> cost 1500 iterations on average; using a balanced binary search tree it
> might cost ~11 iterations. Assuming that linked lists and linked binary
> search trees are similarly cache-unfriendly, that's a ~136 factor of
> improvement.

I would guess that binary trees are more cache-and-speculation
unfriendly as you need to decide which branch you're going down. But
yes, even a RB tree would be a great improvement over the linked list.

>
> >
> >>
> >> Let me see if I can find the bugzilla ticket...
> >>
> >> Ah, I got it. Apologies, I misremembered: Andrew's comment was not
> >> about the protocol database, but about the handle database. Here it
> >> is:
> >>
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=988#c7
> >>
> >> (the BZ is still CONFIRMED btw...)
> >>
> >> Still, I think it must be related in a way. Namely, an EFI handle
> >> exists if and only if at least one protocol interface is installed on
> >> it. If you 

Re: [edk2-devel] [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118

[Yao, Jiewen]

Hi Doug
Thanks for the fix.

Please remember to CC all SecurityPkg maintainer and reviewer.

I will merge after several days to see if there is any additional feedback from 
the community.

Thank you
Yao, Jiewen

> -Original Message-
> From: Douglas Flick [MSFT] 
> Sent: Friday, January 12, 2024 2:16 AM
> To: devel@edk2.groups.io
> Cc: Douglas Flick [MSFT] ; Yao, Jiewen
> 
> Subject: [PATCH 0/6] SECURITY PATCHES TCBZ4117 & TCBZ4118
> 
> This patch series include the combined / merged security patches
> (as seperate commits) for TCBZ4117 (CVE-2022-36763) and TCBZ4118
> (CVE-2022-36764) for DxeTpm2MeasureBootLib and DxeTpmMeasureBootLib.
> These patches have already been reviewed by SecurityPkg Maintainer
> (Jiewen) on GHSA.
> 
> This patch series (specifically TCBZ4117) supersedes TCBZ2168.
> 
> Cc: Jiewen Yao 
> 
> Douglas Flick [MSFT] (6):
>   SecurityPkg: DxeTpm2MeasureBootLib: SECURITY PATCH 4117 - CVE
> 2022-36763
>   SecurityPkg: DxeTpmMeasureBootLib: SECURITY PATCH 4117 - CVE
> 2022-36763
>   SecurityPkg: : Adding CVE 2022-36763 to SecurityFixes.yaml
>   SecurityPkg: DxeTpm2MeasureBootLib: SECURITY PATCH 4118 - CVE
> 2022-36764
>   SecurityPkg: DxeTpmMeasureBootLib: SECURITY PATCH 4118 - CVE
> 2022-36764
>   SecurityPkg: : Adding CVE 2022-36764 to SecurityFixes.yaml
> 
>  SecurityPkg/Test/SecurityPkgHostTest.dsc  |   2 +
>  .../DxeTpm2MeasureBootLib.inf |   4 +-
>  ...Tpm2MeasureBootLibSanitizationTestHost.inf |  28 ++
>  .../DxeTpmMeasureBootLib.inf  |   4 +-
>  ...eTpmMeasureBootLibSanitizationTestHost.inf |  28 ++
>  .../DxeTpm2MeasureBootLibSanitization.h   | 139 +++
>  .../DxeTpmMeasureBootLibSanitization.h| 137 +++
>  .../DxeTpm2MeasureBootLib.c   |  87 ++--
>  .../DxeTpm2MeasureBootLibSanitization.c   | 319 +++
>  .../DxeTpm2MeasureBootLibSanitizationTest.c   | 345 
>  .../DxeTpmMeasureBootLib.c|  53 ++-
>  .../DxeTpmMeasureBootLibSanitization.c| 285 +
>  .../DxeTpmMeasureBootLibSanitizationTest.c| 387 ++
>  SecurityPkg/SecurityFixes.yaml|  36 ++
>  SecurityPkg/SecurityPkg.ci.yaml   |   2 +
>  15 files changed, 1801 insertions(+), 55 deletions(-)
>  create mode 100644
> SecurityPkg/Library/DxeTpm2MeasureBootLib/InternalUnitTest/DxeTpm2Measur
> eBootLibSanitizationTestHost.inf
>  create mode 100644
> SecurityPkg/Library/DxeTpmMeasureBootLib/InternalUnitTest/DxeTpmMeasureB
> ootLibSanitizationTestHost.inf
>  create mode 100644
> SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLibSanitiza
> tion.h
>  create mode 100644
> SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLibSanitizatio
> n.h
>  create mode 100644
> SecurityPkg/Library/DxeTpm2MeasureBootLib/DxeTpm2MeasureBootLibSanitiza
> tion.c
>  create mode 100644
> SecurityPkg/Library/DxeTpm2MeasureBootLib/InternalUnitTest/DxeTpm2Measur
> eBootLibSanitizationTest.c
>  create mode 100644
> SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLibSanitizatio
> n.c
>  create mode 100644
> SecurityPkg/Library/DxeTpmMeasureBootLib/InternalUnitTest/DxeTpmMeasureB
> ootLibSanitizationTest.c
>  create mode 100644 SecurityPkg/SecurityFixes.yaml
> 
> --
> 2.43.0


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113637): https://edk2.groups.io/g/devel/message/113637
Mute This Topic: https://groups.io/mt/103675434/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

Hi Gerd,

I explain the reason why 47 here is since virtual addresses are sign-extended 
in the commit message.
About the technical background, I also mentioned in the commit message " When 
5-Level Paging is disabled and the PhysicalAddressBits retrived  from CPU HOB 
or CpuId is bigger than 47". Could you please provide more detailed suggestion 
about the commit message?

Or can we merge the code firstly? Then I'll raise another PR to make the 
comments around the code more detailed as we want.

Thanks,
Dun

-Original Message-
From: Gerd Hoffmann  
Sent: Thursday, January 11, 2024 6:21 PM
To: Tan, Dun 
Cc: devel@edk2.groups.io; Ni, Ray ; Laszlo Ersek 
; Kumar, Rahul R 
Subject: Re: [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

On Thu, Jan 11, 2024 at 04:59:47PM +0800, Dun Tan wrote:
> When creating smm page table, limit maximum supported physical address 
> bits returned by
> CalculateMaximumSupportAddress() to 47 if 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the PhysicalAddressBits retrived 
> from CPU HOB or CpuId is bigger than 47, and since virtual addresses 
> are sign-extended, only [0, 2^47-1] range in 52-bit physical address 
> is mapped in page table.

> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page 
> + table  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);  if (!Is5LevelPagingNeeded && 
> + (PhysicalAddressBits > 47)) {
> +PhysicalAddressBits = 47;
> +  }

The code change is fine but the comment should be more verbose and explain the 
why 47 not 48 is used here.  The discussion on the patch clearly showed that 
the technical background is not obvious ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113636): https://edk2.groups.io/g/devel/message/113636
Mute This Topic: https://groups.io/mt/103658816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update to latest

[Michael D Kinney]

Merged: https://github.com/tianocore/edk2/pull/5256


From: Joey Vagedes via groups.io 
Sent: Thursday, January 11, 2024 3:16 PM
To: Kinney, Michael D ; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update to latest

Thanks Mike,

I've updated the PR / branch with the reviewed-by tag. It is ready to be merged 
at your convenience.

pip-requirements.txt: Update to latest by Javagedes · Pull Request #5256 · 
tianocore/edk2 (github.com)

Thanks,
Joey


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113635): https://edk2.groups.io/g/devel/message/113635
Mute This Topic: https://groups.io/mt/103671350/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update to latest

[Joey Vagedes via groups.io]

Thanks Mike,

I've updated the PR / branch with the reviewed-by tag. It is ready to be merged 
at your convenience.

pip-requirements.txt: Update to latest by Javagedes · Pull Request #5256 · 
tianocore/edk2 (github.com) ( https://github.com/tianocore/edk2/pull/5256 )

Thanks,
Joey


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113634): https://edk2.groups.io/g/devel/message/113634
Mute This Topic: https://groups.io/mt/103671350/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update to latest

[Michael D Kinney]

Reviewed-by: Michael D Kinney 

> -Original Message-
> From: Joey Vagedes 
> Sent: Thursday, January 11, 2024 1:35 PM
> To: devel@edk2.groups.io
> Cc: Andrew Fish ; Leif Lindholm ;
> Kinney, Michael D 
> Subject: [PATCH v1 1/1] pip-requirements.txt: Update to latest
> 
> From: "Joey Vagedes (from Dev Box)" 
> 
> Updates edk2-pytool-extensions, edk2-pytool-library, and regex to their
> latest respective releases.
> 
> Signed-off-by: Joey Vagedes 
> Cc: Andrew Fish 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> ---
>  pip-requirements.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/pip-requirements.txt b/pip-requirements.txt
> index 8177c60d1808..6420078822b5 100644
> --- a/pip-requirements.txt
> +++ b/pip-requirements.txt
> @@ -12,9 +12,9 @@
>  # https://www.python.org/dev/peps/pep-0440/#version-specifiers
> 
>  ##
> 
> 
> 
> -edk2-pytool-library==0.19.3
> 
> -edk2-pytool-extensions~=0.25.1
> 
> +edk2-pytool-library==0.19.9
> 
> +edk2-pytool-extensions==0.26.4
> 
>  edk2-basetools==0.1.48
> 
>  antlr4-python3-runtime==4.7.1
> 
>  lcov-cobertura==2.0.2
> 
> -regex==2023.8.8
> 
> +regex==2023.12.25
> 
> --
> 2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113633): https://edk2.groups.io/g/devel/message/113633
Mute This Topic: https://groups.io/mt/103671350/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] .pytool/Readme.md: Update matrix for DynamicTablesPkg

[Joey Vagedes via groups.io]

Hello.

Reviewed-by: Joey Vagedes 

This has been merged via PR 5257 ( https://github.com/tianocore/edk2/pull/5257 
) , commit SHA 58355ec1926563efc954821a2851365182a4ebd4

Thanks,
Joey


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113632): https://edk2.groups.io/g/devel/message/113632
Mute This Topic: https://groups.io/mt/103522062/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] pip-requirements.txt: Update to latest

[Joey Vagedes via groups.io]

From: "Joey Vagedes (from Dev Box)" 

Updates edk2-pytool-extensions, edk2-pytool-library, and regex to their
latest respective releases.

Signed-off-by: Joey Vagedes 
Cc: Andrew Fish 
Cc: Leif Lindholm 
Cc: Michael D Kinney 
---
 pip-requirements.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pip-requirements.txt b/pip-requirements.txt
index 8177c60d1808..6420078822b5 100644
--- a/pip-requirements.txt
+++ b/pip-requirements.txt
@@ -12,9 +12,9 @@
 # https://www.python.org/dev/peps/pep-0440/#version-specifiers
 ##
 
-edk2-pytool-library==0.19.3
-edk2-pytool-extensions~=0.25.1
+edk2-pytool-library==0.19.9
+edk2-pytool-extensions==0.26.4
 edk2-basetools==0.1.48
 antlr4-python3-runtime==4.7.1
 lcov-cobertura==2.0.2
-regex==2023.8.8
+regex==2023.12.25
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113631): https://edk2.groups.io/g/devel/message/113631
Mute This Topic: https://groups.io/mt/103671350/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] OvmfPkg: Add CloudHv support to PlatformScanE820 utility function.

[Thomas Barrett]

The PlatformScanE820 utility function is not currently compatible
with CloudHv since it relies on the prescence of the "etc/e820"
QemuFwCfg file. Update the PlatformScanE820 to iterate through the
PVH e820 entries when running on a CloudHv guest.

Cc: Anatol Belski 
Cc: Ard Biesheuvel 
Cc: Gerd Hoffmann 
Cc: Jianyong Wu 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Cc: Rob Bradford 
Signed-off-by: Thomas Barrett 
---
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 95 +---
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c 
b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 662e7e85bb..76a9dc9211 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -248,6 +248,67 @@ PlatformReservationConflictCB (
   PlatformInfoHob->PcdPciMmio64Base = NewBase;

 }

 

+/**

+  Returns PVH memmap

+  @param Entries  Pointer to PVH memmap

+  @param CountNumber of entries

+  @return EFI_STATUS

+**/

+EFI_STATUS

+GetPvhMemmapEntries (

+  struct hvm_memmap_table_entry  **Entries,

+  UINT32 *Count

+  )

+{

+  UINT32 *PVHResetVectorData;

+  struct hvm_start_info  *pvh_start_info;

+

+  PVHResetVectorData = (VOID *)(UINTN)PcdGet32 (PcdXenPvhStartOfDayStructPtr);

+  if (PVHResetVectorData == 0) {

+return EFI_NOT_FOUND;

+  }

+

+  pvh_start_info = (struct hvm_start_info *)(UINTN)PVHResetVectorData[0];

+

+  *Entries = (struct hvm_memmap_table_entry 
*)(UINTN)pvh_start_info->memmap_paddr;

+  *Count   = pvh_start_info->memmap_entries;

+

+  return EFI_SUCCESS;

+}

+

+STATIC

+EFI_STATUS

+PlatformScanE820Pvh (

+  IN  E820_SCAN_CALLBACK Callback,

+  IN OUT  EFI_HOB_PLATFORM_INFO  *PlatformInfoHob

+  )

+{

+  struct hvm_memmap_table_entry  *Memmap;

+  UINT32 MemmapEntriesCount;

+  struct hvm_memmap_table_entry  *Entry;

+  EFI_E820_ENTRY64   E820Entry;

+  EFI_STATUS Status;

+  UINT32 Loop;

+

+  Status = GetPvhMemmapEntries (, );

+  if (EFI_ERROR (Status)) {

+return Status;

+  }

+

+  for (Loop = 0; Loop < MemmapEntriesCount; Loop++) {

+Entry = Memmap + Loop;

+

+if (Entry->type == XEN_HVM_MEMMAP_TYPE_RAM) {

+  E820Entry.BaseAddr = Entry->addr;

+  E820Entry.Length   = Entry->size;

+  E820Entry.Type = Entry->type;

+  Callback (, PlatformInfoHob);

+}

+  }

+

+  return EFI_SUCCESS;

+}

+

 /**

   Iterate over the entries in QEMU's fw_cfg E820 RAM map, call the

   passed callback for each entry.

@@ -279,6 +340,10 @@ PlatformScanE820 (
   EFI_E820_ENTRY64  E820Entry;

   UINTN Processed;

 

+  if (PlatformInfoHob->HostBridgeDevId == CLOUDHV_DEVICE_ID) {

+return PlatformScanE820Pvh (Callback, PlatformInfoHob);

+  }

+

   Status = QemuFwCfgFindFile ("etc/e820", , );

   if (EFI_ERROR (Status)) {

 return Status;

@@ -297,36 +362,6 @@ PlatformScanE820 (
   return EFI_SUCCESS;

 }

 

-/**

-  Returns PVH memmap

-

-  @param Entries  Pointer to PVH memmap

-  @param CountNumber of entries

-

-  @return EFI_STATUS

-**/

-EFI_STATUS

-GetPvhMemmapEntries (

-  struct hvm_memmap_table_entry  **Entries,

-  UINT32 *Count

-  )

-{

-  UINT32 *PVHResetVectorData;

-  struct hvm_start_info  *pvh_start_info;

-

-  PVHResetVectorData = (VOID *)(UINTN)PcdGet32 (PcdXenPvhStartOfDayStructPtr);

-  if (PVHResetVectorData == 0) {

-return EFI_NOT_FOUND;

-  }

-

-  pvh_start_info = (struct hvm_start_info *)(UINTN)PVHResetVectorData[0];

-

-  *Entries = (struct hvm_memmap_table_entry 
*)(UINTN)pvh_start_info->memmap_paddr;

-  *Count   = pvh_start_info->memmap_entries;

-

-  return EFI_SUCCESS;

-}

-

 STATIC

 UINT64

 GetHighestSystemMemoryAddressFromPvhMemmap (

-- 
2.34.1

Disclaimer

The information contained in this communication from the sender is 
confidential. It is intended solely for use by the recipient and others 
authorized to receive it. If you are not the recipient, you are hereby notified 
that any disclosure, copying, distribution or taking action in relation of the 
contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been 
automatically archived by Mimecast, a leader in email security and cyber 
resilience. Mimecast integrates email defenses with brand protection, security 
awareness training, web security, compliance and other essential capabilities. 
Mimecast helps protect large and small organizations from malicious activity, 
human error and technology failure; and to lead the movement toward building a 
more resilient world. To find out more, visit our website.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113630): 

[edk2-devel] [PATCH v2 2/2] OvmfPkg: Update PlatformAddressWidthInitialization for CloudHv

[Thomas Barrett]

In addition to initializing the PhysMemAddressWidth and
FirstNonAddress fields in PlatformInfoHob, the
PlatformAddressWidthInitialization function is responsible
for initializing the PcdPciMmio64Base and PcdPciMmio64Size
fields.

Currently, for CloudHv guests, the PcdPciMmio64Base is
placed immediately after either the 4G boundary or the
last RAM region, whichever is greater. We do not change
this behavior.

Previously, when booting CloudHv guests with greater than
1TiB of high memory, the PlatformAddressWidthInitialization
function incorrect calculates the amount of RAM using the
overflowed 24-bit CMOS register.

Now, we update the PlatformAddressWidthInitialization
behavior on CloudHv to scan the E820 entries to detect
the amount of RAM. This allows CloudHv guests to boot with
greater than 1TiB of RAM

Cc: Anatol Belski 
Cc: Ard Biesheuvel 
Cc: Gerd Hoffmann 
Cc: Jianyong Wu 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Cc: Rob Bradford 
Signed-off-by: Thomas Barrett 
---
 OvmfPkg/CloudHv/CloudHvX64.dsc  |  2 ++
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 12 
 2 files changed, 14 insertions(+)

diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index af594959a9..b522fa1059 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -566,6 +566,8 @@
   # Point to the MdeModulePkg/Application/UiApp/UiApp.inf

   gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 
0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }

 

+  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE

+

 


 #

 # Pcd Dynamic Section - list of all EDK II PCD Entries defined by this Platform

diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c 
b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
index 76a9dc9211..f042517bb6 100644
--- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
+++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
@@ -873,6 +873,18 @@ PlatformAddressWidthInitialization (
 

   if (PlatformInfoHob->HostBridgeDevId == 0x /* microvm */) {

 PlatformAddressWidthFromCpuid (PlatformInfoHob, FALSE);

+return;

+  } else if (PlatformInfoHob->HostBridgeDevId == CLOUDHV_DEVICE_ID) {

+PlatformInfoHob->FirstNonAddress = BASE_4GB;

+Status   = PlatformScanE820 
(PlatformGetFirstNonAddressCB, PlatformInfoHob);

+if (EFI_ERROR (Status)) {

+  PlatformInfoHob->FirstNonAddress = BASE_4GB + 
PlatformGetSystemMemorySizeAbove4gb ();

+}

+

+PlatformInfoHob->PcdPciMmio64Base = PlatformInfoHob->FirstNonAddress;

+PlatformAddressWidthFromCpuid (PlatformInfoHob, FALSE);

+PlatformInfoHob->PcdPciMmio64Size = PlatformInfoHob->FirstNonAddress - 
PlatformInfoHob->PcdPciMmio64Base;

+

 return;

   }

 

-- 
2.34.1

Disclaimer

The information contained in this communication from the sender is 
confidential. It is intended solely for use by the recipient and others 
authorized to receive it. If you are not the recipient, you are hereby notified 
that any disclosure, copying, distribution or taking action in relation of the 
contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been 
automatically archived by Mimecast, a leader in email security and cyber 
resilience. Mimecast integrates email defenses with brand protection, security 
awareness training, web security, compliance and other essential capabilities. 
Mimecast helps protect large and small organizations from malicious activity, 
human error and technology failure; and to lead the movement toward building a 
more resilient world. To find out more, visit our website.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113629): https://edk2.groups.io/g/devel/message/113629
Mute This Topic: https://groups.io/mt/103668740/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/2] Support CloudHv guests with >1TB of RAM.

[Thomas Barrett]

This series adds support for cloud-hypervisor guests with >1TiB
of RAM to Ovmf. This bug was solved for Qemu back in 2017
https://bugzilla.redhat.com/show_bug.cgi?id=1468526. The bug is
still occuring for CloudHv guests because the PlatformScanE820
utility is not currently supported for CloudHv. 

My working branch for these changes can be found here:
https://github.com/thomasbarrett/edk2/tree/cloud-hv-1tb-ram

Cc: Anatol Belski 
Cc: Ard Biesheuvel 
Cc: Gerd Hoffmann 
Cc: Jianyong Wu 
Cc: Jiewen Yao 
Cc: Laszlo Ersek 
Cc: Rob Bradford 

Thomas Barrett (2):
  OvmfPkg: Add CloudHv support to PlatformScanE820 utility function.
  OvmfPkg: Update PlatformAddressWidthInitialization for CloudHv

 OvmfPkg/CloudHv/CloudHvX64.dsc  |   2 +
 OvmfPkg/Library/PlatformInitLib/MemDetect.c | 107 ++--
 2 files changed, 79 insertions(+), 30 deletions(-)

-- 
2.34.1

Disclaimer

The information contained in this communication from the sender is 
confidential. It is intended solely for use by the recipient and others 
authorized to receive it. If you are not the recipient, you are hereby notified 
that any disclosure, copying, distribution or taking action in relation of the 
contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been 
automatically archived by Mimecast, a leader in email security and cyber 
resilience. Mimecast integrates email defenses with brand protection, security 
awareness training, web security, compliance and other essential capabilities. 
Mimecast helps protect large and small organizations from malicious activity, 
human error and technology failure; and to lead the movement toward building a 
more resilient world. To find out more, visit our website.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113628): https://edk2.groups.io/g/devel/message/113628
Mute This Topic: https://groups.io/mt/103668720/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/BaseLib: Fix InternalSwitchStack bug for RISCV64.

[yorange]

The problem has been fixed in this patch ( Re: [PATCH v2] MdePkg/BaseLib:Fix 
boot DxeCore hang on riscv platform (groups.io) ( 
https://edk2.groups.io/g/devel/message/113624 ) ), no reply is needed here, 
thank you all.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113627): https://edk2.groups.io/g/devel/message/113627
Mute This Topic: https://groups.io/mt/103494172/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] CloudHv: Add CloudHv support to PlatformScanE820 utility function.

[Thomas Barrett]

Hey Laslo,

Thank you for the link to your git workflow and your patience. I will send
out a ‘PATCH v2’ series that hopefully has all the necessary components by
following the steps in the guide.

Thomas

On Thu, Jan 11, 2024 at 1:39 AM Laszlo Ersek  wrote:

> Hello Thomas,
>
> (+ Jianyong, Anatol, Gerd)
>
> On 1/10/24 23:21, Thomas Barrett wrote:
> > Signed-off-by: Thomas Barrett 
> > ---
> > OvmfPkg/Library/PlatformInitLib/MemDetect.c | 95 ++---
> > 1 file changed, 65 insertions(+), 30 deletions(-)
>
> please don't paste patches in email bodies; they are hard to read
> (review) and effectively impossible to apply that way.
>
> Here's the official dev guide:
>
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
>
> A few more personal ideas:
>
>
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers
> 
>
> Please don't forget to run the GetMaintainer.py script either, for
> composing the Cc: tags in the commit message body.
>
>
> Laszlo
>

Disclaimer

The information contained in this communication from the sender is 
confidential. It is intended solely for use by the recipient and others 
authorized to receive it. If you are not the recipient, you are hereby notified 
that any disclosure, copying, distribution or taking action in relation of the 
contents of this information is strictly prohibited and may be unlawful.

This email has been scanned for viruses and malware, and may have been 
automatically archived by Mimecast, a leader in email security and cyber 
resilience. Mimecast integrates email defenses with brand protection, security 
awareness training, web security, compliance and other essential capabilities. 
Mimecast helps protect large and small organizations from malicious activity, 
human error and technology failure; and to lead the movement toward building a 
more resilient world. To find out more, visit our website.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113626): https://edk2.groups.io/g/devel/message/113626
Mute This Topic: https://groups.io/mt/103657892/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Sami Mujawar]

Hi Gua,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 11/01/2024, 15:19, "Guo, Gua" mailto:gua@intel.com>> wrote:


Maybe I can add error handle but they will have several case need to do if it's 
fine.
It maybe increasing some BIOS size.


Error Handle Error Handle Error Handle Error Handle
A > B -> C --> CreateHob 
> return NULL


All caller chain may need to add it if we really want to prevent it on release.
[SAMI] As I understand, in most cases such an error may not be recoverable. In 
such cases, would it make sense for the ASSERT() implementation to call a 
Panic() in release builds? 
Adding a Panic() API has been discussed before at 
https://edk2.groups.io/g/devel/message/86354.
[/SAMI]

Thanks,
Gua
-Original Message-
From: Sami Mujawar mailto:sami.muja...@arm.com>> 
Sent: Thursday, January 11, 2024 11:02 PM
To: Guo, Gua mailto:gua@intel.com>>; 
devel@edk2.groups.io 
Cc: Marc Beatove mailto:mbeat...@google.com>>; Ard 
Biesheuvel mailto:ardb+tianoc...@kernel.org>>; Ni, 
Ray mailto:ray...@intel.com>>; Mathews, John 
mailto:john.math...@intel.com>>; Gerd Hoffmann 
mailto:kra...@redhat.com>>; nd mailto:n...@arm.com>>
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()


Hi Gua,


Please find my response inline marked [SAMI].


Regards,


Sami Mujawar
On 11/01/2024, 14:19, "Guo, Gua" mailto:gua@intel.com> 
>> wrote:




You mean we need to add below error handle after all callers ?




Hob = CreateHob (...)
ASSERT (Hob != NULL); < Here [SAMI] That would certainly help 
catch issues in the debug builds. But the problem with asserts is, they vanish 
in release builds. 
I think we should consider adding appropriate error handling in the calling 
functions to make sure that they do not result in a crash.
[/SAMI]








Thanks,
Gua
-Original Message-
From: Sami Mujawar mailto:sami.muja...@arm.com> 
>>
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua mailto:gua@intel.com> 
>>; devel@edk2.groups.io 
 >
Cc: Marc Beatove mailto:mbeat...@google.com> 
>>; Ard Biesheuvel 
mailto:ardb+tianoc...@kernel.org> 
>>; Ni, Ray 
mailto:ray...@intel.com> >>; Mathews, John mailto:john.math...@intel.com> >>; Gerd Hoffmann mailto:kra...@redhat.com> >>; nd mailto:n...@arm.com> 
>>
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()




Hi Gua,




Thank you for this patch.
Please see my response inline marked [SAMI].




Regards,




Sami Mujawar




On 11/01/2024, 09:15, "gua@intel.com  
>  >>" mailto:gua@intel.com> 
>   
>   
 
 
 
 
 









Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765








The CreateHob() function aligns the requested size to 8 performing the 
following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); ```








No checks are performed to ensure this value doesn't overflow, and could lead 
to CreateHob() returning a smaller HOB than requested, which could lead to OOB 
HOB accesses.








Reported-by: Marc Beatove mailto:mbeat...@google.com> 
> 
 

Re: [edk2-devel] [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform

[Sunil V L]

On Fri, Jan 05, 2024 at 03:47:07PM +, Andrei Warkentin wrote:
> Looks reasonable to me.
> 
> Reviewed-by: Andrei Warkentin 
> 
> > -Original Message-
> > From: Yang Wang 
> > Sent: Wednesday, December 27, 2023 8:57 PM
> > To: Warkentin, Andrei ; devel@edk2.groups.io
> > Cc: Yang Wang ; Ran Wang ;
> > Bamvor Jian ZHANG ; Gao, Liming
> > ; Kinney, Michael D
> > ; Sunil V L ; Liu,
> > Zhiguang 
> > Subject: [PATCH v2] MdePkg/BaseLib:Fix boot DxeCore hang on riscv platform
> > 
> > For scene of
> > HandOffToDxeCore()->SwitchStack(DxeCoreEntryPoint)->
> > InternalSwitchStack()->LongJump(),Variable HobList.Raw
> > will be passed (from *Context1 to register a0) to
> > DxeMain() in parameter *HobStart.
> > 
> > However, meanwhile the function LongJump() overrides
> > register a0 with a1 (-1)  due to commit (ea628f28e5 "RISCV: Fix
> > InternalLongJump to return correct value"), then cause hang.
> > 
> > Replacing calling LongJump() with new InternalSwitchStackAsm() to pass
> > addres data in register s0 to register a0 could fix this issue (just
> > like the solution in MdePkg/Library/BaseLib/AArch64/SwitchStack.S)
> > 
> > Signed-off-by: Yang Wang 
> > Reviewed-by: Ran Wang 
> > Cc: Bamvor Jian ZHANG 
> > Cc: Andrei Warkentin 
> > Cc: Liming Gao 
> > Cc: Michael D Kinney 
> > Cc: Sunil V L 
> > Cc: Zhiguang Liu 
> > ---
Thanks for the patch!. Merged this as #5255 after fixing a minor
formatting issue.

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113624): https://edk2.groups.io/g/devel/message/113624
Mute This Topic: https://groups.io/mt/103395756/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

Maybe I can add error handle but they will have several case need to do if it's 
fine.
It maybe increasing some BIOS size.

Error Handle Error HandleError Handle   
Error Handle
A > B -> C --> CreateHob 
> return NULL

All caller chain may need to add it if we really want to prevent it on release.

Thanks,
Gua
-Original Message-
From: Sami Mujawar  
Sent: Thursday, January 11, 2024 11:02 PM
To: Guo, Gua ; devel@edk2.groups.io
Cc: Marc Beatove ; Ard Biesheuvel 
; Ni, Ray ; Mathews, John 
; Gerd Hoffmann ; nd 
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

Hi Gua,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar
On 11/01/2024, 14:19, "Guo, Gua" mailto:gua@intel.com>> wrote:


You mean we need to add below error handle after all callers ?


Hob = CreateHob (...)
ASSERT (Hob != NULL); < Here [SAMI] That would certainly help 
catch issues in the debug builds. But the problem with asserts is, they vanish 
in release builds. 
I think we should consider adding appropriate error handling in the calling 
functions to make sure that they do not result in a crash.
[/SAMI]




Thanks,
Gua
-Original Message-
From: Sami Mujawar mailto:sami.muja...@arm.com>>
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua mailto:gua@intel.com>>; 
devel@edk2.groups.io 
Cc: Marc Beatove mailto:mbeat...@google.com>>; Ard 
Biesheuvel mailto:ardb+tianoc...@kernel.org>>; Ni, 
Ray mailto:ray...@intel.com>>; Mathews, John 
mailto:john.math...@intel.com>>; Gerd Hoffmann 
mailto:kra...@redhat.com>>; nd mailto:n...@arm.com>>
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()


Hi Gua,


Thank you for this patch.
Please see my response inline marked [SAMI].


Regards,


Sami Mujawar


On 11/01/2024, 09:15, "gua@intel.com  
>" mailto:gua@intel.com> >> wrote:




From: Gua Guo mailto:gua@intel.com> 
>>




REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 
 
 





Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765




The CreateHob() function aligns the requested size to 8 performing the 
following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); ```




No checks are performed to ensure this value doesn't overflow, and could lead 
to CreateHob() returning a smaller HOB than requested, which could lead to OOB 
HOB accesses.




Reported-by: Marc Beatove mailto:mbeat...@google.com> 
>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org> >>
Cc: Sami Mujawar mailto:sami.muja...@arm.com> 
>>
Cc: Ray Ni mailto:ray...@intel.com> >>
Cc: John Mathew mailto:john.math...@intel.com> 
>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com> 
>>
Signed-off-by: Gua Guo mailto:gua@intel.com> 
>>
---
.../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++
1 file changed, 7 insertions(+)




diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..bb8426dc0a 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor
+++ eHobLib.c
@@ -34,6 +34,13 @@ CreateHob (








HandOffHob = GetHobList ();












+ //




+ // Check Length to avoid data overflow.




+ //




+ if (HobLength > MAX_UINT16 - 0x7) {




+ return NULL;
[SAMI] Although this fix is correct, I think it shifts the problem somewhere 
else. 
If the above condition occurs, a NULL is returned. A quick scan reveals that 
the calling functions do not check the returned value before use.
e.g. 
https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170
 

There are multiple such places where the calling functions do not check the 
pointer returned by CreateHob(). 
I believe a similar situation can happen 

Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Sami Mujawar]

Hi Gua,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar
On 11/01/2024, 14:19, "Guo, Gua" mailto:gua@intel.com>> wrote:


You mean we need to add below error handle after all callers ?


Hob = CreateHob (...)
ASSERT (Hob != NULL); < Here
[SAMI] That would certainly help catch issues in the debug builds. But the 
problem with asserts is, they vanish in release builds. 
I think we should consider adding appropriate error handling in the calling 
functions to make sure that they do not result in a crash.
[/SAMI]




Thanks,
Gua
-Original Message-
From: Sami Mujawar mailto:sami.muja...@arm.com>> 
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua mailto:gua@intel.com>>; 
devel@edk2.groups.io 
Cc: Marc Beatove mailto:mbeat...@google.com>>; Ard 
Biesheuvel mailto:ardb+tianoc...@kernel.org>>; Ni, 
Ray mailto:ray...@intel.com>>; Mathews, John 
mailto:john.math...@intel.com>>; Gerd Hoffmann 
mailto:kra...@redhat.com>>; nd mailto:n...@arm.com>>
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()


Hi Gua,


Thank you for this patch.
Please see my response inline marked [SAMI].


Regards,


Sami Mujawar


On 11/01/2024, 09:15, "gua@intel.com  
>" mailto:gua@intel.com> >> wrote:




From: Gua Guo mailto:gua@intel.com> 
>>




REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 
 
 





Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765




The CreateHob() function aligns the requested size to 8 performing the 
following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); ```




No checks are performed to ensure this value doesn't overflow, and could lead 
to CreateHob() returning a smaller HOB than requested, which could lead to OOB 
HOB accesses.




Reported-by: Marc Beatove mailto:mbeat...@google.com> 
>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org> >>
Cc: Sami Mujawar mailto:sami.muja...@arm.com> 
>>
Cc: Ray Ni mailto:ray...@intel.com> >>
Cc: John Mathew mailto:john.math...@intel.com> 
>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com> 
>>
Signed-off-by: Gua Guo mailto:gua@intel.com> 
>>
---
.../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++
1 file changed, 7 insertions(+)




diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..bb8426dc0a 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor
+++ eHobLib.c
@@ -34,6 +34,13 @@ CreateHob (








HandOffHob = GetHobList ();












+ //




+ // Check Length to avoid data overflow.




+ //




+ if (HobLength > MAX_UINT16 - 0x7) {




+ return NULL;
[SAMI] Although this fix is correct, I think it shifts the problem somewhere 
else. 
If the above condition occurs, a NULL is returned. A quick scan reveals that 
the calling functions do not check the returned value before use.
e.g. 
https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170
 

There are multiple such places where the calling functions do not check the 
pointer returned by CreateHob(). 
I believe a similar situation can happen for the other patches in this series.
[/SAMI]


+ }




+




HobLength = (UINT16)((HobLength + 0x7) & (~0x7));












FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;




--
2.39.2.windows.1















-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113622): https://edk2.groups.io/g/devel/message/113622
Mute This Topic: https://groups.io/mt/103658960/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/5] Platform/RaspberryPi/DualSerialPortLib: Always configure the pl011

[Jeremy Linton]

Hi,

On 1/11/24 01:46, Ard Biesheuvel wrote:

Hi Jeremy,

Thanks for the patches.


Thanks for looking at them!




On Thu, 11 Jan 2024 at 00:52, Jeremy Linton  wrote:


The rpi's config.txt controls which uart (pl011, or miniuart) is
selected as the console. TFA and edk2 follow its lead, but if the
miniuart is selected as the primary and the machine is booted in ACPI
mode the baud/etc is never configured for the pl011. The linux kernel
won't reconfigure it either as its listed as a "SBSA" uart, so it
simply won't work.

This re-enables BT on the pl011 in ACPI mode, and it somewhat starts
to work again.

Signed-off-by: Jeremy Linton 
---
  .../DualSerialPortLib/DualSerialPortLib.c | 37 +++
  1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c 
b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
index d2f983bf0a..79545d93d6 100644
--- a/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
+++ b/Platform/RaspberryPi/Library/DualSerialPortLib/DualSerialPortLib.c
@@ -76,6 +76,8 @@ SerialPortInitialize (
EFI_PARITY_TYPE Parity;
UINT8   DataBits;
EFI_STOP_BITS_TYPE  StopBits;
+  RETURN_STATUS   Ret;
+  UINTN   Timeout;


What is this for?


IIRC, its as you see, being used below to avoid an infinite loop when 
the miniuart doesn't become ready. That shouldn't be possible with this 
set, so its a safety change.


But IIRC, the infinite loop can be triggered by trying to enable the 
miniuart early, when its not the console, before the config dxe is run 
to turn the clock/regulator on. Which is what I was originally trying to 
do in this patch by configuring both uarts unconditionally.







//
// First thing we need to do is determine which of PL011 or miniUART is 
selected
@@ -85,23 +87,27 @@ SerialPortInitialize (
  UsePl011UartSet = TRUE;
}

-  if (UsePl011Uart) {
-BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
+  // always init the pl011 on the pi4, linux expects a SBSA uart to be at 
115200
+  // this means we need to set the baud/etc even if we arn't using it as a 
console


^ I should probably fix the spelling issues there too.


+  if ((UsePl011Uart) || (RPI_MODEL == 4)) {
  ReceiveFifoDepth = 0; // Use default FIFO depth
+BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);


Shouldn't we hardcode 115200 here if !UsePl011Uart?


Probably? For spec compliance for sure.

Although the BT as I mentioned is flaky, it will scan and connect to 
things like keyboards/mice and sorta work, although it may drop the 
connections/etc. A2dp devices are a no-go. But, running the port faster 
than 115200 seems to at least extend how long it works. So, presumably 
part of the problem is that the BT wants to bump the baud rate and reset 
the device, neither of which it can do in ACPI mode at the moment. And 
for clarity it does some of this in DT mode too, so its not entirely 
just baud rate and reset problems. I spent a fair bit of time a year+ 
back trying to get the BT to work reliably in just DT mode with mainline 
on the miniuart as well as on the pl011 where it sorta works and moved 
on to other things before solving the problem.


So, I guess I can hard-code it here, at least then we are spec compliant.





  Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
  DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits);

-return PL011UartInitializePort (
- PL011_UART_REGISTER_BASE,
- PL011UartClockGetFreq(),
- ,
- ,
- ,
- ,
- 
- );
-  } else {
+Ret = PL011UartInitializePort (
+   PL011_UART_REGISTER_BASE,
+   PL011UartClockGetFreq(),
+   ,
+   ,
+   ,
+   ,
+   
+   );
+  }
+
+  if (!UsePl011Uart) {
  SerialRegisterBase = MINI_UART_REGISTER_BASE;
  Divisor = SerialPortGetDivisor (PcdGet32 (PcdSerialBaudRate));

@@ -127,7 +133,8 @@ SerialPortInitialize (
  // Wait for the serial port to be ready.
  // Verify that both the transmit FIFO and the shift register are empty.
  //
-while ((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & 
(B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY));
+Timeout = 1000;
+while (((SerialPortReadRegister (SerialRegisterBase, R_UART_LSR) & 
(B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) != (B_UART_LSR_TEMT | B_UART_LSR_TXRDY)) && 
(Timeout--));

  //
  // Configure baud rate
@@ -158,9 +165,9 @@ SerialPortInitialize (
  // Put Modem Control Register(MCR) into its reset state of 0x00.
  //
  SerialPortWriteRegister (SerialRegisterBase, R_UART_MCR, 0x00);
-
-return RETURN_SUCCESS;
+Ret = RETURN_SUCCESS;
}
+  return Ret;
  }

  /**
--
2.43.0

Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

You mean we need to add below error handle after all callers ?

Hob = CreateHob (...)
ASSERT (Hob != NULL); < Here


Thanks,
Gua
-Original Message-
From: Sami Mujawar  
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua ; devel@edk2.groups.io
Cc: Marc Beatove ; Ard Biesheuvel 
; Ni, Ray ; Mathews, John 
; Gerd Hoffmann ; nd 
Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

Hi Gua,

Thank you for this patch.
Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 11/01/2024, 09:15, "gua@intel.com " 
mailto:gua@intel.com>> wrote:


From: Gua Guo mailto:gua@intel.com>>


REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 



Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765


The CreateHob() function aligns the requested size to 8 performing the 
following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); ```


No checks are performed to ensure this value doesn't overflow, and could lead 
to CreateHob() returning a smaller HOB than requested, which could lead to OOB 
HOB accesses.


Reported-by: Marc Beatove mailto:mbeat...@google.com>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: John Mathew mailto:john.math...@intel.com>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com>>
Signed-off-by: Gua Guo mailto:gua@intel.com>>
---
.../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++
1 file changed, 7 insertions(+)


diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..bb8426dc0a 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor
+++ eHobLib.c
@@ -34,6 +34,13 @@ CreateHob (




HandOffHob = GetHobList ();






+ //


+ // Check Length to avoid data overflow.


+ //


+ if (HobLength > MAX_UINT16 - 0x7) {


+ return NULL;
[SAMI] Although this fix is correct, I think it shifts the problem somewhere 
else. 
If the above condition occurs, a NULL is returned. A quick scan reveals that 
the calling functions do not check the returned value before use.
e.g. 
https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170
There are multiple such places where the calling functions do not check the 
pointer returned by CreateHob(). 
I believe a similar situation can happen for the other patches in this series.
[/SAMI]

+ }


+


HobLength = (UINT16)((HobLength + 0x7) & (~0x7));






FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;


--
2.39.2.windows.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113620): https://edk2.groups.io/g/devel/message/113620
Mute This Topic: https://groups.io/mt/103658960/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Sami Mujawar]

Hi Gua,

Thank you for this patch.
Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 11/01/2024, 09:15, "gua@intel.com " 
mailto:gua@intel.com>> wrote:


From: Gua Guo mailto:gua@intel.com>>


REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 



Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765


The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```


No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.


Reported-by: Marc Beatove mailto:mbeat...@google.com>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: John Mathew mailto:john.math...@intel.com>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com>>
Signed-off-by: Gua Guo mailto:gua@intel.com>>
---
.../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++
1 file changed, 7 insertions(+)


diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..bb8426dc0a 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
@@ -34,6 +34,13 @@ CreateHob (




HandOffHob = GetHobList ();






+ //


+ // Check Length to avoid data overflow.


+ //


+ if (HobLength > MAX_UINT16 - 0x7) {


+ return NULL;
[SAMI] Although this fix is correct, I think it shifts the problem somewhere 
else. 
If the above condition occurs, a NULL is returned. A quick scan reveals that 
the calling functions do not check the returned value before use.
e.g. 
https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170
There are multiple such places where the calling functions do not check the 
pointer returned by CreateHob(). 
I believe a similar situation can happen for the other patches in this series.
[/SAMI]

+ }


+


HobLength = (UINT16)((HobLength + 0x7) & (~0x7));






FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;


-- 
2.39.2.windows.1







-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113619): https://edk2.groups.io/g/devel/message/113619
Mute This Topic: https://groups.io/mt/103658960/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Gerd Hoffmann]

In some cases (specifically when the flash update region is
small but crosses a multiple of P30_MAX_BUFFER_SIZE_IN_BYTES)
NorFlashWriteSingleBlock reads only one instead of two
P30_MAX_BUFFER_SIZE_IN_BYTES blocks into the shadow buffer.

That leads to random crap being written to the second block,
which in turn can corrupt both the variable store and the
FTW work space.

This patch fixes the calculation.

Signed-off-by: Gerd Hoffmann 
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c 
b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 1afd60ce66eb..cdc809d75e3d 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
Instance,
Lba,
Offset & ~BOUNDARY_OF_32_WORDS,
-   (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
+   (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | 
BOUNDARY_OF_32_WORDS) + 1,
Instance->ShadowBuffer
);
 if (EFI_ERROR (Status)) {
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113618): https://edk2.groups.io/g/devel/message/113618
Mute This Topic: https://groups.io/mt/103661868/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

Hi @Gao, Liming

I may need to get your help to check this change when you're available.
If it's fine for you from MdeModulePkg. I think we can merge this PR.

https://github.com/tianocore/edk2/pull/5252

Thanks,
Gua

-Original Message-
From: Guo, Gua  
Sent: Thursday, January 11, 2024 5:15 PM
To: devel@edk2.groups.io
Cc: Guo, Gua ; Marc Beatove ; Gao, 
Liming ; Mathews, John ; Gerd 
Hoffmann 
Subject: [PATCH v2 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob()

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8 performing the 
following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); ```

No checks are performed to ensure this value doesn't overflow, and could lead 
to CreateHob() returning a smaller HOB than requested, which could lead to OOB 
HOB accesses.

Reported-by: Marc Beatove 
Cc: Liming Gao 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c b/MdeModulePkg/Core/Pei/Hob/Hob.c 
index c4882a23cd..985da50995 100644
--- a/MdeModulePkg/Core/Pei/Hob/Hob.c
+++ b/MdeModulePkg/Core/Pei/Hob/Hob.c
@@ -85,7 +85,7 @@ PeiCreateHob (
   //   // Check Length to avoid data overflow.   //-  if (0x1 - Length <= 
0x7) {+  if (MAX_UINT16 - Length < 0x7) { return EFI_INVALID_PARAMETER;   } 
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113617): https://edk2.groups.io/g/devel/message/113617
Mute This Topic: https://groups.io/mt/103658964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/4] RISC-V: Add support for Sstc extension

[Sunil V L]

On Mon, Jan 08, 2024 at 05:06:46PM +0530, Sunil V L wrote:
> This series adds the support for RISC-V Sstc extension in EDK2 timer
> implementation. Sstc extension allows S-mode software to program the
> timer directly without using SBI calls.
> 
> Currently, PCD variable is used to detect whether feature is enabled. By
> default the feature is enabled and platforms need to set the PCD to
> disable the feature if Sstc is not supported.
> 
> For RiscVVirtQemu, it is disabled by default (until extension discovery
> feature is enabled).
> 
> Changes since v1:
>   1) Updated "PATCH 3" to address Laszlo's comments.
>   2) Updated RB tag for PATCH 4.
> 
> Cc: Andrei Warkentin 
> Cc: Ard Biesheuvel 
> Cc: Gerd Hoffmann 
> Cc: Jiewen Yao 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Ray Ni 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Zhiguang Liu 
> 
Thank you very much for reviews!. Merged as #5232.

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113616): https://edk2.groups.io/g/devel/message/113616
Mute This Topic: https://groups.io/mt/103595207/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Laszlo Ersek]

On 1/11/24 09:46, Laszlo Ersek wrote:
> On 1/10/24 22:50, Pedro Falcato wrote:

>> For the protocol database, you'd replace the linked list with a simple
>> hashtable, hashed by protocol. Something as simple as LIST_ENTRY
>> mProtocolHashtable[64]; would probably be enough to fan out most of
>> the problems (I think? How many different protocols are there?)
> 
> I can answer this question reasonably well, I think. I have a script
> that collects symbolic names of GUIDs from the edk2 tree (plus hardcodes
> a number of well-known but not edk2 GUIDs), and creates a "sed" script
> out of them. Then another script uses this "sed" script for filtering
> edk2 logs -- the idea being to replace the whole bunch of logged GUIDs
> with their symbolic names. That makes logs much easier to read.
> 
> The generator script is written such a way that the generated "sed"
> script only grows over time; put differently, this "dictionary" of
> name<->GUID associations never forgets, it only picks up new GUIDs. The
> "sed" script (= the dictionary file) consists of entries like
> 
> s:FFB19961-79CC-4684-84A8-C31B0A2BBE82:[EarlyFdt16550SerialPortHookLib]:ig
> s:FFB56F09-65E3-4462-A799-2F0D1930D38C:[DxeContextBufferModuleConfigLib]:ig
> s:FFE06BDD-6107-46A6-7BB2-5A9C7EC5275C:[EfiAcpiTableProtocol]:ig
> s:FFF12B8D-7696-4C8B-A985-2747075B4F50:[EfiSystemNvDataFv]:ig
> 
> it's sorted uniquely by GUID.
> 
> Right now, it has 3074 entries. (People like generating GUIDs! :) In
> PI/UEFI/edk2, *everything* is a GUID, not just protocols!)

If I grep the dictionary for "Protocol", I get 515 hits.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113615): https://edk2.groups.io/g/devel/message/113615
Mute This Topic: https://groups.io/mt/103594587/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob()

[Gerd Hoffmann]

On Thu, Jan 11, 2024 at 05:14:35PM +0800, gua@intel.com wrote:
> From: Gua Guo 
> 
> Gua Guo (4):
>   UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
>   StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
>   EmbeddedPkg/Hob: Integer Overflow in CreateHob()
>   MdeModulePkg/Hob: Integer Overflow in CreateHob()

Series:
Acked-by: Gerd Hoffmann 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113614): https://edk2.groups.io/g/devel/message/113614
Mute This Topic: https://groups.io/mt/103658961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[Gerd Hoffmann]

On Thu, Jan 11, 2024 at 04:59:47PM +0800, Dun Tan wrote:
> When creating smm page table, limit maximum
> supported physical address bits returned by
> CalculateMaximumSupportAddress() to 47 if
> 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the
> PhysicalAddressBits retrived from CPU HOB or
> CpuId is bigger than 47, and since virtual
> addresses are sign-extended, only [0, 2^47-1]
> range in 52-bit physical address is mapped
> in page table.

> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page table
> +  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);
> +  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
> +PhysicalAddressBits = 47;
> +  }

The code change is fine but the comment should be more verbose and
explain the why 47 not 48 is used here.  The discussion on the patch
clearly showed that the technical background is not obvious ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113613): https://edk2.groups.io/g/devel/message/113613
Mute This Topic: https://groups.io/mt/103658816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: change name of gMpInformationHobGuid2

[Laszlo Ersek]

On 1/11/24 10:00, Dun Tan wrote:
> Change name of gMpInformationHobGuid2 to
> gMpInformation2HobGuid. It's to align with
> the file name MpInformation2.h and the
> structure name MP_INFORMATION2_HOB_DATA.
> 
> Signed-off-by: Dun Tan 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 8 
>  UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 2 +-
>  UefiCpuPkg/Include/Guid/MpInformation2.h | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 6 +++---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 2 +-
>  UefiCpuPkg/UefiCpuPkg.dec| 2 +-
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> index 93919be94f..2ce4d6ab50 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -566,7 +566,7 @@ GetProcessorCoreType (
>  }
>  
>  /**
> -  Create gMpInformationHobGuid2.
> +  Create gMpInformation2HobGuid.
>  **/
>  VOID
>  BuildMpInformationHob (
> @@ -618,13 +618,13 @@ BuildMpInformationHob (
>//
>// Create MP_INFORMATION2_HOB. when the max HobLength 0xFFF8 is not 
> enough, there
>// will be a MP_INFORMATION2_HOB series in the HOB list.
> -  // In the HOB list, there is a gMpInformationHobGuid2 with 0 value 
> NumberOfProcessors
> +  // In the HOB list, there is a gMpInformation2HobGuid with 0 value 
> NumberOfProcessors
>// fields to indicate it's the last MP_INFORMATION2_HOB.
>//
>while (NumberOfProcessorsInHob != 0) {
>  NumberOfProcessorsInHob = MIN (NumberOfProcessors - ProcessorIndex, 
> MaxProcessorsPerHob);
>  MpInformation2HobData   = BuildGuidHob (
> -,
> +,
>  sizeof (MP_INFORMATION2_HOB_DATA) + sizeof 
> (MP_INFORMATION2_ENTRY) * NumberOfProcessorsInHob
>  );
>  ASSERT (MpInformation2HobData != NULL);
> @@ -744,7 +744,7 @@ InitializeCpuMpWorker (
>ASSERT_EFI_ERROR (Status);
>  
>//
> -  // Create gMpInformationHobGuid2
> +  // Create gMpInformation2HobGuid
>//
>BuildMpInformationHob ();
>  
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf 
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> index 812fa179bd..9ab2623bd0 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
> @@ -50,7 +50,7 @@
>  
>  [Guids]
>gEdkiiMigratedFvInfoGuid ## 
> SOMETIMES_CONSUMES ## HOB
> -  gMpInformationHobGuid2## PRODUCES
> +  gMpInformation2HobGuid## PRODUCES
>  
>  [Ppis]
>gEfiPeiMpServicesPpiGuid  ## PRODUCES
> diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h 
> b/UefiCpuPkg/Include/Guid/MpInformation2.h
> index 43185a4b01..2d9266f061 100644
> --- a/UefiCpuPkg/Include/Guid/MpInformation2.h
> +++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
> @@ -53,6 +53,6 @@ typedef struct {
>  #define GET_MP_INFORMATION_ENTRY(MpInfoHobData, Index) \
>  (MP_INFORMATION2_ENTRY *)((UINTN)&((MP_INFORMATION2_HOB_DATA 
> *)(MpInfoHobData))->Entry + (MpInfoHobData)->EntrySize * Index)
>  
> -extern EFI_GUID  gMpInformationHobGuid2;
> +extern EFI_GUID  gMpInformation2HobGuid;
>  
>  #endif
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index 9b230772cb..cd394826ff 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -776,7 +776,7 @@ GetMpInformation (
>HobIndex  = 0;
>HobCount  = 0;
>  
> -  FirstMpInfo2Hob = GetFirstGuidHob ();
> +  FirstMpInfo2Hob = GetFirstGuidHob ();
>ASSERT (FirstMpInfo2Hob != NULL);
>GuidHob = FirstMpInfo2Hob;
>while (GuidHob != NULL) {
> @@ -792,7 +792,7 @@ GetMpInformation (
>  
>  HobCount++;
>  *NumberOfCpus += MpInformation2HobData->NumberOfProcessors;
> -GuidHob= GetNextGuidHob (, GET_NEXT_HOB 
> (GuidHob));
> +GuidHob= GetNextGuidHob (, GET_NEXT_HOB 
> (GuidHob));
>}
>  
>ASSERT (*NumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> @@ -820,7 +820,7 @@ GetMpInformation (
>GuidHob = FirstMpInfo2Hob;
>while (HobIndex < HobCount) {
>  MpInfo2Hobs[HobIndex++] = GET_GUID_HOB_DATA (GuidHob);
> -GuidHob = GetNextGuidHob (, 
> GET_NEXT_HOB (GuidHob));
> +GuidHob = GetNextGuidHob (, 
> GET_NEXT_HOB (GuidHob));
>}
>  
>ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof 
> (EFI_PROCESSOR_INFORMATION) * (*MaxNumberOfCpus));
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 793220aba3..a018954ed7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -120,7 

Re: [edk2-devel] [PATCH 1/2] CloudHv: Add CloudHv support to PlatformScanE820 utility function.

[Laszlo Ersek]

Hello Thomas,

(+ Jianyong, Anatol, Gerd)

On 1/10/24 23:21, Thomas Barrett wrote:
> Signed-off-by: Thomas Barrett 
> ---
> OvmfPkg/Library/PlatformInitLib/MemDetect.c | 95 ++---
> 1 file changed, 65 insertions(+), 30 deletions(-)

please don't paste patches in email bodies; they are hard to read
(review) and effectively impossible to apply that way.

Here's the official dev guide:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

A few more personal ideas:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers

Please don't forget to run the GetMaintainer.py script either, for
composing the Cc: tags in the commit message body.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113611): https://edk2.groups.io/g/devel/message/113611
Mute This Topic: https://groups.io/mt/103657892/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/4] EmbeddedPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Leif Lindholm 
Reviewed-by: Ard Biesheuvel 
Cc: Abner Chang 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 EmbeddedPkg/Library/PrePiHobLib/Hob.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/EmbeddedPkg/Library/PrePiHobLib/Hob.c 
b/EmbeddedPkg/Library/PrePiHobLib/Hob.c
index 8eb175aa96..1fe3ea93e4 100644
--- a/EmbeddedPkg/Library/PrePiHobLib/Hob.c
+++ b/EmbeddedPkg/Library/PrePiHobLib/Hob.c
@@ -110,6 +110,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113608): https://edk2.groups.io/g/devel/message/113608
Mute This Topic: https://groups.io/mt/103658962/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Guo Dong 
Cc: Sean Rhodes 
Cc: James Lu 
Reviewed-by: Gua Guo 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c 
b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
index 2c3acbbc19..39f07d1964 100644
--- a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
+++ b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c
@@ -110,6 +110,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113609): https://edk2.groups.io/g/devel/message/113609
Mute This Topic: https://groups.io/mt/103658963/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Cc: Liming Gao 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c b/MdeModulePkg/Core/Pei/Hob/Hob.c
index c4882a23cd..985da50995 100644
--- a/MdeModulePkg/Core/Pei/Hob/Hob.c
+++ b/MdeModulePkg/Core/Pei/Hob/Hob.c
@@ -85,7 +85,7 @@ PeiCreateHob (
   //
   // Check Length to avoid data overflow.
   //
-  if (0x1 - Length <= 0x7) {
+  if (MAX_UINT16 - Length < 0x7) {
 return EFI_INVALID_PARAMETER;
   }
 
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113610): https://edk2.groups.io/g/devel/message/113610
Mute This Topic: https://groups.io/mt/103658964/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

V2
1. UefiPayloadPkg/Hob: Integer : Add Reviewed-by and Authored-by

2. StandaloneMmPkg/Hob: Integer Overflow in : Add Reviewed-by and Authored-by

3. EmbeddedPkg/Hob: Integer Overflow in CreateHob() : Add Reviewed-by and 
Authored-by

4. MdeModulePkg/Hob: Integer Overflow in CreateHob() : Add Authored-by

V1

1. UefiPayloadPkg/Hob: Integer

2. StandaloneMmPkg/Hob: Integer Overflow in

3. EmbeddedPkg/Hob: Integer Overflow in CreateHob()

4. MdeModulePkg/Hob: Integer Overflow in CreateHob()

Cc: Ard Biesheuvel 

Cc: Gerd Hoffmann 

Cc: John Mathew 

Cc: Vincent Zimmer 

Gua Guo (4):
  UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
  StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
  EmbeddedPkg/Hob: Integer Overflow in CreateHob()
  MdeModulePkg/Hob: Integer Overflow in CreateHob()

 EmbeddedPkg/Library/PrePiHobLib/Hob.c  | 7 +++
 MdeModulePkg/Core/Pei/Hob/Hob.c| 2 +-
 .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c| 7 +++
 UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c| 7 +++
 4 files changed, 22 insertions(+), 1 deletion(-)

--
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113607): https://edk2.groups.io/g/devel/message/113607
Mute This Topic: https://groups.io/mt/103658961/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

[Guo, Gua]

From: Gua Guo 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166

Fix integer overflow in various CreateHob instances.
Fixes: CVE-2022-36765

The CreateHob() function aligns the requested size to 8
performing the following operation:
```
HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
```

No checks are performed to ensure this value doesn't
overflow, and could lead to CreateHob() returning a smaller
HOB than requested, which could lead to OOB HOB accesses.

Reported-by: Marc Beatove 
Reviewed-by: Ard Biesheuvel 
Cc: Sami Mujawar 
Cc: Ray Ni 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c| 7 +++
 1 file changed, 7 insertions(+)

diff --git 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..bb8426dc0a 100644
--- 
a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ 
b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
@@ -34,6 +34,13 @@ CreateHob (
 
   HandOffHob = GetHobList ();
 
+  //
+  // Check Length to avoid data overflow.
+  //
+  if (HobLength > MAX_UINT16 - 0x7) {
+return NULL;
+  }
+
   HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
 
   FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-- 
2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113606): https://edk2.groups.io/g/devel/message/113606
Mute This Topic: https://groups.io/mt/103658960/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

Hi Gerd,

Could you help to review this V3 patch? The related code and comments has been 
modified based on your previous comments.
Please ignore the V2 patch set. There was a typo error in the V2 patch and was 
corrected in V3 patch.

Thanks,
Dun

-Original Message-
From: devel@edk2.groups.io  On Behalf Of duntan
Sent: Thursday, January 11, 2024 5:00 PM
To: devel@edk2.groups.io
Cc: Ni, Ray ; Laszlo Ersek ; Kumar, Rahul 
R ; Gerd Hoffmann 
Subject: [edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in 
special case

When creating smm page table, limit maximum supported physical address bits 
returned by
CalculateMaximumSupportAddress() to 47 if 5-Level Paging is disabled.
When 5-Level Paging is disabled and the
PhysicalAddressBits retrived from CPU HOB or CpuId is bigger than 47, and since 
virtual addresses are sign-extended, only [0, 2^47-1] range in 52-bit physical 
address is mapped in page table.

Signed-off-by: Dun Tan 
Reviewed-by: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index ddd9be66b5..35c282a771 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -137,11 +137,13 @@ GetSubEntriesNum (
 /**
   Calculate the maximum support address.
 
+  @param[in] Is5LevelPagingNeededIf 5-level paging enabling is needed.
+
   @return the maximum support address.
 **/
 UINT8
 CalculateMaximumSupportAddress (
-  VOID
+  BOOLEAN  Is5LevelPagingNeeded
   )
 {
   UINT32  RegEax;
@@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
 }
   }
 
+  //
+  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page 
+ table  // when 5-Level Paging is disabled.
+  //
+  ASSERT (PhysicalAddressBits <= 52);
+  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
+PhysicalAddressBits = 47;
+  }
+
   return PhysicalAddressBits;
 }
 
@@ -197,7 +208,7 @@ SmmInitPageTable (
   mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess);
   m1GPageTableSupport   = Is1GPageSupport ();
   m5LevelPagingNeeded   = Is5LevelPagingNeeded ();
-  mPhysicalAddressBits  = CalculateMaximumSupportAddress ();
+  mPhysicalAddressBits  = CalculateMaximumSupportAddress 
(m5LevelPagingNeeded);
   PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
   if (m5LevelPagingNeeded) {
 mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
--
2.31.1.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113605): https://edk2.groups.io/g/devel/message/113605
Mute This Topic: https://groups.io/mt/103658816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint

[levi.yun]

Hi Brian.


> Ard didn't want a SerialPortInitialize() call directly in the
> all-platform StandaloneMmCore _ModuleEntryPoint() function, which is
> understandable.  So perhaps you could either:

Thanks to corret me :)

> 1. Propose a platform-specific callout at that point and a library class
> to implement it, with an empty instance for general use and your own
> platform-specific instance which calls SerialPortInitialize().

Thanks for suggestion.
IIUC, It looks good to me to make something like
PlatformEalryInitialize hook called in early stage of StandaloneMm
and let it be implemented on each edk2-platform to initialize Serialport early 
and etc.

Many thanks!





IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113604): https://edk2.groups.io/g/devel/message/113604
Mute This Topic: https://groups.io/mt/103540969/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdePkg: Add EFI_UNSUPPORTED return for some Runtime Service functions

[Ren, Suqiang]

Hi All,

Any comments about this patch?

Thanks
Ren, Suqiang

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Ren, Suqiang
Sent: Wednesday, December 27, 2023 4:47 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D ; Gao, Liming 
; Liu, Zhiguang 
Subject: [edk2-devel] [PATCH 1/1] MdePkg: Add EFI_UNSUPPORTED return for some 
Runtime Service functions

According to UEFI Spec 2.10 page 206, if any EFI_RUNTIME_SERVICES* calls are 
not supported for use by the OS at runtime, an EFI_RT_PROPERTIES_TABLE 
configuration table should be published describing which runtime services are 
supported at runtime. So need to add EFI_UNSUPPORTED return for some Runtime 
Service functions.

REF: UEFI spec 2.10 section 8 Services — Runtime Services

Signed-off-by: Suqiang Ren 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Zhiguang Liu 
---
 MdePkg/Include/Uefi/UefiSpec.h | 40 --
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Uefi/UefiSpec.h b/MdePkg/Include/Uefi/UefiSpec.h 
index 5de00e8ea2af..b25485b06763 100644
--- a/MdePkg/Include/Uefi/UefiSpec.h
+++ b/MdePkg/Include/Uefi/UefiSpec.h
@@ -320,6 +320,9 @@ EFI_STATUS
 map that requires a mapping.
   @retval EFI_NOT_FOUND A virtual address was supplied for an address 
that is not found
 in the memory map.
+  @retval EFI_UNSUPPORTED   This call is not supported by this platform at 
the time the call is made.
+The platform should describe this runtime 
service as unsupported at runtime
+via an EFI_RT_PROPERTIES_TABLE configuration 
table.
 
 **/
 typedef
@@ -415,6 +418,9 @@ EFI_STATUS
 not have the EFI_OPTIONAL_PTR bit set.
   @retval EFI_NOT_FOUND The pointer pointed to by Address was not 
found to be part
 of the current memory map. This is normally 
fatal.
+  @retval EFI_UNSUPPORTED   This call is not supported by this platform at 
the time the call is made.
+The platform should describe this runtime 
service as unsupported at runtime
+via an EFI_RT_PROPERTIES_TABLE configuration 
table.
 
 **/
 typedef
@@ -679,6 +685,10 @@ VOID
   @retval EFI_INVALID_PARAMETER  The DataSize is not too small and Data is 
NULL.
   @retval EFI_DEVICE_ERROR   The variable could not be retrieved due to a 
hardware error.
   @retval EFI_SECURITY_VIOLATION The variable could not be retrieved due to an 
authentication failure.
+  @retval EFI_UNSUPPORTEDAfter ExitBootServices() has been called, 
this return code may be returned
+ if no variable storage is supported. The 
platform should describe this
+ runtime service as unsupported at runtime via 
an EFI_RT_PROPERTIES_TABLE
+ configuration table.
 
 **/
 typedef
@@ -715,6 +725,10 @@ EFI_STATUS
   @retval EFI_INVALID_PARAMETER Null-terminator is not found in the first 
VariableNameSize bytes of
 the input VariableName buffer.
   @retval EFI_DEVICE_ERROR  The variable could not be retrieved due to a 
hardware error.
+  @retval EFI_UNSUPPORTED   After ExitBootServices() has been called, this 
return code may be returned
+if no variable storage is supported. The 
platform should describe this
+runtime service as unsupported at runtime via 
an EFI_RT_PROPERTIES_TABLE
+configuration table.
 
 **/
 typedef
@@ -757,6 +771,9 @@ EFI_STATUS
  but the AuthInfo does NOT pass the validation 
check carried out by the firmware.
 
   @retval EFI_NOT_FOUND  The variable trying to be updated or deleted 
was not found.
+  @retval EFI_UNSUPPORTEDThis call is not supported by this platform 
at the time the call is made.
+ The platform should describe this runtime 
service as unsupported at runtime
+ via an EFI_RT_PROPERTIES_TABLE configuration 
table.
 
 **/
 typedef
@@ -809,6 +826,9 @@ typedef struct {
   @retval EFI_SUCCESS   The operation completed successfully.
   @retval EFI_INVALID_PARAMETER Time is NULL.
   @retval EFI_DEVICE_ERROR  The time could not be retrieved due to 
hardware error.
+  @retval EFI_UNSUPPORTED   This call is not supported by this platform at 
the time the call is made.
+The platform should describe this runtime 
service as unsupported at runtime
+via an EFI_RT_PROPERTIES_TABLE configuration 
table.
 
 **/
 typedef
@@ -826,6 +846,9 @@ EFI_STATUS
   @retval EFI_SUCCESS   The operation completed successfully.
   @retval EFI_INVALID_PARAMETER A 

Re: [edk2-devel] [PATCH V2 1/1] MdePkg: Update the definition of FileName on EFI_FILE_INFO

[Ren, Suqiang]

Hi All,

Any comments about this patch?

Thanks
Ren, Suqiang

-Original Message-
From: devel@edk2.groups.io  On Behalf Of Ren, Suqiang
Sent: Tuesday, December 26, 2023 1:22 PM
To: devel@edk2.groups.io
Cc: Kinney, Michael D ; Gao, Liming 
; Liu, Zhiguang 
Subject: [edk2-devel] [PATCH V2 1/1] MdePkg: Update the definition of FileName 
on EFI_FILE_INFO

Add the description of FileName to align with UEFI spec 2.10.

REF: UEFI spec 2.10 Table 13.5.16

Signed-off-by: Suqiang Ren 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Zhiguang Liu 
---
 MdePkg/Include/Guid/FileInfo.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Include/Guid/FileInfo.h b/MdePkg/Include/Guid/FileInfo.h 
index 2b7edf36aabc..c152789b40c8 100644
--- a/MdePkg/Include/Guid/FileInfo.h
+++ b/MdePkg/Include/Guid/FileInfo.h
@@ -46,7 +46,7 @@ typedef struct {
   ///
   UINT64  Attribute;
   ///
-  /// The Null-terminated name of the file.
+  /// The Null-terminated name of the file. For a root directory, the name is 
an empty string.
   ///
   CHAR16  FileName[1];
 } EFI_FILE_INFO;
--
2.26.2.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113602): https://edk2.groups.io/g/devel/message/113602
Mute This Topic: https://groups.io/mt/103366883/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH] UefiCpuPkg: change name of gMpInformationHobGuid2

[duntan]

Change name of gMpInformationHobGuid2 to
gMpInformation2HobGuid. It's to align with
the file name MpInformation2.h and the
structure name MP_INFORMATION2_HOB_DATA.

Signed-off-by: Dun Tan 
Cc: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 8 
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf | 2 +-
 UefiCpuPkg/Include/Guid/MpInformation2.h | 2 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c   | 6 +++---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 2 +-
 UefiCpuPkg/UefiCpuPkg.dec| 2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 93919be94f..2ce4d6ab50 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -566,7 +566,7 @@ GetProcessorCoreType (
 }
 
 /**
-  Create gMpInformationHobGuid2.
+  Create gMpInformation2HobGuid.
 **/
 VOID
 BuildMpInformationHob (
@@ -618,13 +618,13 @@ BuildMpInformationHob (
   //
   // Create MP_INFORMATION2_HOB. when the max HobLength 0xFFF8 is not enough, 
there
   // will be a MP_INFORMATION2_HOB series in the HOB list.
-  // In the HOB list, there is a gMpInformationHobGuid2 with 0 value 
NumberOfProcessors
+  // In the HOB list, there is a gMpInformation2HobGuid with 0 value 
NumberOfProcessors
   // fields to indicate it's the last MP_INFORMATION2_HOB.
   //
   while (NumberOfProcessorsInHob != 0) {
 NumberOfProcessorsInHob = MIN (NumberOfProcessors - ProcessorIndex, 
MaxProcessorsPerHob);
 MpInformation2HobData   = BuildGuidHob (
-,
+,
 sizeof (MP_INFORMATION2_HOB_DATA) + sizeof 
(MP_INFORMATION2_ENTRY) * NumberOfProcessorsInHob
 );
 ASSERT (MpInformation2HobData != NULL);
@@ -744,7 +744,7 @@ InitializeCpuMpWorker (
   ASSERT_EFI_ERROR (Status);
 
   //
-  // Create gMpInformationHobGuid2
+  // Create gMpInformation2HobGuid
   //
   BuildMpInformationHob ();
 
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
index 812fa179bd..9ab2623bd0 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.inf
@@ -50,7 +50,7 @@
 
 [Guids]
   gEdkiiMigratedFvInfoGuid ## 
SOMETIMES_CONSUMES ## HOB
-  gMpInformationHobGuid2## PRODUCES
+  gMpInformation2HobGuid## PRODUCES
 
 [Ppis]
   gEfiPeiMpServicesPpiGuid  ## PRODUCES
diff --git a/UefiCpuPkg/Include/Guid/MpInformation2.h 
b/UefiCpuPkg/Include/Guid/MpInformation2.h
index 43185a4b01..2d9266f061 100644
--- a/UefiCpuPkg/Include/Guid/MpInformation2.h
+++ b/UefiCpuPkg/Include/Guid/MpInformation2.h
@@ -53,6 +53,6 @@ typedef struct {
 #define GET_MP_INFORMATION_ENTRY(MpInfoHobData, Index) \
 (MP_INFORMATION2_ENTRY *)((UINTN)&((MP_INFORMATION2_HOB_DATA 
*)(MpInfoHobData))->Entry + (MpInfoHobData)->EntrySize * Index)
 
-extern EFI_GUID  gMpInformationHobGuid2;
+extern EFI_GUID  gMpInformation2HobGuid;
 
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 9b230772cb..cd394826ff 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -776,7 +776,7 @@ GetMpInformation (
   HobIndex  = 0;
   HobCount  = 0;
 
-  FirstMpInfo2Hob = GetFirstGuidHob ();
+  FirstMpInfo2Hob = GetFirstGuidHob ();
   ASSERT (FirstMpInfo2Hob != NULL);
   GuidHob = FirstMpInfo2Hob;
   while (GuidHob != NULL) {
@@ -792,7 +792,7 @@ GetMpInformation (
 
 HobCount++;
 *NumberOfCpus += MpInformation2HobData->NumberOfProcessors;
-GuidHob= GetNextGuidHob (, GET_NEXT_HOB 
(GuidHob));
+GuidHob= GetNextGuidHob (, GET_NEXT_HOB 
(GuidHob));
   }
 
   ASSERT (*NumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
@@ -820,7 +820,7 @@ GetMpInformation (
   GuidHob = FirstMpInfo2Hob;
   while (HobIndex < HobCount) {
 MpInfo2Hobs[HobIndex++] = GET_GUID_HOB_DATA (GuidHob);
-GuidHob = GetNextGuidHob (, 
GET_NEXT_HOB (GuidHob));
+GuidHob = GetNextGuidHob (, 
GET_NEXT_HOB (GuidHob));
   }
 
   ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof 
(EFI_PROCESSOR_INFORMATION) * (*MaxNumberOfCpus));
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 793220aba3..a018954ed7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -120,7 +120,7 @@
   gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
   gEfiMemoryAttributesTableGuid## CONSUMES ## SystemTable
   gSmmBaseHobGuid  ## CONSUMES
-  gMpInformationHobGuid2   ## CONSUMES # Assume the HOB must 
has been 

[edk2-devel] [Patch V3] UefiCpuPkg:Limit PhysicalAddressBits in special case

[duntan]

When creating smm page table, limit maximum
supported physical address bits returned by
CalculateMaximumSupportAddress() to 47 if
5-Level Paging is disabled.
When 5-Level Paging is disabled and the
PhysicalAddressBits retrived from CPU HOB or
CpuId is bigger than 47, and since virtual
addresses are sign-extended, only [0, 2^47-1]
range in 52-bit physical address is mapped
in page table.

Signed-off-by: Dun Tan 
Reviewed-by: Ray Ni 
Cc: Laszlo Ersek 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index ddd9be66b5..35c282a771 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -137,11 +137,13 @@ GetSubEntriesNum (
 /**
   Calculate the maximum support address.
 
+  @param[in] Is5LevelPagingNeededIf 5-level paging enabling is needed.
+
   @return the maximum support address.
 **/
 UINT8
 CalculateMaximumSupportAddress (
-  VOID
+  BOOLEAN  Is5LevelPagingNeeded
   )
 {
   UINT32  RegEax;
@@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
 }
   }
 
+  //
+  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page table
+  // when 5-Level Paging is disabled.
+  //
+  ASSERT (PhysicalAddressBits <= 52);
+  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
+PhysicalAddressBits = 47;
+  }
+
   return PhysicalAddressBits;
 }
 
@@ -197,7 +208,7 @@ SmmInitPageTable (
   mCpuSmmRestrictedMemoryAccess = PcdGetBool (PcdCpuSmmRestrictedMemoryAccess);
   m1GPageTableSupport   = Is1GPageSupport ();
   m5LevelPagingNeeded   = Is5LevelPagingNeeded ();
-  mPhysicalAddressBits  = CalculateMaximumSupportAddress ();
+  mPhysicalAddressBits  = CalculateMaximumSupportAddress 
(m5LevelPagingNeeded);
   PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
   if (m5LevelPagingNeeded) {
 mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113600): https://edk2.groups.io/g/devel/message/113600
Mute This Topic: https://groups.io/mt/103658816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V2] UefiCpuPkg:Limit PhysicalAddressBits in speicial case

[duntan]

Oh, thanks for your comments! Will correct it in next version patch.

Thanks,
Dun

-Original Message-
From: Laszlo Ersek  
Sent: Thursday, January 11, 2024 4:48 PM
To: Tan, Dun ; devel@edk2.groups.io
Cc: Ni, Ray ; Kumar, Rahul R ; Gerd 
Hoffmann 
Subject: Re: [Patch V2] UefiCpuPkg:Limit PhysicalAddressBits in speicial case

On 1/11/24 03:11, Dun Tan wrote:
> When creating smm page table, limit maximum supported physical address 
> bits returned by
> CalculateMaximumSupportAddress() to 47 if 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the PhysicalAddressBits retrived 
> from CPU HOB or CpuId is bigger than 47, and since virtual addresses 
> are sign-extended, only [0, 2^47-1] range in 52-bit physical address 
> is mapped in page table.
> 
> Signed-off-by: Dun Tan 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)

I'll let Gerd review this (thanks!), I just want to point out a typo in the 
subject: "speicial" should be "special".

Thanks
Laszlo

> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index ddd9be66b5..35c282a771 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -137,11 +137,13 @@ GetSubEntriesNum (
>  /**
>Calculate the maximum support address.
>  
> +  @param[in] Is5LevelPagingNeededIf 5-level paging enabling is needed.
> +
>@return the maximum support address.
>  **/
>  UINT8
>  CalculateMaximumSupportAddress (
> -  VOID
> +  BOOLEAN  Is5LevelPagingNeeded
>)
>  {
>UINT32  RegEax;
> @@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
>  }
>}
>  
> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page 
> + table  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);  if (!Is5LevelPagingNeeded && 
> + (PhysicalAddressBits > 47)) {
> +PhysicalAddressBits = 47;
> +  }
> +
>return PhysicalAddressBits;
>  }
>  
> @@ -197,7 +208,7 @@ SmmInitPageTable (
>mCpuSmmRestrictedMemoryAccess = PcdGetBool 
> (PcdCpuSmmRestrictedMemoryAccess);
>m1GPageTableSupport   = Is1GPageSupport ();
>m5LevelPagingNeeded   = Is5LevelPagingNeeded ();
> -  mPhysicalAddressBits  = CalculateMaximumSupportAddress ();
> +  mPhysicalAddressBits  = CalculateMaximumSupportAddress 
> (m5LevelPagingNeeded);
>PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
>if (m5LevelPagingNeeded) {
>  mPagingMode = m1GPageTableSupport ? Paging5Level1GB : 
> Paging5Level;



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113599): https://edk2.groups.io/g/devel/message/113599
Mute This Topic: https://groups.io/mt/103655312/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg: Fix issue that IsModified is wrongly set in PageTableMap

[Laszlo Ersek]

On 1/11/24 03:03, Ni, Ray wrote:
>> This function is incredibly complicated, so reviewing this patch is
>> hard, even after reading the bugzilla ticket.
>>
>> The commit message is useless. It should contain a brief description of
>> the problem, and how the fix resolves the problem.
>>
>> The documentation of the PageTableLibMapInLevel() function is wrong,
>> even before this patch. It documents the "IsModified" output-only
>> parameter as follows:
>>
>> "TRUE means page table is modified. FALSE means page table is not
>> modified."
>>
>> This states that "IsModified" is always set on output, to either FALSE
>> or TRUE. Which is an incorrect statement; in reality the caller is
>> expected to pre-set (*IsModified) to FALSE, and PageTableLibMapInLevel()
>> will (conditionally!) perform a FALSE->TRUE transition only.
>>
>> Now, this patch may fix a bug, but it makes the above-described
>> documentation issue worse, by further restricting the condition for said
>> FALSE->TRUE transition.
> 
> Laszlo, thanks for the comments!
> Though the fixing looks simple, Zhiguang and I did have several rounds of 
> offline discussions
> regarding how to fix it.
> 
> When the lib accesses the page table content, CPU would set the "Access" bit 
> in the page entry
> that points to the page table memory being accessed by the lib.
> 
> So, even when the "Modify" is FALSE (indicating caller doesn't want the lib 
> to modify the page table),
> lib code should not modify the page table but CPU still sets the "Access" bit 
> in some of the entries due to
> the reasons above.

Huh, tricky!

Should the comparison explicitly mask out the Accessed bit from each of
the "before" page table entry and the "after" one, perhaps?

> I agree it will be better that the commit message carries above details.
> 
> Zhiguang,
> Can we update the code to always assign "IsModified"? I thought we did that 
> but it seems not.

That seems doable by (e.g.) setting (*IsModified) to FALSE right at the
top of the function, and then the logic would match the existent
comments, I think. However, I've not checked whether callers actually
rely on this "summing" logic of the IsModified output parameter -- like
call the function a number of times in a row, using a common local
variable to receive IsModified, and then check the local variable to see
if *any one* of the calls in the loop has modified the page table.

Thanks
Laszlo

> 
>>
>> The fix per se looks vaguely reasonable to me (really the function is so
>> complicated that verifying this change from scratch would take me ages),
>> but minimally, the documentation of "IsModified" should certainly be
>> updated too. To something like this:
>>
>>   @param[out] IsModified  If "Modify" is TRUE on input and the function
>>   has actually modified the page table, then
>> set
>>   to TRUE on output. Not overwritten
>> otherwise.
>>
>> Laszlo
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113598): https://edk2.groups.io/g/devel/message/113598
Mute This Topic: https://groups.io/mt/103636407/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in ConvertMemoryPageToNotPresent

[Laszlo Ersek]

On 1/11/24 03:08, Ni, Ray wrote:
> 
> 
> Thanks,
> Ray
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Wednesday, January 10, 2024 7:57 PM
>> To: Liu, Zhiguang ; devel@edk2.groups.io
>> Cc: Ni, Ray ; Kumar, Rahul R ;
>> Gerd Hoffmann 
>> Subject: Re: [PATCH] UefiCpuPkg/CpuMpPei: Don't write CR3 in
>> ConvertMemoryPageToNotPresent
>>
>> On 1/10/24 06:43, Zhiguang Liu wrote:
>>> After ConvertMemoryPageToNotPresent, there is always a flush TLB
>>> function. So, to improve performance, there is no need to write CR3
>>> inside ConvertMemoryPageToNotPresent
>>>
>>> Cc: Ray Ni 
>>> Cc: Laszlo Ersek 
>>> Cc: Rahul Kumar 
>>> Cc: Gerd Hoffmann 
>>> Signed-off-by: Zhiguang Liu 
>>> ---
>>>  UefiCpuPkg/CpuMpPei/CpuPaging.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>> b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> index 15c7015fb8..c6894458f7 100644
>>> --- a/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> +++ b/UefiCpuPkg/CpuMpPei/CpuPaging.c
>>> @@ -167,7 +167,6 @@ ConvertMemoryPageToNotPresent (
>>>}
>>>
>>>ASSERT_EFI_ERROR (Status);
>>> -  AsmWriteCr3 (PageTable);
>>>return Status;
>>>  }
>>>
>>
>> (1) I mostly understand the point that the commit message makes, but the
>> message is not clear enough. The real point is that we have two
>> ConvertMemoryPageToNotPresent() calls altogether, and each of those
>> takes place in a *loop*, and each of those loops is followed by a
>> CpuFlushTlb() call.
>>
>> The loops matter. If there were no loops, then we might be motivated to
>> choose a different solution (for example, to move centralize the
>> CpuFlushTlb() calls *inside* ConvertMemoryPageToNotPresent(), or
>> something similar).
>>
>> So, please update the commit message; mention the loops.
>>
>> (2) I can't easily see why this change is actually correct. IIRC,
>> writing CR3 has a "side effect" of flushing the TLB. But obviously,
>> that's not the *only* effect of writing CR3. You could say that
>> CpuFlushTlb() performs a *strict subset* of what AsmWriteCr3() performs.
>> Therefore, in order to replace AsmWriteCr3() with just CpuFlushTlb(),
>> you need to demonstrate that the functionality that now is *not* going
>> to be done has always been superfluous. In more direct terms, you need
>> to show that the AsmWriteCr3() write call that's being removed here
>> never actuall changes the *value* of CR3.
>>
>> And I cannot show that myself very easily.
>> ConvertMemoryPageToNotPresent(). In ConvertMemoryPageToNotPresent(),
>> "PageTable" is first set from AsmReadCr3(), then passed twice to
>> PageTableMap() by reference (!), and then it is written back to CR3. If
>> at least one of those PageTableMap() calls change "PageTable", then the
>> AsmWriteCr3() call at the end that's now being removed actually changes
>> the value of CR3, and then the patch would be wrong.
>>
>> It's possible that PageTableMap() never changes the value of
>> "PageTable", but it must be proved, and the evidence should be included
>> in the commit message.
>>
>> (3) Furthermore, with the patch applied, ConvertMemoryPageToNotPresent()
>> will no longer flush the TLB itself -- that will always be left to the
>> caller. This new caller responsibility should be documented in the
>> leading comment of ConvertMemoryPageToNotPresent(). We already have a
>> paragraph there starting with "Caller should make sure..."
>>
>> Sorry for making such a big deal out of this, but such simple-looking
>> changes can sometimes case obscure (and rarely occurring) bugs down the
>> road. If you already have evidence that CR3 does not change here, that's
>> great, but then please don't think it's "obvious"; just go ahead and
>> document it.
> 
> Laszlo,
> Happy to see these comments! All make sense!
> 
> PageTableMap() only modifies the PageTable root pointer when creating from 
> zero.
> Since there is only one instance of the PageTableLib, I think we could update 
> the
> PageTableLib API comments to explicitly mention that. So point (2) will be 
> resolved.

That should work, thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113597): https://edk2.groups.io/g/devel/message/113597
Mute This Topic: https://groups.io/mt/103636435/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V2] UefiCpuPkg:Limit PhysicalAddressBits in speicial case

[Laszlo Ersek]

On 1/11/24 03:11, Dun Tan wrote:
> When creating smm page table, limit maximum
> supported physical address bits returned by
> CalculateMaximumSupportAddress() to 47 if
> 5-Level Paging is disabled.
> When 5-Level Paging is disabled and the
> PhysicalAddressBits retrived from CPU HOB or
> CpuId is bigger than 47, and since virtual
> addresses are sign-extended, only [0, 2^47-1]
> range in 52-bit physical address is mapped
> in page table.
> 
> Signed-off-by: Dun Tan 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Cc: Rahul Kumar 
> Cc: Gerd Hoffmann 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)

I'll let Gerd review this (thanks!), I just want to point out a typo in
the subject: "speicial" should be "special".

Thanks
Laszlo

> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index ddd9be66b5..35c282a771 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -137,11 +137,13 @@ GetSubEntriesNum (
>  /**
>Calculate the maximum support address.
>  
> +  @param[in] Is5LevelPagingNeededIf 5-level paging enabling is needed.
> +
>@return the maximum support address.
>  **/
>  UINT8
>  CalculateMaximumSupportAddress (
> -  VOID
> +  BOOLEAN  Is5LevelPagingNeeded
>)
>  {
>UINT32  RegEax;
> @@ -164,6 +166,15 @@ CalculateMaximumSupportAddress (
>  }
>}
>  
> +  //
> +  // Only [0, 2^47 -1] in 52-bit physical addresses is mapped in page table
> +  // when 5-Level Paging is disabled.
> +  //
> +  ASSERT (PhysicalAddressBits <= 52);
> +  if (!Is5LevelPagingNeeded && (PhysicalAddressBits > 47)) {
> +PhysicalAddressBits = 47;
> +  }
> +
>return PhysicalAddressBits;
>  }
>  
> @@ -197,7 +208,7 @@ SmmInitPageTable (
>mCpuSmmRestrictedMemoryAccess = PcdGetBool 
> (PcdCpuSmmRestrictedMemoryAccess);
>m1GPageTableSupport   = Is1GPageSupport ();
>m5LevelPagingNeeded   = Is5LevelPagingNeeded ();
> -  mPhysicalAddressBits  = CalculateMaximumSupportAddress ();
> +  mPhysicalAddressBits  = CalculateMaximumSupportAddress 
> (m5LevelPagingNeeded);
>PatchInstructionX86 (gPatch5LevelPagingNeeded, m5LevelPagingNeeded, 1);
>if (m5LevelPagingNeeded) {
>  mPagingMode = m1GPageTableSupport ? Paging5Level1GB : Paging5Level;



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113596): https://edk2.groups.io/g/devel/message/113596
Mute This Topic: https://groups.io/mt/103655312/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Memory Attribute for depex section

[Laszlo Ersek]

On 1/10/24 22:50, Pedro Falcato wrote:
> On Wed, Jan 10, 2024 at 1:45 PM Laszlo Ersek 
> wrote:
>>
>> (+ Andrew)
>>
>> On 1/10/24 14:09, Laszlo Ersek wrote:
>>
>>> I think that keeping the depex section read-only is valuable, so I'd
>>> rule out #2. I'd also not start with option #1 -- copying the depex
>>> to heap memory, just so this optimization can succeed. I'd simply
>>> start by removing the optimization, and measuring how much driver
>>> dispatch slows down in practice, on various platforms.
>>>
>>> Can you try this? (I have only build-tested and "uncrustified" it.)
>>>
>>> The patch removes the EFI_DEP_REPLACE_TRUE handling altogether, plus
>>> it CONST-ifies the Iterator pointer (which points into the DEPEX
>>> section), so that the compiler catch any possible accesses at *build
>>> time* that would write to the write-protected DEPEX memory area.
>>
>> On a tangent: the optimization in question highlights a more general
>> problem, namely that the DXE (and possibly MM/SMM) protocol databases
>> are considered slow, for some access patterns.
>>
>> Edk2 implements those protocol databases with linked lists, where
>> lookup costs O(n) operations (average and worst cases). And protocol
>> lookups are quite frequent (for example, in depex evaluations, they
>> could be considered "particularly frequent").
>>
>> (1) The "Tasks" wiki page mentions something *similar* (but not the
>> same); see
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/Tasks#datahub--gcd-scalability
>>
>> The description is: "The DXE core's DataHub and GCD (Global Coherency
>> Domain) layers don't scale well as the number of data items gets
>> large, since they are based on simple linked lists. Find better data
>> structures."
>
> How large do they usually get? What's the worst case?

No idea.

>
>>
>> The same might apply more or less to the protocol database
>> implementation.
>>
>> (2) More to the point, Andrew Fish reported earlier that at Apple,
>> they had rewritten the DXE protocol database, using the red-black
>> tree OrderedCollectionLib that I had contributed previously to edk2
>> -- and they saw significant performance improvements.
>>
>> So upstreaming that feature to edk2 could be very valuable.
>> (Red-black trees have O(log(n)) time cost (worst case) for lookup,
>> insertion and deletion, and O(n) cost for iterating through the whole
>> data structure.)
>
> FWIW, can we do better than an RB tree? They're notoriously cache
> unfriendly...

Sure, if someone contributes a different data structure that is suitable
for the task :)

RB trees may be cache unfriendly, but the functionality they provide
with O(log(n)) worst case performance is amazing. You can use them as
read-write associate arrays, sorted lists supporting forward and
backward traversal (in fact tools for sorting), priority queues, etc.

When I contributed the code, edk2 didn't have any associative array
type, so something generic that would address the widest range of use
cases looked like a good idea. (And indeed the library has been well
applied in several of those use cases since, in various parts of edk2 --
for sorting, uniqueness-checking, async interface token tracking &
lookups.)

This is not an argument against a more suitable data structure of
course. Just pointing out that the RB tree has worked well thus far.
E.g., under the BZ link below, Andrew mentions a diagnostic tool that
creates 3000 handles. Looking up an element in a 3000-element list would
cost 1500 iterations on average; using a balanced binary search tree it
might cost ~11 iterations. Assuming that linked lists and linked binary
search trees are similarly cache-unfriendly, that's a ~136 factor of
improvement.

>
>>
>> Let me see if I can find the bugzilla ticket...
>>
>> Ah, I got it. Apologies, I misremembered: Andrew's comment was not
>> about the protocol database, but about the handle database. Here it
>> is:
>>
>> https://bugzilla.tianocore.org/show_bug.cgi?id=988#c7
>>
>> (the BZ is still CONFIRMED btw...)
>>
>> Still, I think it must be related in a way. Namely, an EFI handle
>> exists if and only if at least one protocol interface is installed on
>> it. If you uninstall the last protocol interface from a handle, then
>> the handle is destroyed -- in fact that's the *only* way to destroy a
>> handle, to my understanding. See
>> EFI_BOOT_SERVICES.UninstallProtocolInterface() in the UEFI spec: "If
>> the last protocol interface is removed from a handle, the handle is
>> freed and is no longer valid". Furthermore, calling
>> InstallProtocolInterface() and InstallMultipleProtocolInterfaces() is
>> how one *creates* new handles.
>>
>> So given how handles and protocol interfaces are conceptually
>> interlinked, an rbtree-based protocol DB might have to maintain
>> multiple rbtrees internally (for the ability to search the database
>> quickly with different types of "keys"). I don't have a design ready
>> in my mind at all (I'm not that familiar with the *current*,
>> 

Re: [edk2-devel] [PATCH v1 0/4] Bz4166: Integer Overflow in CreateHob()

[Ard Biesheuvel]

On Thu, 11 Jan 2024 at 09:35, Guo, Gua  wrote:
>
> CC: @Mathews, John and @Zimmer, Vincent
>
> Hi @Gerd Hoffmann
>
> My company teammate share me your patch can resolved 
> https://bugzilla.tianocore.org/show_bug.cgi?id=4166. So the signed-off name 
> is your name.
>

Again, a signed-off-by line is *not* a statement of authorship. You
*cannot* add it on someone else's behalf if you want to credit the
author.

A signed-off-by line is a statement by the contributor of the code to
indicate that the contributed code is made available under conditions
that are in agreement with the open source license of the project.

If you want to credit the author, you can mention their name in the
commit log, or add some other tag (authored-by, for example).

If you want to contribute code by another author, and you know you are
able to do so under the terms, you should indicate so by adding your
own signed-off line to the patch.

Thanks,
Ard.

> If you have any concern, you can also share for me, if you don't have concern 
> please also let me know, before merging it.
>
> It's PR https://github.com/tianocore/edk2/pull/5252/
>
> Thanks,
> Gua
> -Original Message-
> From: devel@edk2.groups.io  On Behalf Of Guo, Gua
> Sent: Thursday, January 11, 2024 1:15 PM
> To: devel@edk2.groups.io
> Cc: Guo, Gua 
> Subject: [edk2-devel] [PATCH v1 0/4] Bz4166: Integer Overflow in CreateHob()
>
> From: Gua Guo 
>
> Fix Integer Overflow for CVE-2022-36765
> 1. UefiPayloadPkg/Hob: Integer Overflow in CreateHob() 2. 
> StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 3. EmbeddedPkg/Hob: 
> Integer Overflow in CreateHob() 4. MdeModulePkg/Hob: Integer Overflow in 
> CreateHob()
>
>
> Gerd Hoffmann (4):
>   UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
>   StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
>   EmbeddedPkg/Hob: Integer Overflow in CreateHob()
>   MdeModulePkg/Hob: Integer Overflow in CreateHob()
>
>  EmbeddedPkg/Library/PrePiHobLib/Hob.c   | 6 ++
>  MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
>  .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 6 ++
>  UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c | 6 ++
>  4 files changed, 19 insertions(+), 1 deletion(-)
>
> --
> 2.39.2.windows.1
>
>
>
>
>
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113594): https://edk2.groups.io/g/devel/message/113594
Mute This Topic: https://groups.io/mt/103657270/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Bz4166: Integer Overflow in CreateHob()

[Guo, Gua]

CC: @Mathews, John and @Zimmer, Vincent

Hi @Gerd Hoffmann

My company teammate share me your patch can resolved 
https://bugzilla.tianocore.org/show_bug.cgi?id=4166. So the signed-off name is 
your name. 

If you have any concern, you can also share for me, if you don't have concern 
please also let me know, before merging it.

It's PR https://github.com/tianocore/edk2/pull/5252/

Thanks,
Gua
-Original Message-
From: devel@edk2.groups.io  On Behalf Of Guo, Gua
Sent: Thursday, January 11, 2024 1:15 PM
To: devel@edk2.groups.io
Cc: Guo, Gua 
Subject: [edk2-devel] [PATCH v1 0/4] Bz4166: Integer Overflow in CreateHob()

From: Gua Guo 

Fix Integer Overflow for CVE-2022-36765
1. UefiPayloadPkg/Hob: Integer Overflow in CreateHob() 2. StandaloneMmPkg/Hob: 
Integer Overflow in CreateHob() 3. EmbeddedPkg/Hob: Integer Overflow in 
CreateHob() 4. MdeModulePkg/Hob: Integer Overflow in CreateHob()


Gerd Hoffmann (4):
  UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
  StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
  EmbeddedPkg/Hob: Integer Overflow in CreateHob()
  MdeModulePkg/Hob: Integer Overflow in CreateHob()

 EmbeddedPkg/Library/PrePiHobLib/Hob.c   | 6 ++
 MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
 .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 6 ++
 UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c | 6 ++
 4 files changed, 19 insertions(+), 1 deletion(-)

--
2.39.2.windows.1








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113593): https://edk2.groups.io/g/devel/message/113593
Mute This Topic: https://groups.io/mt/103657270/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-