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

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

[Ranbir Singh]

v2 -> v3:
  - Updated FORWARD_NULL issue
      - Included DEBUG message and CpuDeadLoop within if blocks
  - Updated MISSING_BREAK issues resolution
      - Merged fallthrough case with default with conditional messaging

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  | 29 +++++++++++++++++
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++---------
 2 files changed, 48 insertions(+), 14 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH v3 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 | 29 ++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
index b54187ec228e..597cbe4646e8 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
@@ -267,6 +267,16 @@ UsbHcGetPciAddrForHostAddr (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+    //
+    // Should never be here
+    //
+    DEBUG ((DEBUG_ERROR, "UsbHcGetPciAddrForHostAddr: Invalid host memory pointer passed\n"));
+    CpuDeadLoop ();
+    return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -322,6 +332,16 @@ UsbHcGetHostAddrForPciAddr (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+    //
+    // Should never be here
+    //
+    DEBUG ((DEBUG_ERROR, "UsbHcGetHostAddrForPciAddr: Invalid pci memory pointer passed\n"));
+    CpuDeadLoop ();
+    return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -603,6 +623,15 @@ UsbHcFreeMem (
   //
   ASSERT (Block != NULL);
 
+  if (Block == NULL) {
+    //
+    // Should never be here
+    //
+    DEBUG ((DEBUG_ERROR, "UsbHcFreeMem: Invalid memory pointer passed\n"));
+    CpuDeadLoop ();
+    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 (#111012): https://edk2.groups.io/g/devel/message/111012
Mute This Topic: https://groups.io/mt/102502055/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 11/10/23 06:22, 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 | 29 ++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> index b54187ec228e..597cbe4646e8 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c
> @@ -267,6 +267,16 @@ UsbHcGetPciAddrForHostAddr (
>    }
>  
>    ASSERT ((Block != NULL));
> +
> +  if (Block == NULL) {
> +    //
> +    // Should never be here
> +    //
> +    DEBUG ((DEBUG_ERROR, "UsbHcGetPciAddrForHostAddr: Invalid host memory pointer passed\n"));
> +    CpuDeadLoop ();
> +    return 0;
> +  }
> +
>    //
>    // calculate the pci memory address for host memory address.
>    //
> @@ -322,6 +332,16 @@ UsbHcGetHostAddrForPciAddr (
>    }
>  
>    ASSERT ((Block != NULL));
> +
> +  if (Block == NULL) {
> +    //
> +    // Should never be here
> +    //
> +    DEBUG ((DEBUG_ERROR, "UsbHcGetHostAddrForPciAddr: Invalid pci memory pointer passed\n"));
> +    CpuDeadLoop ();
> +    return 0;
> +  }
> +
>    //
>    // calculate the pci memory address for host memory address.
>    //
> @@ -603,6 +623,15 @@ UsbHcFreeMem (
>    //
>    ASSERT (Block != NULL);
>  
> +  if (Block == NULL) {
> +    //
> +    // Should never be here
> +    //
> +    DEBUG ((DEBUG_ERROR, "UsbHcFreeMem: Invalid memory pointer passed\n"));
> +    CpuDeadLoop ();
> +    return;
> +  }
> +
>    //
>    // Release the current memory block if it is empty and not the head
>    //

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111166): https://edk2.groups.io/g/devel/message/111166
Mute This Topic: https://groups.io/mt/102502055/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 v3 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 may not be evident to any general code
reader/developer or static analyis tool why there is no break in between.

Merge the USB_ENDPOINT_CONTROL and default using conditional debug print.

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

Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++---------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 05528a478baf..00b3a13a95bb 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -2825,6 +2825,7 @@ XhcInitializeEndpointContext (
   UINTN                    NumEp;
   UINTN                    EpIndex;
   UINT8                    EpAddr;
+  UINT8                    EpType;
   UINT8                    Direction;
   UINT8                    Dci;
   UINT8                    MaxDci;
@@ -2871,7 +2872,8 @@ XhcInitializeEndpointContext (
       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;
@@ -2974,13 +2976,13 @@ XhcInitializeEndpointContext (
 
         break;
 
-      case USB_ENDPOINT_CONTROL:
-        //
-        // Do not support control transfer now.
-        //
-        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n"));
       default:
-        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: 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;
     }
@@ -3028,6 +3030,7 @@ XhcInitializeEndpointContext64 (
   UINTN                    NumEp;
   UINTN                    EpIndex;
   UINT8                    EpAddr;
+  UINT8                    EpType;
   UINT8                    Direction;
   UINT8                    Dci;
   UINT8                    MaxDci;
@@ -3074,7 +3077,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 +3181,14 @@ 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;
     }
-- 
2.34.1



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

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

[Laszlo Ersek]

On 11/10/23 06:22, 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 may not be evident to any general code
> reader/developer or static analyis tool why there is no break in between.
> 
> Merge the USB_ENDPOINT_CONTROL and default using conditional debug print.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++---------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 05528a478baf..00b3a13a95bb 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -2825,6 +2825,7 @@ XhcInitializeEndpointContext (
>    UINTN                    NumEp;
>    UINTN                    EpIndex;
>    UINT8                    EpAddr;
> +  UINT8                    EpType;
>    UINT8                    Direction;
>    UINT8                    Dci;
>    UINT8                    MaxDci;
> @@ -2871,7 +2872,8 @@ XhcInitializeEndpointContext (
>        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;
> @@ -2974,13 +2976,13 @@ XhcInitializeEndpointContext (
>  
>          break;
>  
> -      case USB_ENDPOINT_CONTROL:
> -        //
> -        // Do not support control transfer now.
> -        //
> -        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n"));
>        default:
> -        DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: 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;
>      }
> @@ -3028,6 +3030,7 @@ XhcInitializeEndpointContext64 (
>    UINTN                    NumEp;
>    UINTN                    EpIndex;
>    UINT8                    EpAddr;
> +  UINT8                    EpType;
>    UINT8                    Direction;
>    UINT8                    Dci;
>    UINT8                    MaxDci;
> @@ -3074,7 +3077,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 +3181,14 @@ 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;
>      }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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