Re: [PATCH V4] MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol

[Laszlo Ersek <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

On 06/23/17 12:09, Amit Kumar wrote:
> Change since v3:
> 1) Fixed issue when Attributes = EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> and Inteface = NULL case. [Reported by:star.zeng at intel.com]
> 
> Change Since v2:
> 1) Modified to use EFI_ERROR to get status code
> 
> Change since v1:
> 1) Fixed typo protocal to protocol
> 2) Fixed coding style
> 
> Modified source code to update Interface as per spec.
> 1) In case of Protocol is un-supported, interface should be returned NULL.
> 2) In case of any error, interface should not be modified.
> 3) In case of Test Protocol, interface is optional.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Amit Kumar <amit.ak@samsung.com>
> ---
>  MdeModulePkg/Core/Dxe/Hand/Handle.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

This patch breaks *at least* the following drivers:
- IntelFrameworkModulePkg/.../IsaBusDxe
- IntelFrameworkModulePkg/.../IsaSerialDxe
- MdeModulePkg/.../TerminalDxe
- MdeModulePkg/.../ScsiBusDxe

I have patches for the first three. I'm too tired to complete the patch
for the fourth module (and any modules that might still be affected, but
I'd only see those in the OVMF boot process if I got past the ScsiBusDxe
problem.)

The issue was reported by Gabriel here:

https://lists.01.org/pipermail/edk2-devel/2017-June/011886.html

although he didn't get past of the first driver, of course.

Now, what this patch does is valid, as far as the UEFI spec is
concerned. And the above (now broken) drivers are all incorrect to
assume that EFI_ALREADY_STARTED guarantees that OpenProtocol() has
filled in Interface on output.

However, such an intrusive fix must be accompanied by extensive testing;
preferably using one of the in-tree emulation platforms, such as OvmfPkg
running on QEMU. And, when testing uncovers the failures in those
drivers, patches should be submitted to fix those drivers *first*, and
then the patch for OpenProtocol() should be presented *last*.

I'm attaching the first three patches I have now. As I said I'm too
tired to work on ScsiBusDxe (the SCSIBusDriverBindingStart() function
has the same bug around EFI_ALREADY_STARTED). Which is why I'm not
posting the first three patches normally, with git-send-email; the work
is incomplete.

Honestly, I'm thinking that commit 45cfcd8dccf8 should be reverted now,
then all the drivers used by OvmfPkg should be fixed up (not by me,
obviously!), and once OVMF boots again, we can re-commit (= cherry-pick)
commit 45cfcd8dccf8.

Thanks
Laszlo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-IntelFrameworkModulePkg-IsaBusDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0001-IntelFrameworkModulePkg-IsaBusDxe-fix-OpenProtocol-usage.patch", Size: 5181 bytes --]

From 0726d69fa17396cfdf77db262dcfcac27aa8c848 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 1/3] IntelFrameworkModulePkg/IsaBusDxe: fix OpenProtocol()
 usage

The IsaBusControllerDriverStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
IsaBusControllerDriverStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and IsaAcpi have uninitialized (indeterminate) values,
leading to crashes.

