[edk2-devel] [PATCH 0/2] Add DECLARE_LENGTH opcode of dependency expression

[edk2-devel] [PATCH 0/2] Add DECLARE_LENGTH opcode of dependency expression

[Li, Yi]

Mantis: https://mantis.uefi.org/mantis/view.php?id=2025

To avoid messy parsing of the Depex section of a Capsule, it would
be a lot easier for everyone involved if we preceded the Capsule Depex
Section with a length declaration. It provides simple bounds checking
to avoid having to parse the op-codes, but in the case of a malformed
depex being parsed, avoid other issues which can be messy.

REF: UEFI spec 2.10 Table 23.4
Syntax
DECLARE_LENGTH <32-bit Length>
Description
Declares an 32-bit byte length of the entire dependency expression.
Behaviors and Restrictions
This opcode must be the first one in a dependency expression.

Yi Li (2):
  MdePkg: Add DECLARE_LENGTH opcode of dependency expression
  FmpDevicePkg: Add DECLARE_LENGTH opcode of dependency expression

 .../FmpDependencyLib/FmpDependencyLib.c       | 35 ++++++++
 .../PrivateInclude/FmpLastAttemptStatus.h     |  3 +
 .../EvaluateDependencyUnitTest.c              | 84 ++++++++++++++++---
 MdePkg/Include/Protocol/FirmwareManagement.h  | 29 +++----
 4 files changed, 125 insertions(+), 26 deletions(-)

-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112686): https://edk2.groups.io/g/devel/message/112686
Mute This Topic: https://groups.io/mt/103257906/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] MdePkg: Add DECLARE_LENGTH opcode of dependency expression

[Li, Yi]

Mantis: https://mantis.uefi.org/mantis/view.php?id=2025

To avoid messy parsing of the Depex section of a Capsule, it would
be a lot easier for everyone involved if we preceded the Capsule Depex
Section with a length declaration. It provides simple bounds checking
to avoid having to parse the op-codes, but in the case of a malformed
depex being parsed, avoid other issues which can be messy.

REF: UEFI spec 2.10 Table 23.4

Signed-off-by: Yi Li <yi1.li@intel.com>

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/Protocol/FirmwareManagement.h | 29 ++++++++++----------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/MdePkg/Include/Protocol/FirmwareManagement.h b/MdePkg/Include/Protocol/FirmwareManagement.h
index f37067df3455..e535bb697d88 100644
--- a/MdePkg/Include/Protocol/FirmwareManagement.h
+++ b/MdePkg/Include/Protocol/FirmwareManagement.h
@@ -30,20 +30,21 @@ typedef struct _EFI_FIRMWARE_MANAGEMENT_PROTOCOL EFI_FIRMWARE_MANAGEMENT_PROTOCO
 ///
 /// Dependency Expression Opcode
 ///
-#define EFI_FMP_DEP_PUSH_GUID     0x00
-#define EFI_FMP_DEP_PUSH_VERSION  0x01
-#define EFI_FMP_DEP_VERSION_STR   0x02
-#define EFI_FMP_DEP_AND           0x03
-#define EFI_FMP_DEP_OR            0x04
-#define EFI_FMP_DEP_NOT           0x05
-#define EFI_FMP_DEP_TRUE          0x06
-#define EFI_FMP_DEP_FALSE         0x07
-#define EFI_FMP_DEP_EQ            0x08
-#define EFI_FMP_DEP_GT            0x09
-#define EFI_FMP_DEP_GTE           0x0A
-#define EFI_FMP_DEP_LT            0x0B
-#define EFI_FMP_DEP_LTE           0x0C
-#define EFI_FMP_DEP_END           0x0D
+#define EFI_FMP_DEP_PUSH_GUID       0x00
+#define EFI_FMP_DEP_PUSH_VERSION    0x01
+#define EFI_FMP_DEP_VERSION_STR     0x02
+#define EFI_FMP_DEP_AND             0x03
+#define EFI_FMP_DEP_OR              0x04
+#define EFI_FMP_DEP_NOT             0x05
+#define EFI_FMP_DEP_TRUE            0x06
+#define EFI_FMP_DEP_FALSE           0x07
+#define EFI_FMP_DEP_EQ              0x08
+#define EFI_FMP_DEP_GT              0x09
+#define EFI_FMP_DEP_GTE             0x0A
+#define EFI_FMP_DEP_LT              0x0B
+#define EFI_FMP_DEP_LTE             0x0C
+#define EFI_FMP_DEP_END             0x0D
+#define EFI_FMP_DEP_DECLARE_LENGTH  0x0E
 
 ///
 /// Image Attribute - Dependency
