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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-