Restore the previous behavior of IsaBusControllerDriverStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: "Gabriel L. Somlo" <gsomlo@gmail.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c | 51 +++++++++++++++++-----
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c b/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
index 1312f260f9ae..7d996cf86eb1 100644
--- a/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
+++ b/IntelFrameworkModulePkg/Bus/Isa/IsaBusDxe/IsaBus.c
@@ -252,62 +252,91 @@ IsaBusControllerDriverStart (
                   Controller,
                   EFI_OPEN_PROTOCOL_GET_PROTOCOL
                   );
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
   //
   // Open Device Path Protocol
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
-    return Status;
+  if (EFI_ERROR (Status)) {
+    if  (Status != EFI_ALREADY_STARTED) {
+      return Status;
+    }
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
 
   //
   // Open ISA Acpi Protocol
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaAcpiProtocolGuid,
                   (VOID **) &IsaAcpi,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
+  if (EFI_ERROR (Status)) {
+    if (Status != EFI_ALREADY_STARTED) {
+      //
+      // Close opened protocol. (This is required after BY_DRIVER, and not
+      // required but permitted after GET_PROTOCOL.)
+      //
+      gBS->CloseProtocol (
+             Controller,
+             &gEfiDevicePathProtocolGuid,
+             This->DriverBindingHandle,
+             Controller
+             );
+      return Status;
+    }
     //
-    // Close opened protocol
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
     //
-    gBS->CloseProtocol (
-           Controller,
-           &gEfiDevicePathProtocolGuid,
-           This->DriverBindingHandle,
-           Controller
-           );
-    return Status;
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiIsaAcpiProtocolGuid,
+                    (VOID **) &IsaAcpi,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
   //
   // The IsaBus driver will use memory below 16M, which is not tested yet,
   // so call CompatibleRangeTest to test them. Since memory below 1M should
   // be reserved to CSM, and 15M~16M might be reserved for Isa hole, test 1M
   // ~15M here
   //
   Status = gBS->LocateProtocol (
                   &gEfiGenericMemTestProtocolGuid,
                   NULL,
                   (VOID **) &GenMemoryTest
                   );
 
   if (!EFI_ERROR (Status)) {
     Status = GenMemoryTest->CompatibleRangeTest (
                               GenMemoryTest,
                               0x100000,
                               0xE00000
-- 
2.13.1.3.g8be5a757fa67


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-IntelFrameworkModulePkg-IsaSerialDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0002-IntelFrameworkModulePkg-IsaSerialDxe-fix-OpenProtocol-usage.patch", Size: 6480 bytes --]

From cf82bd00e6253ebe61b846e263fdc22b11d12dc3 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 2/3] IntelFrameworkModulePkg/IsaSerialDxe: fix OpenProtocol()
 usage

The SerialControllerDriverStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
SerialControllerDriverStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and IsaIo have uninitialized (indeterminate) values,
leading to a failed assertion later.

Restore the previous behavior of SerialControllerDriverStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 .../Bus/Isa/IsaSerialDxe/Serial.c                  | 36 ++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c b/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
index 57ee669d1406..48836f6fee92 100644
--- a/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
+++ b/IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c
@@ -409,73 +409,100 @@ SerialControllerDriverStart (
   UART_DEVICE_PATH                    *Uart;
   UINT32                              FlowControlMap;
   UART_FLOW_CONTROL_DEVICE_PATH       *FlowControl;
   EFI_DEVICE_PATH_PROTOCOL            *TempDevicePath;
   UINT32                              Control;
 
   SerialDevice = NULL;
   //
   // Get the Parent Device Path
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
-  if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
-    return Status;
+  if (EFI_ERROR (Status)) {
+    if (Status != EFI_ALREADY_STARTED) {
+      return Status;
+    }
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
   }
   //
   // Report status code enable the serial
   //
   REPORT_STATUS_CODE_WITH_DEVICE_PATH (
     EFI_PROGRESS_CODE,
     EFI_P_PC_ENABLE | EFI_PERIPHERAL_SERIAL_PORT,
     ParentDevicePath
     );
 
   //
   // Grab the IO abstraction we need to get any work done
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaIoProtocolGuid,
                   (VOID **) &IsaIo,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   if (EFI_ERROR (Status) && Status != EFI_ALREADY_STARTED) {
     goto Error;
   }
 
   if (Status == EFI_ALREADY_STARTED) {
 
     if (RemainingDevicePath == NULL || IsDevicePathEnd (RemainingDevicePath)) {
       //
       // If RemainingDevicePath is NULL or is the End of Device Path Node
       //
       return EFI_SUCCESS;
     }
 
     //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiIsaIoProtocolGuid,
+                    (VOID **) &IsaIo,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+
+    //
     // Make sure a child handle does not already exist.  This driver can only
     // produce one child per serial port.
     //
     Status = gBS->OpenProtocolInformation (
                     Controller,
                     &gEfiIsaIoProtocolGuid,
                     &OpenInfoBuffer,
                     &EntryCount
                     );
     if (EFI_ERROR (Status)) {
       return Status;
     }
 
     Status = EFI_ALREADY_STARTED;
     for (Index = 0; Index < EntryCount; Index++) {
       if ((OpenInfoBuffer[Index].Attributes & EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER) != 0) {
         Status = gBS->OpenProtocol (
                         OpenInfoBuffer[Index].ControllerHandle,
@@ -659,36 +686,41 @@ SerialControllerDriverStart (
                   );
   if (EFI_ERROR (Status)) {
     goto Error;
   }
   //
   // Open For Child Device
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiIsaIoProtocolGuid,
                   (VOID **) &IsaIo,
                   This->DriverBindingHandle,
                   SerialDevice->Handle,
                   EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
                   );
 
 Error:
   if (EFI_ERROR (Status)) {
+    //
+    // Closing the protocol interfaces opened with BY_DRIVER is required.
+    // Closing the protocol interfaces opened with GET_PROTOCOL is not required
+    // but permitted.
+    //
     gBS->CloseProtocol (
            Controller,
            &gEfiDevicePathProtocolGuid,
            This->DriverBindingHandle,
            Controller
            );
     gBS->CloseProtocol (
            Controller,
            &gEfiIsaIoProtocolGuid,
            This->DriverBindingHandle,
            Controller
            );
     if (SerialDevice != NULL) {
       if (SerialDevice->DevicePath != NULL) {
         gBS->FreePool (SerialDevice->DevicePath);
       }
 
       FreeUnicodeStringTable (SerialDevice->ControllerNameTable);
-- 
2.13.1.3.g8be5a757fa67


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-MdeModulePkg-TerminalDxe-fix-OpenProtocol-usage.patch --]
[-- Type: text/x-patch; name="0003-MdeModulePkg-TerminalDxe-fix-OpenProtocol-usage.patch", Size: 6776 bytes --]

From 899c1020de7d1239891d61e15a034aa1b13a8381 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Tue, 27 Jun 2017 01:34:25 +0200
Subject: [PATCH 3/3] MdeModulePkg/TerminalDxe: fix OpenProtocol() usage

The TerminalDriverBindingStart() function exploits the previous,
non-standard behavior of edk2 that OpenProtocol() outputs the valid
protocol Interface even when OpenProtocol() returns EFI_ALREADY_STARTED.

This behavior has been deleted with commit 45cfcd8dccf8
("MdeModulePkg/DxeCore: Fixed Interface returned by CoreOpenProtocol",
2017-06-23); Interface is no longer set on error (which conforms to the
UEFI spec). As a result, when the OpenProtocol() calls in
TerminalDriverBindingStart() fail with EFI_ALREADY_STARTED,
ParentDevicePath and SerialIo have uninitialized (indeterminate) values,
leading to a failed assertion later.

Restore the previous behavior of TerminalDriverBindingStart() by
explicitly getting the protocol interfaces with
EFI_OPEN_PROTOCOL_GET_PROTOCOL.

Cc: "Gabriel L. Somlo" <gsomlo@gmail.com>
Cc: Amit Kumar <amit.ak@samsung.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 .../Universal/Console/TerminalDxe/Terminal.c       | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
index c90879b0dd87..16fe6de21f1e 100644
--- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
+++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c
@@ -508,36 +508,50 @@ TerminalDriverBindingStart (
   UINTN                               EntryCount;
   UINTN                               Index;
   EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL     *SimpleTextOutput;
   EFI_SIMPLE_TEXT_INPUT_PROTOCOL      *SimpleTextInput;
   EFI_UNICODE_STRING_TABLE            *ControllerNameTable;
 
   //
   // Get the Device Path Protocol to build the device path of the child device
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   (VOID **) &ParentDevicePath,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
+  if (Status == EFI_ALREADY_STARTED) {
+    //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiDevicePathProtocolGuid,
+                    (VOID **) &ParentDevicePath,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+  }
 
   //
   // Open the Serial I/O Protocol BY_DRIVER.  It might already be started.
   //
   Status = gBS->OpenProtocol (
                   Controller,
                   &gEfiSerialIoProtocolGuid,
                   (VOID **) &SerialIo,
                   This->DriverBindingHandle,
                   Controller,
                   EFI_OPEN_PROTOCOL_BY_DRIVER
                   );
   ASSERT ((Status == EFI_SUCCESS) || (Status == EFI_ALREADY_STARTED));
 
   if (!IsHotPlugDevice (ParentDevicePath)) {
     //
     // if the serial device is a hot plug device, do not update the
     // ConInDev, ConOutDev, and StdErrDev variables.
@@ -549,36 +563,49 @@ TerminalDriverBindingStart (
 
   //
   // Do not create any child for END remaining device path.
   //
   if ((RemainingDevicePath != NULL) && IsDevicePathEnd (RemainingDevicePath)) {
     return EFI_SUCCESS;
   }
 
   if (Status == EFI_ALREADY_STARTED) {
 
     if (RemainingDevicePath == NULL) {
       //
       // If RemainingDevicePath is NULL or is the End of Device Path Node
       //
       return EFI_SUCCESS;
     }
 
     //
+    // Repeat the above OpenProtocol() call with GET_PROTOCOL.
+    //
+    Status = gBS->OpenProtocol (
+                    Controller,
+                    &gEfiSerialIoProtocolGuid,
+                    (VOID **) &SerialIo,
+                    This->DriverBindingHandle,
+                    Controller,
+                    EFI_OPEN_PROTOCOL_GET_PROTOCOL
+                    );
+    ASSERT_EFI_ERROR (Status);
+
+    //
     // This driver can only produce one child per serial port.
     // Change its terminal type as remaining device path requests.
     //
     Status = gBS->OpenProtocolInformation (
                     Controller,
                     &gEfiSerialIoProtocolGuid,
                     &OpenInfoBuffer,
                     &EntryCount
                     );
     if (!EFI_ERROR (Status)) {
       Status = EFI_NOT_FOUND;
       for (Index = 0; Index < EntryCount; Index++) {
         if ((OpenInfoBuffer[Index].Attributes & EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER) != 0) {
           Status = gBS->OpenProtocol (
                           OpenInfoBuffer[Index].ControllerHandle,
                           &gEfiSimpleTextInProtocolGuid,
                           (VOID **) &SimpleTextInput,
                           This->DriverBindingHandle,
@@ -850,36 +877,41 @@ FreeResources:
   }
 
   if (TerminalDevice->TerminalConsoleModeData != NULL) {
     FreePool (TerminalDevice->TerminalConsoleModeData);
   }
 
   FreePool (TerminalDevice);
 
 CloseProtocols:
 
   //
   // Remove Parent Device Path from
   // the Console Device Environment Variables
   //
   TerminalRemoveConsoleDevVariable (EFI_CON_IN_DEV_VARIABLE_NAME, ParentDevicePath);
   TerminalRemoveConsoleDevVariable (EFI_CON_OUT_DEV_VARIABLE_NAME, ParentDevicePath);
   TerminalRemoveConsoleDevVariable (EFI_ERR_OUT_DEV_VARIABLE_NAME, ParentDevicePath);
 
+  //
+  // Closing the protocol interfaces opened with BY_DRIVER is required. Closing
+  // the protocol interfaces opened with GET_PROTOCOL is not required but
+  // permitted.
+  //
   Status = gBS->CloseProtocol (
                   Controller,
                   &gEfiSerialIoProtocolGuid,
                   This->DriverBindingHandle,
                   Controller
                   );
   ASSERT_EFI_ERROR (Status);
 
   Status = gBS->CloseProtocol (
                   Controller,
                   &gEfiDevicePathProtocolGuid,
                   This->DriverBindingHandle,
                   Controller
                   );
   ASSERT_EFI_ERROR (Status);
   return Status;
 }
 
-- 
2.13.1.3.g8be5a757fa67