From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: [PATCH] MdeModulePkg/Bus/Pci/UhciDxe: Fix various Coverity issues To: devel@edk2.groups.io From: "Ranbir Singh" X-Originating-Location: Bengaluru, Karnataka, IN (122.172.85.38) X-Originating-Platform: Windows Chrome 108 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Tue, 03 Jan 2023 22:45:07 -0800 Message-ID: Content-Type: multipart/alternative; boundary="pM1Po69zWQqcHtwAC16w" --pM1Po69zWQqcHtwAC16w Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable The function UhciConvertPollRate has a check ASSERT (Interval !=3D 0); but this comes into play only in DEBUG mode. In Release mode, there is no handling if the Interval parameter value is ZERO. To avoid shifting by a negative amount later in the code flow in this undesirable case, it is better to handle it as well by simply returning ZERO. The functions UsbHcGetPciAddressForHostMem and UsbHcFreeMem do have ASSERT ((Block !=3D 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 !=3D 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=3D4211 Signed-off-by: Ranbir Singh --- MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 ++++ MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c=C2=A0 | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c b/MdeModulePkg/Bus/Pc= i/UhciDxe/UhciSched.c index c08f949696..8ddef4b68c 100644 --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c @@ -214,6 +214,10 @@ UhciConvertPollRate ( ASSERT (Interval !=3D 0); +=C2=A0 if (Interval =3D=3D 0) { +=C2=A0 =C2=A0 return 0; +=C2=A0 } + // // Find the index (1 based) of the highest non-zero bit // diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci= /UhciDxe/UsbHcMem.c index c3d46f60be..3794f888e1 100644 --- a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c @@ -250,6 +250,11 @@ UsbHcGetPciAddressForHostMem ( } ASSERT ((Block !=3D NULL)); + +=C2=A0 if (Block =3D=3D NULL) { +=C2=A0 =C2=A0 return 0; +=C2=A0 } + // // calculate the pci memory address for host memory address. // @@ -536,6 +541,10 @@ UsbHcFreeMem ( // ASSERT (Block !=3D NULL); +=C2=A0 if (Block =3D=3D NULL) { +=C2=A0 =C2=A0 return; +=C2=A0 } + // // Release the current memory block if it is empty and not the head // -- 2.36.1.windows.1 --pM1Po69zWQqcHtwAC16w Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable
The function UhciConvertPollRate has a check
 
    ASSERT (Interval !=3D 0);
 
but this comes into play only in DEBUG mode. In Release mode, there is=
no handling if the Interval parameter value is ZERO. To avoid shifting=
by a negative amount later in the code flow in this undesirable case,<= /div>
it is better to handle it as well by simply returning ZERO.
 
The functions UsbHcGetPciAddressForHostMem and UsbHcFreeMem do have
 
    ASSERT ((Block !=3D 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<= /div>
loop and the loop exits because of Block !=3D NULL; condition, then th= ere
is no "Block" NULL pointer check afterwards and the code proceeds to d= o
dereferencing "Block" which will lead to CRASH.
 
Hence, for safety add NULL pointer checks always.
 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4211
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
---
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 ++++
 MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c  | 9 +++++++++
 2 files changed, 13 insertions(+)
 
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c b/MdeModulePkg/B= us/Pci/UhciDxe/UhciSched.c
index c08f949696..8ddef4b68c 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
@@ -214,6 +214,10 @@ UhciConvertPollRate (
 
   ASSERT (Interval !=3D 0);
 
+  if (Interval =3D=3D 0) {
+    return 0;
+  }
+
   //
   // Find the index (1 based) of the highest non-zero bit
   //
diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c b/MdeModulePkg/Bu= s/Pci/UhciDxe/UsbHcMem.c
index c3d46f60be..3794f888e1 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c
@@ -250,6 +250,11 @@ UsbHcGetPciAddressForHostMem (
   }
 
   ASSERT ((Block !=3D NULL));
+
+  if (Block =3D=3D NULL) {
+    return 0;
+  }
+
   //
   // calculate the pci memory address for host memory addre= ss.
   //
@@ -536,6 +541,10 @@ UsbHcFreeMem (
   //
   ASSERT (Block !=3D NULL);
 
+  if (Block =3D=3D NULL) {
+    return;
+  }
+
   //
   // Release the current memory block if it is empty and no= t the head
   //
--
2.36.1.windows.1
--pM1Po69zWQqcHtwAC16w--