-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112687): https://edk2.groups.io/g/devel/message/112687
Mute This Topic: https://groups.io/mt/103257908/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] FmpDevicePkg: Add DECLARE_LENGTH opcode of dependency expression

[Li, Yi]

Mantis: https://mantis.uefi.org/mantis/view.php?id=2025

To avoid messy parsing of the Depex section of a Capsule, it would
be a lot easier for everyone involved if we preceded the Capsule Depex
Section with a length declaration. It provides simple bounds checking
to avoid having to parse the op-codes, but in the case of a malformed
depex being parsed, avoid other issues which can be messy.

REF: UEFI spec 2.10 Table 23.4

Signed-off-by: Yi Li <yi1.li@intel.com>

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
---
 .../FmpDependencyLib/FmpDependencyLib.c       | 35 ++++++++
 .../PrivateInclude/FmpLastAttemptStatus.h     |  3 +
 .../EvaluateDependencyUnitTest.c              | 84 ++++++++++++++++---
 3 files changed, 110 insertions(+), 12 deletions(-)

diff --git a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
index 76a1ee3f40db..50662e74e065 100644
--- a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
@@ -234,6 +234,7 @@ EvaluateDependency (
   GUID           ImageTypeId;
   UINT32         Version;
   UINT32         LocalLastAttemptStatus;
+  UINT32         DeclaredLength;
 
   LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
 
@@ -489,6 +490,37 @@ EvaluateDependency (
         }
 
         return Element1.Value.Boolean;
+      case EFI_FMP_DEP_DECLARE_LENGTH:
+        if (Iterator + sizeof (UINT32) >= (UINT8 *)Dependencies->Dependencies + DependenciesSize ) {
+          DEBUG ((DEBUG_ERROR, "EvaluateDependency: DECLARE_LENGTH extends beyond end of dependency expression!\n"));
+          LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_BEYOND_DEPEX;
+          goto Error;
+        }
+
+        //
+        // This opcode must be the first one in a dependency expression.
+        //
+        if (Iterator != Dependencies->Dependencies) {
+          DEBUG ((DEBUG_ERROR, "EvaluateDependency: DECLARE_LENGTH is not the first opcode!\n"));
+          LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_NOT_FIRST_OPCODE;
+          goto Error;
+        }
+
+        DeclaredLength = *(UINT32 *)(Iterator + 1);
+        if (DeclaredLength != DependenciesSize) {
+          DEBUG ((DEBUG_ERROR, "EvaluateDependency: DECLARE_LENGTH is not equal to length of dependency expression!\n"));
+          LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_INCORRECT;
+          goto Error;
+        }
+
+        Status = Push (DeclaredLength, VersionType);
+        if (EFI_ERROR (Status)) {
+          LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_PUSH_FAILURE;
+          goto Error;
+        }
+
+        Iterator = Iterator + sizeof (UINT32);
+        break;
       default:
         DEBUG ((DEBUG_ERROR, "EvaluateDependency: Unknown Opcode - %02x!\n", *Iterator));
         LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_UNKNOWN_OPCODE;
@@ -574,6 +606,9 @@ ValidateDependency (
         }
 
         return TRUE;
+      case EFI_FMP_DEP_DECLARE_LENGTH:
+        Depex += sizeof (UINT32) + 1;
+        break;
       default:
         return FALSE;
     }
