public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: "Dandan Bi" <dandan.bi@intel.com>,
	"Hao A Wu" <hao.a.wu@intel.com>,
	"Jian J Wang" <jian.j.wang@intel.com>,
	"Liming Gao" <gaoliming@byosoft.com.cn>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: [PATCH v2 RESEND 1/2] MdeModulePkg/Core/Dxe: assert SectionInstance invariant in FindChildNode()
Date: Thu, 19 Nov 2020 11:53:39 +0100	[thread overview]
Message-ID: <20201119105340.16225-2-lersek@redhat.com> (raw)
In-Reply-To: <20201119105340.16225-1-lersek@redhat.com>

FindChildNode() has two callers: GetSection(), and FindChildNode() itself.

- At the GetSection() call site, a positive (i.e., nonzero)
  SectionInstance is passed. This is because GetSection() takes a
  zero-based (UINTN) SectionInstance, and then passes
  Instance=(SectionInstance+1) to FindChildNode().

- For reaching the recursive FindChildNode() call site, a section type
  mismatch, or a section instance mismatch, is necessary. This means,
  respectively, that SectionInstance will either not have been decreased,
  or not to zero anyway, at the recursive FindChildNode() call site.

Add two ASSERT()s to FindChildNode(), for expressing the (SectionSize>0)
invariant.

In turn, the invariant provides the explanation why, after the recursive
call, a zero SectionInstance implies success. Capture it in a comment.

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c | 23 +++++++++++++++-----
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c b/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
index d678166db475..d7f7ef427422 100644
--- a/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
+++ b/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
@@ -952,8 +952,8 @@ CreateChildNode (
                                  search.
   @param  SearchType             Indicates the type of section to search for.
   @param  SectionInstance        Indicates which instance of section to find.
-                                 This is an in/out parameter to deal with
-                                 recursions.
+                                 This is an in/out parameter and it is 1-based,
+                                 to deal with recursions.
   @param  SectionDefinitionGuid  Guid of section definition
   @param  FoundChild             Output indicating the child node that is found.
   @param  FoundStream            Output indicating which section stream the child
@@ -988,6 +988,8 @@ FindChildNode (
   EFI_STATUS                                    ErrorStatus;
   EFI_STATUS                                    Status;
 
+  ASSERT (*SectionInstance > 0);
+
   CurrentChildNode = NULL;
   ErrorStatus = EFI_NOT_FOUND;
 
@@ -1037,6 +1039,11 @@ FindChildNode (
       }
     }
 
+    //
+    // Type mismatch, or we haven't found the desired instance yet.
+    //
+    ASSERT (*SectionInstance > 0);
+
     if (CurrentChildNode->EncapsulatedStreamHandle != NULL_STREAM_HANDLE) {
       //
       // If the current node is an encapsulating node, recurse into it...
@@ -1050,16 +1057,20 @@ FindChildNode (
                 &RecursedFoundStream,
                 AuthenticationStatus
                 );
-      //
-      // If the status is not EFI_SUCCESS, just save the error code and continue
-      // to find the request child node in the rest stream.
-      //
       if (*SectionInstance == 0) {
+        //
+        // The recursive FindChildNode() call decreased (*SectionInstance) to
+        // zero.
+        //
         ASSERT_EFI_ERROR (Status);
         *FoundChild = RecursedChildNode;
         *FoundStream = RecursedFoundStream;
         return EFI_SUCCESS;
       } else {
+        //
+        // If the status is not EFI_SUCCESS, just save the error code and
+        // continue to find the request child node in the rest stream.
+        //
         ErrorStatus = Status;
       }
     } else if ((CurrentChildNode->Type == EFI_SECTION_GUID_DEFINED) && (SearchType != EFI_SECTION_GUID_DEFINED)) {
-- 
2.19.1.3.g30247aa5d201



  reply	other threads:[~2020-11-19 10:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 10:53 [PATCH v2 RESEND 0/2] security fix: unlimited FV recursion, round 2 (DXE Core) Laszlo Ersek
2020-11-19 10:53 ` Laszlo Ersek [this message]
2020-11-19 10:53 ` [PATCH v2 RESEND 2/2] MdeModulePkg/Core/Dxe: limit FwVol encapsulation section recursion Laszlo Ersek
2020-11-20  5:30 ` 回复: [edk2-devel] [PATCH v2 RESEND 0/2] security fix: unlimited FV recursion, round 2 (DXE Core) gaoliming
2020-11-20 10:57   ` Laszlo Ersek
2020-11-21  1:43   ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201119105340.16225-2-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox