[edk2-devel] [PATCH v2 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity

[edk2-devel] [PATCH v2 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD
  - Updated Cc list
  - Updated the commit message wrt MISSING_BREAK issues

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues
  MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues

 MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c  | 14 ++++++++++++++
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c |  6 ++++++
 2 files changed, 20 insertions(+)

-- 
2.34.1



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

[edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr
and UsbHcFreeMem do have

    ASSERT ((Block != NULL));

statements after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside for
loop and the loop exits because of Block != NULL; condition, then there
is no "Block" NULL pointer check afterwards and the code proceeds to do
dereferencing "Block" which will lead to CRASH.

Hence, for safety add NULL pointer checks always.

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

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
index b54187ec228e..b0654f148c4f 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
@@ -267,6 +267,11 @@ UsbHcGetPciAddrForHostAddr (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+    return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -322,6 +327,11 @@ UsbHcGetHostAddrForPciAddr (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+    return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -603,6 +613,10 @@ UsbHcFreeMem (
   //
   ASSERT (Block != NULL);
 
+  if (Block == NULL) {
+    return;
+  }
+
   //
   // Release the current memory block if it is empty and not the head
   //
-- 
2.34.1



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

Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues

[Laszlo Ersek]

On 11/7/23 06:57, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr
> and UsbHcFreeMem do have
> 
>     ASSERT ((Block != NULL));
> 
> statements after for loop, but these are applicable only in DEBUG mode.
> In RELEASE mode, if for whatever reasons there is no match inside for
> loop and the loop exits because of Block != NULL; condition, then there
> is no "Block" NULL pointer check afterwards and the code proceeds to do
> dereferencing "Block" which will lead to CRASH.
> 
> Hence, for safety add NULL pointer checks always.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> index b54187ec228e..b0654f148c4f 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> @@ -267,6 +267,11 @@ UsbHcGetPciAddrForHostAddr (
>    }
>  
>    ASSERT ((Block != NULL));
> +
> +  if (Block == NULL) {
> +    return 0;
> +  }
> +
>    //
>    // calculate the pci memory address for host memory address.
>    //
> @@ -322,6 +327,11 @@ UsbHcGetHostAddrForPciAddr (
>    }
>  
>    ASSERT ((Block != NULL));
> +
> +  if (Block == NULL) {
> +    return 0;
> +  }
> +
>    //
>    // calculate the pci memory address for host memory address.
>    //

The above two changes are not good.

There is a large number of call sites for these functions, and they
never error-check the returned value. In effect, these functions must
always succeed, or else there is a programming error somewhere in the
driver (potentially the caller supplying incorrect inputs), or the
hardware is broken. If the address mapping fails, we cannot do anything;
we certainly cannot proceed with null pointers (either device zero
addresses or host null pointers). Therefore, I suggest

  if (Block == NULL) {
    CpuDeadLoop ();
  }

instead.

> @@ -603,6 +613,10 @@ UsbHcFreeMem (
>    //
>    ASSERT (Block != NULL);
>  
> +  if (Block == NULL) {
> +    return;
> +  }
> +
>    //
>    // Release the current memory block if it is empty and not the head
>    //

I'm not happy about this either, but at least this one doesn't directly
make things worse than they are.

Note the comment in the context:

  //
  // If Block == NULL, it means that the current memory isn't
  // in the host controller's pool. This is critical because
  // the caller has passed in a wrong memory point
  //
  ASSERT (Block != NULL);

It says "critical", so I'd prefer a CpuDeadLoop() here too...

Laszlo



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

[edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The functions
XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has
a switch-case code in which the case USB_ENDPOINT_CONTROL: falls
through to default:

While this may be intentional, it is not evident to any general code
reader why there is no break; in between. Adding

    // No break; here as this is an intentional fallthrough.

as comment in between makes it explicit. Incidentally, the comment
satisfies Coverity as well.

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

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 05528a478baf..2afecd40cab0 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -2979,6 +2979,9 @@ XhcInitializeEndpointContext (
         // Do not support control transfer now.
         //
         DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n"));
+        //
+        // No break; here as this is an intentional fall through.
+        //
       default:
         DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unknown EP found, Transfer ring is not allocated.\n"));
         EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
@@ -3182,6 +3185,9 @@ XhcInitializeEndpointContext64 (
         // Do not support control transfer now.
         //
         DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unsupport Control EP found, Transfer ring is not allocated.\n"));
+        //
+        // No break; here as this is an intentional fall through.
+        //
       default:
         DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unknown EP found, Transfer ring is not allocated.\n"));
         EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
-- 
2.34.1



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

Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues

[Laszlo Ersek]

On 11/7/23 06:58, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The functions
> XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has
> a switch-case code in which the case USB_ENDPOINT_CONTROL: falls
> through to default:
> 
> While this may be intentional, it is not evident to any general code
> reader why there is no break; in between. Adding
> 
>     // No break; here as this is an intentional fallthrough.
> 
> as comment in between makes it explicit. Incidentally, the comment
> satisfies Coverity as well.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 05528a478baf..2afecd40cab0 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -2979,6 +2979,9 @@ XhcInitializeEndpointContext (
>          // Do not support control transfer now.
>          //
>          DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n"));
> +        //
> +        // No break; here as this is an intentional fall through.
> +        //
>        default:
>          DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unknown EP found, Transfer ring is not allocated.\n"));
>          EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
> @@ -3182,6 +3185,9 @@ XhcInitializeEndpointContext64 (
>          // Do not support control transfer now.
>          //
>          DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unsupport Control EP found, Transfer ring is not allocated.\n"));
> +        //
> +        // No break; here as this is an intentional fall through.
> +        //
>        default:
>          DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unknown EP found, Transfer ring is not allocated.\n"));
>          EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);

Eww, the contradictory DEBUG messages look awkard (in the original code), "unsupported" vs. "unknown"!

How about this one instead:

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 05528a478baf..66e2e7044e06 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -3034,6 +3034,7 @@ XhcInitializeEndpointContext64 (
   EFI_PHYSICAL_ADDRESS     PhyAddr;
   UINT8                    Interval;
   TRANSFER_RING            *EndpointTransferRing;
+  UINT8                    EpType;
 
   MaxDci = 0;
 
@@ -3074,7 +3075,8 @@ XhcInitializeEndpointContext64 (
       InputContext->EP[Dci-1].MaxBurstSize = 0x0;
     }
 
-    switch (EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK) {
+    EpType = EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK;
+    switch (EpType) {
       case USB_ENDPOINT_BULK:
         if (Direction == EfiUsbDataIn) {
           InputContext->EP[Dci-1].CErr   = 3;
@@ -3177,13 +3179,16 @@ XhcInitializeEndpointContext64 (
 
         break;
 
-      case USB_ENDPOINT_CONTROL:
-        //
-        // Do not support control transfer now.
-        //
-        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unsupport Control EP found, Transfer ring is not allocated.\n"));
       default:
-        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unknown EP found, Transfer ring is not allocated.\n"));
+        DEBUG ((
+          DEBUG_INFO,
+          "%a: %a found, Transfer ring is not allocated.\n",
+          __func__,
+          (EpType == USB_ENDPOINT_CONTROL ?
+           "Unsupported Control EP" :
+           "Unknown EP"
+           )
+          ));
         EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
         continue;
     }



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