diff --git a/FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h b/FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
index 39a55dd2c643..aaa3334909c8 100644
--- a/FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
+++ b/FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
@@ -66,6 +66,9 @@ enum LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST {
   LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_FMP_NOT_FOUND,
   LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_PUSH_FAILURE,
   LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_POP_FAILURE,
+  LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_NOT_FIRST_OPCODE,
+  LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_BEYOND_DEPEX,
+  LAST_ATTEMPT_STATUS_DEPENDENCY_LIB_ERROR_DECLARE_LENGTH_INCORRECT,
 
   ///
   /// Last attempt status codes used in FmpDependencyCheckLib
diff --git a/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c b/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
index 0edb7f67306f..352887af2c5e 100644
--- a/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
+++ b/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
@@ -125,19 +125,75 @@ static UINT8  mExpression11[] = {
   EFI_FMP_DEP_END
 };
 
+// Valid Dependency Expression 7: With correct declared length
+static UINT8  mExpression12[] = {
+  EFI_FMP_DEP_DECLARE_LENGTH, 0x35, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_VERSION,   0x01, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0xFA, 0x4D, 0x14, 0x97,0x8E,  0xEB, 0x4D, 0xD1, 0x8B, 0x4D, 0x39, 0x88, 0x24, 0x96, 0x56, 0x42,
+  EFI_FMP_DEP_GT,
+  EFI_FMP_DEP_PUSH_VERSION,   0x03, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0x70, 0x73, 0x2A, 0xA4,0x3A,  0x43, 0x4D, 0x68, 0x9A, 0xA1, 0xDE, 0x62, 0x23, 0x30, 0x6C, 0xF3,
+  EFI_FMP_DEP_GTE,
+  EFI_FMP_DEP_AND,
+  EFI_FMP_DEP_END
+};
+
+// Valid Dependency Expression 7: With longer declared length
+static UINT8  mExpression13[] = {
+  EFI_FMP_DEP_DECLARE_LENGTH, 0x3B, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_VERSION,   0x01, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0xFA, 0x4D, 0x14, 0x97,0x8E,  0xEB, 0x4D, 0xD1, 0x8B, 0x4D, 0x39, 0x88, 0x24, 0x96, 0x56, 0x42,
+  EFI_FMP_DEP_GT,
+  EFI_FMP_DEP_PUSH_VERSION,   0x03, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0x70, 0x73, 0x2A, 0xA4,0x3A,  0x43, 0x4D, 0x68, 0x9A, 0xA1, 0xDE, 0x62, 0x23, 0x30, 0x6C, 0xF3,
+  EFI_FMP_DEP_GTE,
+  EFI_FMP_DEP_AND,
+  EFI_FMP_DEP_END
+};
+
+// Valid Dependency Expression 7: With shorter declared length
+static UINT8  mExpression14[] = {
+  EFI_FMP_DEP_DECLARE_LENGTH, 0x1B, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_VERSION,   0x01, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0xFA, 0x4D, 0x14, 0x97,0x8E,  0xEB, 0x4D, 0xD1, 0x8B, 0x4D, 0x39, 0x88, 0x24, 0x96, 0x56, 0x42,
+  EFI_FMP_DEP_GT,
+  EFI_FMP_DEP_PUSH_VERSION,   0x03, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0x70, 0x73, 0x2A, 0xA4,0x3A,  0x43, 0x4D, 0x68, 0x9A, 0xA1, 0xDE, 0x62, 0x23, 0x30, 0x6C, 0xF3,
+  EFI_FMP_DEP_GTE,
+  EFI_FMP_DEP_AND,
+  EFI_FMP_DEP_END
+};
+
+// Valid Dependency Expression 7: DECLARE_LENGTH opcode is not first one
+static UINT8  mExpression15[] = {
+  EFI_FMP_DEP_PUSH_VERSION,   0x01, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0xFA, 0x4D, 0x14, 0x97,0x8E,  0xEB, 0x4D, 0xD1, 0x8B, 0x4D, 0x39, 0x88, 0x24, 0x96, 0x56, 0x42,
+  EFI_FMP_DEP_GT,
+  EFI_FMP_DEP_DECLARE_LENGTH, 0x35, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_VERSION,   0x03, 0x00, 0x00, 0x00,
+  EFI_FMP_DEP_PUSH_GUID,      0x70, 0x73, 0x2A, 0xA4,0x3A,  0x43, 0x4D, 0x68, 0x9A, 0xA1, 0xDE, 0x62, 0x23, 0x30, 0x6C, 0xF3,
+  EFI_FMP_DEP_GTE,
+  EFI_FMP_DEP_AND,
+  EFI_FMP_DEP_END
+};
+
 // ------------------------------------------------Test Depex------Depex Size----------------Expected Result
-static BASIC_TEST_CONTEXT  mBasicTestTrue1    = { mExpression1, sizeof (mExpression1), TRUE };
-static BASIC_TEST_CONTEXT  mBasicTestTrue2    = { mExpression2, sizeof (mExpression2), TRUE };
-static BASIC_TEST_CONTEXT  mBasicTestFalse1   = { mExpression3, sizeof (mExpression3), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestFalse2   = { mExpression4, sizeof (mExpression4), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid1 = { mExpression1, sizeof (mExpression1) - 1, FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid2 = { mExpression5, sizeof (mExpression5), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid3 = { mExpression6, sizeof (mExpression6), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid4 = { mExpression7, sizeof (mExpression7), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid5 = { mExpression8, sizeof (mExpression8), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid6 = { mExpression9, sizeof (mExpression9), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid7 = { mExpression10, sizeof (mExpression10), FALSE };
-static BASIC_TEST_CONTEXT  mBasicTestInvalid8 = { mExpression11, sizeof (mExpression11), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestTrue1     = { mExpression1, sizeof (mExpression1), TRUE };
+static BASIC_TEST_CONTEXT  mBasicTestTrue2     = { mExpression2, sizeof (mExpression2), TRUE };
+static BASIC_TEST_CONTEXT  mBasicTestFalse1    = { mExpression3, sizeof (mExpression3), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestFalse2    = { mExpression4, sizeof (mExpression4), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid1  = { mExpression1, sizeof (mExpression1) - 1, FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid2  = { mExpression5, sizeof (mExpression5), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid3  = { mExpression6, sizeof (mExpression6), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid4  = { mExpression7, sizeof (mExpression7), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid5  = { mExpression8, sizeof (mExpression8), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid6  = { mExpression9, sizeof (mExpression9), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid7  = { mExpression10, sizeof (mExpression10), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid8  = { mExpression11, sizeof (mExpression11), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestValid1    = { mExpression12, sizeof (mExpression12), TRUE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid9  = { mExpression13, sizeof (mExpression13), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid10 = { mExpression14, sizeof (mExpression14), FALSE };
+static BASIC_TEST_CONTEXT  mBasicTestInvalid11 = { mExpression15, sizeof (mExpression15), FALSE };
 
 /**
   Unit test for EvaluateDependency() API of the FmpDependencyLib.
@@ -233,6 +289,10 @@ UnitTestingEntry (
   AddTestCase (DepexEvalTests, "Error: Operand and operator mismatch", "Test10", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid6);
   AddTestCase (DepexEvalTests, "Error: GUID is NOT FOUND", "Test11", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid7);
   AddTestCase (DepexEvalTests, "Error: Stack Underflow", "Test12", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid8);
+  AddTestCase (DepexEvalTests, "Evaluate to True - 3", "Test13", EvaluateDependencyTest, NULL, NULL, &mBasicTestValid1);
+  AddTestCase (DepexEvalTests, "Error: Declared length too long", "Test14", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid9);
+  AddTestCase (DepexEvalTests, "Error: Declared length too short", "Test15", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid10);
+  AddTestCase (DepexEvalTests, "Error: DECLARE_LENGTH is not first opcode", "Test16", EvaluateDependencyTest, NULL, NULL, &mBasicTestInvalid11);
 
   //
   // Execute the tests.
-- 
2.42.0.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112688): https://edk2.groups.io/g/devel/message/112688
Mute This Topic: https://groups.io/mt/103257910/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-