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

[Ranbir Singh]

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

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
issue

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 9 +
 1 file changed, 9 insertions(+)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110799): https://edk2.groups.io/g/devel/message/110799
Mute This Topic: https://groups.io/mt/102437645/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

The function NotifyPhase has a check

ASSERT (Index < TypeMax);

but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2 at line
137, then it remains at TypeMax as assigned earlier at line 929. This
poses array overrun risk at lines 942 and 943. It is better to deploy
a safety check on Index limit before accessing array elements.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..519e1369f85e 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -939,6 +939,11 @@ NotifyPhase (
 }
 
 ASSERT (Index < TypeMax);
+
+if (Index >= TypeMax) {
+continue;
+}
+
 ResNodeHandled[Index] = TRUE;
 Alignment = RootBridge->ResAllocNode[Index].Alignment;
 BitsOfAlignment   = LowBitSet64 (Alignment + 1);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110800): https://edk2.groups.io/g/devel/message/110800
Mute This Topic: https://groups.io/mt/102437647/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: if there is no scenario of
return EFI_INVALID_PARAMETER;

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=4212

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index 519e1369f85e..3bd91e2787fd 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1531,6 +1531,10 @@ SubmitResources (
   return EFI_INVALID_PARAMETER;
 }
 
+//
+// No break; here as this is an intentional fall through.
+//
+
   case ACPI_ADDRESS_SPACE_TYPE_IO:
 //
 // Check aligment, it should be of the form 2^n-1
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110801): https://edk2.groups.io/g/devel/message/110801
Mute This Topic: https://groups.io/mt/102437649/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity

[Ranbir Singh]

v2 -> v3:
  - Dropped the ASSERT wrt REVERSE_INULL issue as suggested
  - Added Reviewed-by: tags
  - Rebased to latest master HEAD

v1 -> v2:
  - Rebased to latest master HEAD
  - Changed the solution wrt MISSING_BREAK issues

Ranbir Singh (2):
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 5 -
 2 files changed, 10 insertions(+), 1 deletion(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110802): https://edk2.groups.io/g/devel/message/110802
Mute This Topic: https://groups.io/mt/102437985/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function USBMouseDriverBindingStart do have

ASSERT (UsbMouseDevice != NULL);

after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference "UsbMouseDevice"
afterwards which will lead to CRASH.

Hence, for safety add NULL pointer checks always.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
Reviewed-by: Laszlo Ersek 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 451d4b934f4c..67072d476196 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -160,7 +160,10 @@ USBMouseDriverBindingStart (
   }
 
   UsbMouseDevice = AllocateZeroPool (sizeof (USB_MOUSE_DEV));
-  ASSERT (UsbMouseDevice != NULL);
+  if (UsbMouseDevice == NULL) {
+Status = EFI_OUT_OF_RESOURCES;
+goto ErrorExit;
+  }
 
   UsbMouseDevice->UsbIo = UsbIo;
   UsbMouseDevice->Signature = USB_MOUSE_DEV_SIGNATURE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110803): https://edk2.groups.io/g/devel/message/110803
Mute This Topic: https://groups.io/mt/102437986/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

The function GetNextHidItem has a switch-case block in which the case 1:
falls through to case 2: and then case 2: falls through to case 3:.

There is no possibility of the if blocks within case 2: and case 3: to
succeed later and not succeed in the original case and hence the fall
throughs even if it hypothetically happens are redundant as the code
still will eventually return NULL only at the function end point.

Better introduce straight forward break; statement within actual cases.

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

Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
Reviewed-by: Laszlo Ersek 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
index acc19acd98e0..f07e48774a34 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
@@ -89,6 +89,8 @@ GetNextHidItem (
   return StartPos;
 }
 
+break;
+
   case 2:
 //
 // 2-byte data
@@ -99,6 +101,8 @@ GetNextHidItem (
   return StartPos;
 }
 
+break;
+
   case 3:
 //
 // 4-byte data, adjust size
@@ -109,6 +113,8 @@ GetNextHidItem (
   StartPos += 4;
   return StartPos;
 }
+
+break;
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110804): https://edk2.groups.io/g/devel/message/110804
Mute This Topic: https://groups.io/mt/102437987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity

[Ranbir Singh]

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

Ranbir Singh (5):
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 73 +++-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c|  1 -
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 48 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   | 45 ++--
 4 files changed, 113 insertions(+), 54 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110810): https://edk2.groups.io/g/devel/message/110810
Mute This Topic: https://groups.io/mt/102438297/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function PciHotPlugRequestNotify has two if blocks towards the end
of function both containing return. Due to the parameter checks ensured
at the beginning of the function, one of the two if blocks is bound to
come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
redundant/deadcode.

To fix it, either of line 2109 or 2112 can be deleted.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
index 3f8c6e6da7dc..5b71e152f3ea 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
@@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
 //
 // End for
 //
-return EFI_SUCCESS;
   }
 
   return EFI_SUCCESS;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110811): https://edk2.groups.io/g/devel/message/110811
Mute This Topic: https://groups.io/mt/102438298/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly 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=4239

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6594b8eae83f..eda97285ee18 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1428,6 +1428,9 @@ UpdatePciInfo (
   switch (Ptr->AddrSpaceGranularity) {
 case 32:
   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypeMem32;
+  //
+  // No break; here as this is an intentional fall through.
+  //
 case 64:
   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
   break;
@@ -1440,6 +1443,9 @@ UpdatePciInfo (
   switch (Ptr->AddrSpaceGranularity) {
 case 32:
   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
+  //
+  // No break; here as this is an intentional fall through.
+  //
 case 64:
   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
   break;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110812): https://edk2.groups.io/g/devel/message/110812
Mute This Topic: https://groups.io/mt/102438299/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function PciHostBridgeResourceAllocator is not making use of the
generic approach as is used in one of the other function namely -
DumpResourceMap. As a result, the following warnings can be seen as
reported by Coverity e.g.

(30) Event address_of:  Taking address with "&IoBridge" yields a
 singleton pointer.
(31) Event callee_ptr_arith:Passing "&IoBridge" to function
 "FindResourceNode" which uses it as an array. This might corrupt
 or misinterpret adjacent memory locations.

Hence, adopt the generic approach to fix the issues at relevant points.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 84fc0161a19c..71767d3793d4 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
   UINT64 Mem64ResStatus;
   UINT64 PMem64ResStatus;
   UINT32 MaxOptionRomSize;
+  PCI_RESOURCE_NODE  **ChildResources;
+  UINTN  ChildResourceCount;
   PCI_RESOURCE_NODE  *IoBridge;
   PCI_RESOURCE_NODE  *Mem32Bridge;
   PCI_RESOURCE_NODE  *PMem32Bridge;
@@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
 // Create the entire system resource map from the information collected by
 // enumerator. Several resource tree was created
 //
-FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
-FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
-FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
-FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
-FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
-
+ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
+IoBridge = ChildResources[0];
 ASSERT (IoBridge != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
+Mem32Bridge = ChildResources[0];
 ASSERT (Mem32Bridge  != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
+PMem32Bridge = ChildResources[0];
 ASSERT (PMem32Bridge != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
+Mem64Bridge = ChildResources[0];
 ASSERT (Mem64Bridge  != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
+PMem64Bridge = ChildResources[0];
 ASSERT (PMem64Bridge != NULL);
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110813): https://edk2.groups.io/g/devel/message/110813
Mute This Topic: https://groups.io/mt/102438300/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function StartPciDevices has a check

ASSERT (RootBridge != NULL);

but this comes into play only in DEBUG mode. In Release mode, there
is no handling if the RootBridge value is NULL and the code proceeds
to unconditionally dereference "RootBridge" which will lead to CRASH.

Hence, for safety add NULL pointer checks always and return
EFI_NOT_READY if RootBridge value is NULL which is one of the return
values as mentioned in the function description header.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index 581e9075ad41..3de80d98370e 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -772,7 +772,10 @@ StartPciDevices (
   LIST_ENTRY *CurrentLink;
 
   RootBridge = GetRootBridgeByHandle (Controller);
-  ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+return EFI_NOT_READY;
+  }
+
   ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
 
   CurrentLink = mPciDevicePool.ForwardLink;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110814): https://edk2.groups.io/g/devel/message/110814
Mute This Topic: https://groups.io/mt/102438320/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

The return value after calls to functions
gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge,
PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write,
gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is
stored in Status, but it is not made of any use and later Status
gets overridden.

Remove the return value storage in Status or add Status check as
seems appropriate at a particular point.

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

Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 +++-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   |  8 +++
 3 files changed, 72 insertions(+), 46 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index 3de80d98370e..9b0587c94d05 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -544,12 +544,12 @@ DeRegisterPciDevice (
   EFI_OPEN_PROTOCOL_TEST_PROTOCOL
   );
   if (!EFI_ERROR (Status)) {
-Status = gBS->UninstallMultipleProtocolInterfaces (
-Handle,
-&gEfiLoadFile2ProtocolGuid,
-&PciIoDevice->LoadFile2,
-NULL
-);
+gBS->UninstallMultipleProtocolInterfaces (
+   Handle,
+   &gEfiLoadFile2ProtocolGuid,
+   &PciIoDevice->LoadFile2,
+   NULL
+   );
   }
 
   //
@@ -678,19 +678,21 @@ StartPciDevicesOnBridge (
ChildHandleBuffer
);
 
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationSupported,
- 0,
- &Supports
- );
-Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationEnable,
- Supports,
- NULL
- );
+if (!EFI_ERROR (Status)) {
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationSupported,
+   0,
+   &Supports
+   );
+  Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationEnable,
+   Supports,
+   NULL
+   );
+}
 
 return Status;
   } else {
@@ -726,19 +728,21 @@ StartPciDevicesOnBridge (
ChildHandleBuffer
);
 
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationSupported,
- 0,
- &Supports
- );
-Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationEnable,
- Supports,
- NULL
- );
+if (!EFI_ERROR (Status)) {
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationSupported,
+   0,
+   &Supports
+   );
+  Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationEnable,
+   Supports,
+   NULL
+   );
+}
   }
 
   CurrentLink = CurrentLink->ForwardLink;
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index eda97285ee18..636885dd189d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -2796,6 +2796,20 @@ IsPciDeviceRejec

[edk2-devel] [PATCH v2 0/2] BZ 4249: Fix FatPkg/EnhancedFatDxe issues pointed by Coverity

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD

Ranbir Singh (2):
  FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues
  FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 FatPkg/EnhancedFatDxe/DiskCache.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110816): https://edk2.groups.io/g/devel/message/110816
Mute This Topic: https://groups.io/mt/102438361/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The functions FatGetDirEntInfo and FatOpenDirEnt contains the code
statements

  Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
  and
  OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) | 
(DirEnt->Entry.FileCluster);

respectively. As per Coverity report, in both these statements, there is
an "Operand1" with type "UINT16" (16 bits, unsigned) which is promoted in
"(Operand1 << 16) | Operand2" to type "int" (32 bits, signed), then sign-
extended to type "unsigned long long" (64 bits, unsigned). If the result
of "(Operand1 << 16) | Operand2" is greater than 0x7FFF, the upper
bits of the result will all be 1.

So to avoid sign-extension, typecast the Operand1 and then the inter-
-mediate result after << 16 operation with UINTN. Note - UINTN is the
data type of the variable on the LHS of the assignment.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c 
b/FatPkg/EnhancedFatDxe/DirectoryManage.c
index 723fc35f38db..a21b7973cd21 100644
--- a/FatPkg/EnhancedFatDxe/DirectoryManage.c
+++ b/FatPkg/EnhancedFatDxe/DirectoryManage.c
@@ -474,7 +474,7 @@ FatGetDirEntInfo (
 Info   = Buffer;
 Info->Size = ResultSize;
 if ((Entry->Attributes & FAT_ATTRIBUTE_DIRECTORY) != 0) {
-  Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
+  Cluster= (UINTN)((UINTN)(Entry->FileClusterHigh) << 16) | 
Entry->FileCluster;
   Info->PhysicalSize = FatPhysicalDirSize (Volume, Cluster);
   Info->FileSize = Info->PhysicalSize;
 } else {
@@ -1167,7 +1167,7 @@ FatOpenDirEnt (
   //
   Volume = Parent->Volume;
   OFile->FullPathLen = Parent->FullPathLen + 1 + StrLen 
(DirEnt->FileString);
-  OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) | 
(DirEnt->Entry.FileCluster);
+  OFile->FileCluster = (UINTN)((UINTN)(DirEnt->Entry.FileClusterHigh) << 
16) | (DirEnt->Entry.FileCluster);
   InsertTailList (&Parent->ChildHead, &OFile->ChildLink);
 } else {
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110817): https://edk2.groups.io/g/devel/message/110817
Mute This Topic: https://groups.io/mt/102438364/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/2] FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function FatInitializeDiskCache evaluates an expression

FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment

and assigns it to DataCacheSize which is of type UINTN.

As per Coverity report,
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment is a
potentially overflowing expression with type "int" (32 bits, signed)
evaluated using 32-bit arithmetic, and then used in a context that
expects an expression of type "UINTN" (64 bits, unsigned).

To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type "UINTN".

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 FatPkg/EnhancedFatDxe/DiskCache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c 
b/FatPkg/EnhancedFatDxe/DiskCache.c
index d1a34a6a646f..d56e338586d8 100644
--- a/FatPkg/EnhancedFatDxe/DiskCache.c
+++ b/FatPkg/EnhancedFatDxe/DiskCache.c
@@ -477,7 +477,7 @@ FatInitializeDiskCache (
   DiskCache[CacheFat].BaseAddress   = Volume->FatPos;
   DiskCache[CacheFat].LimitAddress  = Volume->FatPos + Volume->FatSize;
   FatCacheSize  = FatCacheGroupCount << 
DiskCache[CacheFat].PageAlignment;
-  DataCacheSize = FAT_DATACACHE_GROUP_COUNT << 
DiskCache[CacheData].PageAlignment;
+  DataCacheSize = (UINTN)FAT_DATACACHE_GROUP_COUNT << 
DiskCache[CacheData].PageAlignment;
   //
   // Allocate the Fat Cache buffer
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110818): https://edk2.groups.io/g/devel/message/110818
Mute This Topic: https://groups.io/mt/102438365/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

As mentioned in the commit message, the comment helps in making it
explicit and evident that the missing break is not a human miss, but
intentional.
Hence, the comment should be considered as being added for the human code
readers / developers.

So even if some static analysis tool flags such an issue, it can be fairly
easy now to ignore that on manual inspection. If desired this can also be
stated in the comment itself like -

+  //
+  // No break here as this is an intentional fall through.
+  // Ignore any static tool issue if pointed.
+  //

Yes, there can be other solutions (which may or may not be worth the
effort), but for now I went with the least code change approach.

On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:

> This comment style only works with Coverity.
>
> Other static analysis tools may flag the same issue again.
>
> It is better to update the logic so no static analysis tool will
> flag this issue.
>
> Mike
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Laszlo
> > Ersek
> > Sent: Tuesday, November 7, 2023 8:23 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Ni, Ray ; Veeresh Sangolli
> > 
> > Subject: Re: [edk2-devel] [PATCH v2 2/5]
> > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function UpdatePciInfo has switch-case code in which there are
> > fall
> > > through from case 32: to case 64:. While this is seeemingly
> > 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=4239
> > >
> > > Cc: Ray Ni 
> > > Co-authored-by: Veeresh Sangolli 
> > > Signed-off-by: Ranbir Singh 
> > > Signed-off-by: Ranbir Singh 
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > index 6594b8eae83f..eda97285ee18 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> > >switch (Ptr->AddrSpaceGranularity) {
> > >  case 32:
> > >PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypeMem32;
> > > +  //
> > > +  // No break; here as this is an intentional fall
> > through.
> > > +  //
> > >  case 64:
> > >PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >break;
> > > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > >switch (Ptr->AddrSpaceGranularity) {
> > >  case 32:
> > >PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypePMem32;
> > > +  //
> > > +  // No break; here as this is an intentional fall
> > through.
> > > +  //
> > >  case 64:
> > >PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >break;
> >
> > Agree, but the semicolon's placement is awkward. I propose
> >
> >   No break here, as this is an intentional fall through.
> >
> > Reviewed-by: Laszlo Ersek 
> >
> >
> >
> > 
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110890): https://edk2.groups.io/g/devel/message/110890
Mute This Topic: https://groups.io/mt/102438299/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

Hi Mike,

I agree that any manual inspection is sort of a burden, more so when it
becomes repetitive in the long run.

When I was doing these Coverity checks (Nov-Dec' 2022), I was working in
Dell and had access to the real systems to check the execution flow as well
as the Coverity status. I could never post these patches while being there,
but happened to raise Bugzilla's and post them there instead hoping that
they would be taken up by somebody further.

I am no longer with Dell and later on when I found that those BZ / issues
pointed out by Coverity still exist as there are no code changes in related
contexts, I thought of taking them forward in whatever limited capacity I
can. I am a bit hesitant to do any further code changes as now I do not
have any systems to check the execution flow as well as the Coverity
status. So, I do not guarantee, but will try to make the code changes
wherever it is easy to ascertain that the functional flow would not be
impacted and the same issue won't exist anymore.

Ranbir Singh

On Wed, Nov 8, 2023 at 9:35 AM Kinney, Michael D 
wrote:

> Hi Ranbir,
>
>
>
> Ignoring false positive in static analysis tools is typically a burden.
>
>
>
> It is better to avoid false positives with code changes as long as the
> code changes do not make the implementation confusing and hard to maintain.
>
>
>
> I think depending on fall through case statements is confusing and
> removing them will make the code easier to understand and maintain.
>
>
>
> Mike
>
>
>
> *From:* Ranbir Singh 
> *Sent:* Tuesday, November 7, 2023 7:51 PM
> *To:* Kinney, Michael D 
> *Cc:* devel@edk2.groups.io; ler...@redhat.com; Ni, Ray ;
> Veeresh Sangolli 
> *Subject:* Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
>
>
>
> As mentioned in the commit message, the comment helps in making it
> explicit and evident that the missing break is not a human miss, but
> intentional.
>
> Hence, the comment should be considered as being added for the human code
> readers / developers.
>
>
>
> So even if some static analysis tool flags such an issue, it can be fairly
> easy now to ignore that on manual inspection. If desired this can also be
> stated in the comment itself like -
>
>
>
> +  //
> +  // No break here as this is an intentional fall through.
>
> +  // Ignore any static tool issue if pointed.
> +  //
>
>
>
> Yes, there can be other solutions (which may or may not be worth the
> effort), but for now I went with the least code change approach.
>
>
>
> On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <
> michael.d.kin...@intel.com> wrote:
>
> This comment style only works with Coverity.
>
> Other static analysis tools may flag the same issue again.
>
> It is better to update the logic so no static analysis tool will
> flag this issue.
>
> Mike
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Laszlo
> > Ersek
> > Sent: Tuesday, November 7, 2023 8:23 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Ni, Ray ; Veeresh Sangolli
> > 
> > Subject: Re: [edk2-devel] [PATCH v2 2/5]
> > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function UpdatePciInfo has switch-case code in which there are
> > fall
> > > through from case 32: to case 64:. While this is seeemingly
> > 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=4239
> > >
> > > Cc: Ray Ni 
> > > Co-authored-by: Veeresh Sangolli 
> > > Signed-off-by: Ranbir Singh 
> > > Signed-off-by: Ranbir Singh 
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > index 6594b8eae83f..eda97285ee18 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumera

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

[Ranbir Singh]

Thanks for the detailed analysis Laszlo.

I don't deny that there cannot be false positives by the tool.

For now, I can move forward with your proposal of replacing continue with
CpuDeadLoop and will post v3.


On Tue, Nov 7, 2023 at 9:17 PM Laszlo Ersek  wrote:

> Hi Ranbir,
>
> On 11/7/23 06:06, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function NotifyPhase has a check
> >
> > ASSERT (Index < TypeMax);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if the Index value is within array limits or not. If for
> > whatever reasons, the Index does not get re-assigned to Index2 at line
> > 137, then it remains at TypeMax as assigned earlier at line 929. This
>
> 137 should be 937
>
> > poses array overrun risk at lines 942 and 943. It is better to deploy
> > a safety check on Index limit before accessing array elements.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index d573e532bac8..519e1369f85e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -939,6 +939,11 @@ NotifyPhase (
> >  }
> >
> >  ASSERT (Index < TypeMax);
> > +
> > +if (Index >= TypeMax) {
> > +continue;
> > +}
> > +
> >  ResNodeHandled[Index] = TRUE;
> >  Alignment =
> RootBridge->ResAllocNode[Index].Alignment;
> >  BitsOfAlignment   = LowBitSet64 (Alignment + 1);
>
> The ASSERT() will never fire. But I agree that it is hard to see.
>
> I propose that we should add
>
>   if (Index == TypeMax) {
> CpuDeadLoop ();
>   }
>
> instead of "continue".
>
> Here's why the ASSERT() will never fire.
>
> - The outer loop (using Index1) will run five times exactly.
>
> - In each execution of the outer loop, we have two branches. Each branch
> flips *at most* one element in ResNodeHandled from FALSE to TRUE.
>
> While each branch writes to exactly one ResNodeHandled element (storing
> TRUE), the original ResNodeHandled value may be FALSE, or may be TRUE.
> (TRUE as original value is not easy to see, but consider that the first
> branch of the outer loop body may notice ResNone for a particular resource
> type *after* the second branch of the outer loop body has assigned a
> resource to that type. That *is* a bug, but a *different* one!)
>
> The point is that the FALSE->TRUE *transition* may happen for at most one
> resource type per outer loop iteration. This means that in the Nth
> iteration of the outer loop (Index1=0, 1, ... 4 inclusive), there are
> initially *at least* (5 - Index1) FALSE elements in ResNodeHandled. In the
> last iteration of the outer loop (Index1=4), there is at least 5 - 4 = 1
> FALSE element in ResNodeHandled.
>
> - This means that the Index2-based inner loop will *always find* an Index2
> where ResNodeHandled is FALSE.
>
> - For the first such Index2 in the inner loop body, Index will be
> assigned, because MaxAlignment starts with 0, and the Alignment field has
> type UINT64.
>
> Therefore the ASSERT will never fire -- it is a correct assertion.
>
> Basically the assert states that we have a resource type to assign at that
> point -- and that claim is correct. So, unfortunately, Coverity is wrong
> here. We should add a CpuDeadLoop() therefore, to tell coverity that we're
> willing to hang there even in RELEASE builds.
>
> "continue" is not useful in any case, because if there are no more
> resource types to assign, then continuing the outer loop makes no sense.
> That is, "break" would make more sense. (But again, that too would never be
> reached.)
>
> ... For making the code easier to understand, I'd perhaps propose (this is
> displayed with "git diff -b"):
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index d573e532bac8..87c85e9df771 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -833,11 +833,12

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

[Ranbir Singh]

v2 -> v3:
  - Rebased to latest master HEAD
  - Replaced continue with CpuDeadLoop wrt OVERRUN issue
  - Changed to more generic solution wrt MISSING_BREAK issue

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

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
  MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
issue

 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 65 ++--
 1 file changed, 31 insertions(+), 34 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110992): https://edk2.groups.io/g/devel/message/110992
Mute This Topic: https://groups.io/mt/102490510/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function NotifyPhase has a check

ASSERT (Index < TypeMax);

but this comes into play only in DEBUG mode. In Release mode, there is
no handling if the Index value is within array limits or not. If for
whatever reasons, the Index does not get re-assigned to Index2 at line
937, then it remains at TypeMax as assigned earlier at line 929. This
poses array overrun risk at lines 942 and 943. It is better to deploy
a safety check on Index limit before accessing array elements.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index d573e532bac8..c2c143068cd2 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -939,6 +939,11 @@ NotifyPhase (
 }
 
 ASSERT (Index < TypeMax);
+
+if (Index == TypeMax) {
+  CpuDeadLoop ();
+}
+
 ResNodeHandled[Index] = TRUE;
 Alignment = RootBridge->ResAllocNode[Index].Alignment;
 BitsOfAlignment   = LowBitSet64 (Alignment + 1);
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110993): https://edk2.groups.io/g/devel/message/110993
Mute This Topic: https://groups.io/mt/102490513/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function SubmitResources has a switch-case code in which the
case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.

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.

SubmitResources function is supposed to handle only Mem or IO resources.
So, update the ResType parameter check reflecting that and re-model the
switch-case code in contention using just one if condition to further
handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
This leads to mostly indentation level code changes. Few ASSERT's later
present are henceforth not required.

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

Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60 +---
 1 file changed, 26 insertions(+), 34 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index c2c143068cd2..ed0aa455bfd4 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -1453,7 +1453,7 @@ SetBusNumbers (
   Submits the I/O and memory resource requirements for the specified PCI Root 
Bridge.
 
   @param This  The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_ 
PROTOCOL instance.
-  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource 
requirements.
+  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory resource 
requirements
are being submitted.
   @param Configuration The pointer to the PCI I/O and PCI memory resource 
descriptor.
 
@@ -1496,7 +1496,7 @@ SubmitResources (
   // descriptors are ignored and the function returns 
EFI_INVALID_PARAMETER.
   //
   for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *)Configuration; 
Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR; Descriptor++) {
-if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
+if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) && 
(Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
   return EFI_INVALID_PARAMETER;
 }
 
@@ -1509,40 +1509,34 @@ SubmitResources (
   (Descriptor->SpecificFlag & 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0 ? L" 
(Prefetchable)" : L""
   ));
 DEBUG ((DEBUG_INFO, "  Length/Alignment = 0x%lx / 0x%lx\n", 
Descriptor->AddrLen, Descriptor->AddrRangeMax));
-switch (Descriptor->ResType) {
-  case ACPI_ADDRESS_SPACE_TYPE_MEM:
-if ((Descriptor->AddrSpaceGranularity != 32) && 
(Descriptor->AddrSpaceGranularity != 64)) {
-  return EFI_INVALID_PARAMETER;
-}
 
-if ((Descriptor->AddrSpaceGranularity == 32) && 
(Descriptor->AddrLen >= SIZE_4GB)) {
-  return EFI_INVALID_PARAMETER;
-}
+if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
+  if ((Descriptor->AddrSpaceGranularity != 32) && 
(Descriptor->AddrSpaceGranularity != 64)) {
+return EFI_INVALID_PARAMETER;
+  }
 
-//
-// If the PCI root bridge does not support separate windows for 
nonprefetchable and
-// prefetchable memory, then the PCI bus driver needs to include 
requests for
-// prefetchable memory in the nonprefetchable memory pool.
-//
-if (((RootBridge->AllocationAttributes & 
EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
-((Descriptor->SpecificFlag & 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) != 0)
-)
-{
-  return EFI_INVALID_PARAMETER;
-}
+  if ((Descriptor->AddrSpaceGranularity == 32) && (Descriptor->AddrLen 
>= SIZE_4GB)) {
+return EFI_INVALID_PARAMETER;
+  }
 
-  case ACPI_ADDRESS_SPACE_TYPE_IO:
-//
-// Check aligment, it should be of the form 2^n-1
-//
-if (GetPowerOfTwo64 (Descriptor->AddrRangeMax + 1) != 
(Descriptor->AddrRangeMax + 1)) {
-  return EFI_INVALID_PARAMETER;
-}
+  //
+  // If the PCI root bridge does not support separate windows for 
nonprefetchable and
+  // prefetchable memory, then the PCI bus driver needs to include 
requests for
+  // prefetchable memory in the nonprefetchable memory pool.
+  //
+  if (((RootBridge->AllocationAttributes & 
EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0) &&
+  ((Descriptor->SpecificFlag &

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

[Ranbir Singh]

As far as I know, from a secure coding perspective, it would be recommended
that array overrun condition check is captured in the code even if it is
felt that it will never hit.

Generally speaking, I won't be in favour of handling other ASSERT
conditions updates even if required if they are not related to array
overrun conditions i.e., the context of the patch.

If someone / PCI maintainers can advise in this patch context what should
be done in the array overrun condition, I will be happy to update,
otherwise, sorry to say I won't be able to pursue this particular one
further and hence would be leaving the related code with the status quo
here.

On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:

> Hi Ranbir,
>
> A deadloop without even a debug print is not good behavior.
>
> If this condition really represents a condition where it is not possible
> to complete the PCI resource allocation/assignment, then an error status
> code should be returned to the caller of NotifyPhase().  Perhaps
> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
> likely be updated to do the same.
>
> This may also require the caller of this service, the PCI Bus Driver,
> to be reviewed to make sure it handles error conditions from NotifyPhase().
>
> I recommend you get help on the proposed code changes from the PCI
> subsystem maintainers.
>
> Thanks,
>
> Mike
>
>
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Ranbir
> > Singh
> > Sent: Thursday, November 9, 2023 9:39 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Ni, Ray ; Veeresh Sangolli
> > 
> > Subject: [edk2-devel] [PATCH v3 1/2]
> > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
> >
> > From: Ranbir Singh 
> >
> > The function NotifyPhase has a check
> >
> > ASSERT (Index < TypeMax);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there is
> > no handling if the Index value is within array limits or not. If for
> > whatever reasons, the Index does not get re-assigned to Index2 at line
> > 937, then it remains at TypeMax as assigned earlier at line 929. This
> > poses array overrun risk at lines 942 and 943. It is better to deploy
> > a safety check on Index limit before accessing array elements.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index d573e532bac8..c2c143068cd2 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -939,6 +939,11 @@ NotifyPhase (
> >  }
> >
> >
> >
> >  ASSERT (Index < TypeMax);
> >
> > +
> >
> > +if (Index == TypeMax) {
> >
> > +  CpuDeadLoop ();
> >
> > +}
> >
> > +
> >
> >  ResNodeHandled[Index] = TRUE;
> >
> >  Alignment = RootBridge-
> > >ResAllocNode[Index].Alignment;
> >
> >  BitsOfAlignment   = LowBitSet64 (Alignment + 1);
> >
> > --
> > 2.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#110993):
> > https://edk2.groups.io/g/devel/message/110993
> > Mute This Topic: https://groups.io/mt/102490513/1643496
> > Group Owner: devel+ow...@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kin...@intel.com]
> > -=-=-=-=-=-=
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111007): https://edk2.groups.io/g/devel/message/111007
Mute This Topic: https://groups.io/mt/102490513/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

Options before us till now -

1. Add array overrun check and Debug statement before CpuDeadLoop within
2. Status Quo (not everything can be ideal :-))

Question before us
 - Is 1 better than 2 ?


On Fri, Nov 10, 2023 at 8:41 AM Ranbir Singh 
wrote:

> As far as I know, from a secure coding perspective, it would be
> recommended that array overrun condition check is captured in the code even
> if it is felt that it will never hit.
>
> Generally speaking, I won't be in favour of handling other ASSERT
> conditions updates even if required if they are not related to array
> overrun conditions i.e., the context of the patch.
>
> If someone / PCI maintainers can advise in this patch context what should
> be done in the array overrun condition, I will be happy to update,
> otherwise, sorry to say I won't be able to pursue this particular one
> further and hence would be leaving the related code with the status quo
> here.
>
> On Fri, Nov 10, 2023 at 2:10 AM Kinney, Michael D <
> michael.d.kin...@intel.com> wrote:
>
>> Hi Ranbir,
>>
>> A deadloop without even a debug print is not good behavior.
>>
>> If this condition really represents a condition where it is not possible
>> to complete the PCI resource allocation/assignment, then an error status
>> code should be returned to the caller of NotifyPhase().  Perhaps
>> EFI_OUT_OF_RESOURCES.  The other ASSERT() conditions in this API should
>> likely be updated to do the same.
>>
>> This may also require the caller of this service, the PCI Bus Driver,
>> to be reviewed to make sure it handles error conditions from
>> NotifyPhase().
>>
>> I recommend you get help on the proposed code changes from the PCI
>> subsystem maintainers.
>>
>> Thanks,
>>
>> Mike
>>
>>
>>
>> > -Original Message-
>> > From: devel@edk2.groups.io  On Behalf Of Ranbir
>> > Singh
>> > Sent: Thursday, November 9, 2023 9:39 AM
>> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
>> > Cc: Ni, Ray ; Veeresh Sangolli
>> > 
>> > Subject: [edk2-devel] [PATCH v3 1/2]
>> > MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>> >
>> > From: Ranbir Singh 
>> >
>> > The function NotifyPhase has a check
>> >
>> > ASSERT (Index < TypeMax);
>> >
>> > but this comes into play only in DEBUG mode. In Release mode, there is
>> > no handling if the Index value is within array limits or not. If for
>> > whatever reasons, the Index does not get re-assigned to Index2 at line
>> > 937, then it remains at TypeMax as assigned earlier at line 929. This
>> > poses array overrun risk at lines 942 and 943. It is better to deploy
>> > a safety check on Index limit before accessing array elements.
>> >
>> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
>> >
>> > Cc: Ray Ni 
>> > Co-authored-by: Veeresh Sangolli 
>> > Signed-off-by: Ranbir Singh 
>> > Signed-off-by: Ranbir Singh 
>> > ---
>> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 5 +
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > index d573e532bac8..c2c143068cd2 100644
>> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
>> > @@ -939,6 +939,11 @@ NotifyPhase (
>> >  }
>> >
>> >
>> >
>> >  ASSERT (Index < TypeMax);
>> >
>> > +
>> >
>> > +if (Index == TypeMax) {
>> >
>> > +  CpuDeadLoop ();
>> >
>> > +}
>> >
>> > +
>> >
>> >  ResNodeHandled[Index] = TRUE;
>> >
>> >  Alignment = RootBridge-
>> > >ResAllocNode[Index].Alignment;
>> >
>> >  BitsOfAlignment   = LowBitSet64 (Alignment + 1);
>> >
>> > --
>> > 2.34.1
>> >
>> >
>> >
>> > -=-=-=-=-=-=
>> > Groups.io Links: You receive all messages sent to this group.
>> > View/Reply Online (#110993):
>> > https://edk2.groups.io/g/devel/message/110993
>> > Mute This Topic: https://groups.io/mt/102490513/1643496
>> > Group Owner: devel+ow...@edk2.groups.io
>> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
>> > [michael.d.kin...@intel.com]
>> > -=-=-=-=-=-=
>> >
>>
>>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111009): https://edk2.groups.io/g/devel/message/111009
Mute This Topic: https://groups.io/mt/102490513/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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 
Signed-off-by: Ranbir Singh 
---
 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 (
   UINTNNumEp;
   UINTNEpIndex;
   UINT8EpAddr;
+  UINT8EpType;
   UINT8Direction;
   UINT8Dci;
   UINT8MaxDci;
@@ -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 (
   UINTNNumEp;
   UINTNEpIndex;
   UINT8EpAddr;
+  UINT8EpType;
   UINT8Direction;
   UINT8Dci;
   UINT8MaxDci;
@@ -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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Ranbir Singh]

I was apprehensive about this from the very beginning.

Anyway, for now I will be dropping this issue from the series.

On Tue, Nov 7, 2023 at 10:11 PM Laszlo Ersek  wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function PciHostBridgeResourceAllocator is not making use of the
> > generic approach as is used in one of the other function namely -
> > DumpResourceMap. As a result, the following warnings can be seen as
> > reported by Coverity e.g.
> >
> > (30) Event address_of:Taking address with "&IoBridge" yields a
> >  singleton pointer.
> > (31) Event callee_ptr_arith:  Passing "&IoBridge" to function
> >  "FindResourceNode" which uses it as an array. This might corrupt
> >  or misinterpret adjacent memory locations.
> >
> > Hence, adopt the generic approach to fix the issues at relevant points.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 
> >  1 file changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 84fc0161a19c..71767d3793d4 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
> >UINT64 Mem64ResStatus;
> >UINT64 PMem64ResStatus;
> >UINT32 MaxOptionRomSize;
> > +  PCI_RESOURCE_NODE  **ChildResources;
> > +  UINTN  ChildResourceCount;
> >PCI_RESOURCE_NODE  *IoBridge;
> >PCI_RESOURCE_NODE  *Mem32Bridge;
> >PCI_RESOURCE_NODE  *PMem32Bridge;
> > @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
> >  // Create the entire system resource map from the information
> collected by
> >  // enumerator. Several resource tree was created
> >  //
> > -FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
> > -FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
> > -FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
> > -FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
> > -FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
> > -
> > +ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool,
> NULL);
> > +ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +ASSERT (ChildResources != NULL);
> > +FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
> > +IoBridge = ChildResources[0];
> >  ASSERT (IoBridge != NULL);
> > +
> > +ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool,
> NULL);
> > +ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +ASSERT (ChildResources != NULL);
> > +FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
> > +Mem32Bridge = ChildResources[0];
> >  ASSERT (Mem32Bridge  != NULL);
> > +
> > +ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool,
> NULL);
> > +ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +ASSERT (ChildResources != NULL);
> > +FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
> > +PMem32Bridge = ChildResources[0];
> >  ASSERT (PMem32Bridge != NULL);
> > +
> > +ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool,
> NULL);
> > +ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +ASSERT (ChildResources != NULL);
> > +FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
> > +Mem64Bridge = ChildResources[0];
> >  ASSERT (Mem64Bridge  != NULL);
> > +
> > +ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool,
> NULL);
> > +ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +ASSERT (ChildResourc

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

EFI_NOT_READY was listed as one of the error return values in the function
header of StartPciDevices(). So, I considered it here.

Maybe we can go by a dual approach, including CpuDeadLoop() in
StartPciDevices() as well as add return value check at the call site in
PciBusDriverBindingStart().

On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek  wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function StartPciDevices has a check
> >
> > ASSERT (RootBridge != NULL);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there
> > is no handling if the RootBridge value is NULL and the code proceeds
> > to unconditionally dereference "RootBridge" which will lead to CRASH.
> >
> > Hence, for safety add NULL pointer checks always and return
> > EFI_NOT_READY if RootBridge value is NULL which is one of the return
> > values as mentioned in the function description header.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > index 581e9075ad41..3de80d98370e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > @@ -772,7 +772,10 @@ StartPciDevices (
> >LIST_ENTRY *CurrentLink;
> >
> >RootBridge = GetRootBridgeByHandle (Controller);
> > -  ASSERT (RootBridge != NULL);
> > +  if (RootBridge == NULL) {
> > +return EFI_NOT_READY;
> > +  }
> > +
> >ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> >
> >CurrentLink = mPciDevicePool.ForwardLink;
>
> I don't think this is a good fix.
>
> There is one call site, namely in PciBusDriverBindingStart(). That call
> site does not check the return value. (Of course /s)
>
> I think that this ASSERT() can indeed never fail. Therefore I suggest
> CpuDeadLoop() instead.
>
> If you insist that CpuDeadLoop() is "too risky" here, then the patch is
> acceptable, but then the StartPciDevices() call site in
> PciBusDriverBindingStart() must check the error properly: we must not
> install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
> propagate the error outwards.
>
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111017): https://edk2.groups.io/g/devel/message/111017
Mute This Topic: https://groups.io/mt/102438320/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

I considered the least code change approach without considering any
optimization which also works as such.

However, yes, the duplicated /unnecessary subsequent checks can be removed.

On Tue, Nov 7, 2023 at 9:51 PM Laszlo Ersek  wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function PciHotPlugRequestNotify has two if blocks towards the end
> > of function both containing return. Due to the parameter checks ensured
> > at the beginning of the function, one of the two if blocks is bound to
> > come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
> > redundant/deadcode.
>
> Agree with the analysis.
>
> >
> > To fix it, either of line 2109 or 2112 can be deleted.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > index 3f8c6e6da7dc..5b71e152f3ea 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
> >  //
> >  // End for
> >  //
> > -return EFI_SUCCESS;
> >}
> >
> >return EFI_SUCCESS;
>
> Disagree with the fix.
>
> Given the checks on "Operation" at the top of the function, the
> condition (near the end of the function)
>
>   if (Operation == EfiPciHotplugRequestRemove) {
>
> will always evaluate to TRUE. (Operation can be only Add or Remove, and
> if it is Add, then we don't reach this location.)
>
> Therefore, we should remove this condition, and *unnest* the dependent
> logic.
>
> As a result of *that*, you'll have
>
>   return EFI_SUCCESS;
>   return EFI_SUCCESS;
>
> at the end of the function, and *then* you should remove either one of
> them.
>
> Thanks
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111018): https://edk2.groups.io/g/devel/message/111018
Mute This Topic: https://groups.io/mt/102438298/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

It was hard to conclude at my end as well what to do where. So I just threw
it open ...

- Status assignment can be ignored or
- Maintain the existing behaviour and just log the error given the original
code existence for quite a long now

Various files were clubbed together being part of the same module and all
having the same UNUSED_VALUE issue.
If you say so, I will split, but many lines are just cosmetic changes
(indentation level) - after impact of inclusion / removal of Status storage
/ Status value check.
Will need to figure out what is the best split though :-)

On Tue, Nov 7, 2023 at 10:50 PM Laszlo Ersek  wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > The return value after calls to functions
> > gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge,
> > PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write,
> > gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is
> > stored in Status, but it is not made of any use and later Status
> > gets overridden.
> >
> > Remove the return value storage in Status or add Status check as
> > seems appropriate at a particular point.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68
> +++-
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   |  8 +++
> >  3 files changed, 72 insertions(+), 46 deletions(-)
>
> First of all, please split up this patch. It's hard to review it like
> this, with unrelated pieces of logic lumped together.
>
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > index 3de80d98370e..9b0587c94d05 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > @@ -544,12 +544,12 @@ DeRegisterPciDevice (
> >EFI_OPEN_PROTOCOL_TEST_PROTOCOL
> >);
> >if (!EFI_ERROR (Status)) {
> > -Status = gBS->UninstallMultipleProtocolInterfaces (
> > -Handle,
> > -&gEfiLoadFile2ProtocolGuid,
> > -&PciIoDevice->LoadFile2,
> > -NULL
> > -);
> > +gBS->UninstallMultipleProtocolInterfaces (
> > +   Handle,
> > +   &gEfiLoadFile2ProtocolGuid,
> > +   &PciIoDevice->LoadFile2,
> > +   NULL
> > +   );
> >}
> >
> >//
>
> OK
>
> > @@ -678,19 +678,21 @@ StartPciDevicesOnBridge (
> > ChildHandleBuffer
> > );
> >
> > -PciIoDevice->PciIo.Attributes (
> > - &(PciIoDevice->PciIo),
> > - EfiPciIoAttributeOperationSupported,
> > - 0,
> > - &Supports
> > - );
> > -Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
> > -PciIoDevice->PciIo.Attributes (
> > - &(PciIoDevice->PciIo),
> > - EfiPciIoAttributeOperationEnable,
> > - Supports,
> > - NULL
> > - );
> > +if (!EFI_ERROR (Status)) {
> > +  PciIoDevice->PciIo.Attributes (
> > +   &(PciIoDevice->PciIo),
> > +   EfiPciIoAttributeOperationSupported,
> > +   0,
> > +   &Supports
> > +   );
> > +  Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
> > +  PciIoDevice->PciIo.Attributes (
> > +   &(PciIoDevice->PciIo),
> > +   EfiPciIoAttributeOperationEnable,
> > +   Supports,
> > +   NULL
> > +   );
> > +}
> >
> >  return Status;
> >} else {
>
> OK
>
> > @@ -726,19 +728,21 @@ StartPciDevicesOnBridge (
> > ChildHandleBuffer
> >

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek  wrote:

> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
See, Does the following change look acceptable to you ?

ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+CpuDeadLoop ();
+return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes
CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the
return statement afterwards will never reach execution (=> no need to
handle this return value at the call sites), but will satisfy static
analysis tools as the "RootBridge" dereference scenario doesn't arise
thereafter.


> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek  > <mailto:ler...@redhat.com>> wrote:
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function StartPciDevices has a check
> > >
> > > ASSERT (RootBridge != NULL);
> > >
> > > but this comes into play only in DEBUG mode. In Release mode, there
> > > is no handling if the RootBridge value is NULL and the code
> proceeds
> > > to unconditionally dereference "RootBridge" which will lead to
> CRASH.
> > >
> > > Hence, for safety add NULL pointer checks always and return
> > > EFI_NOT_READY if RootBridge value is NULL which is one of the
> return
> > > values as mentioned in the function description header.
> > >
> >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
> > >
> > > Cc: Ray Ni mailto:ray...@intel.com>>
> > > Co-authored-by: Veeresh Sangolli  > <mailto:veeresh.sango...@dellteam.com>>
> > > Signed-off-by: Ranbir Singh 
> > > Signed-off-by: Ranbir Singh  > <mailto:rsi...@ventanamicro.com>>
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 -
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > index 581e9075ad41..3de80d98370e 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > > @@ -772,7 +772,10 @@ StartPciDevices (
> > >LIST_ENTRY *CurrentLink;
> > >
> > >RootBridge = GetRootBridgeByHandle (Controller);
> > > -  ASSERT (RootBridge != NULL);
> > > +  if (RootBridge == NULL) {
> > > +return EFI_NOT_READY;
> > > +  }
> > > +
> > >ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> > >
> > >CurrentLink = mPciDevicePool.ForwardLink;
> >
> > I don't think this is a good fix.
> >
> > There is one call site, namely in PciBusDriverBindingStart(). That
> call
> > site does not check the return value. (Of course /s)
> >
> > I think that this ASSERT() can indeed never fail. Therefore I suggest
> > CpuDeadLoop() instead.
> >
> > If you insist that CpuDeadLoop() is "too risky" here, then the patch
> is
> > acceptable, but then the StartPciDevices() call site in
> > PciBusDriverBindingStart() must check the error properly: we must not
> > install "gEfiPciEnumerationCompleteProtocolGuid", and the function
> must
> > propagate the error outwards.
> >
> > Laszlo
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111202): https://edk2.groups.io/g/devel/message/111202
Mute This Topic: https://groups.io/mt/102438320/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

On Mon, Nov 13, 2023 at 10:03 PM Laszlo Ersek  wrote:

> On 11/9/23 18:39, Ranbir Singh wrote:
> > From: Ranbir Singh 
> >
> > The function SubmitResources has a switch-case code in which the
> > case ACPI_ADDRESS_SPACE_TYPE_MEM: which falls through to
> > case ACPI_ADDRESS_SPACE_TYPE_IO: to include additional common check.
> >
> > 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.
> >
> > SubmitResources function is supposed to handle only Mem or IO resources.
> > So, update the ResType parameter check reflecting that and re-model the
> > switch-case code in contention using just one if condition to further
> > handle other parameter checks specific to ACPI_ADDRESS_SPACE_TYPE_MEM.
> > This leads to mostly indentation level code changes. Few ASSERT's later
> > present are henceforth not required.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4212
> >
> > Cc: Ray Ni 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 60
> +---
> >  1 file changed, 26 insertions(+), 34 deletions(-)
>
> I have applied this patch locally, and displayed it with
>
>   git show -W -b
>
> Let me make comments on that:
>
> >  /**
> >
> >Submits the I/O and memory resource requirements for the specified
> PCI Root Bridge.
> >
> >@param This  The EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_
> PROTOCOL instance.
> > -  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements.
> > +  @param RootBridgeHandle  The PCI Root Bridge whose I/O and memory
> resource requirements
> > are being submitted.
> >@param Configuration The pointer to the PCI I/O and PCI memory
> resource descriptor.
> >
> >@retval EFI_SUCCESSSucceed.
> >@retval EFI_INVALID_PARAMETER  Wrong parameters passed in.
> >  **/
> > @@ -1460,137 +1460,129 @@ EFIAPI
> >  SubmitResources (
> >IN EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL  *This,
> >IN EFI_HANDLERootBridgeHandle,
> >IN VOID  *Configuration
> >)
> >  {
> >LIST_ENTRY *Link;
> >PCI_HOST_BRIDGE_INSTANCE   *HostBridge;
> >PCI_ROOT_BRIDGE_INSTANCE   *RootBridge;
> >EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR  *Descriptor;
> >PCI_RESOURCE_TYPE  Type;
> >
> >//
> >// Check the input parameter: Configuration
> >//
> >if (Configuration == NULL) {
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> >HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
> >for (Link = GetFirstNode (&HostBridge->RootBridges)
> > ; !IsNull (&HostBridge->RootBridges, Link)
> > ; Link = GetNextNode (&HostBridge->RootBridges, Link)
> > )
> >{
> >  RootBridge = ROOT_BRIDGE_FROM_LINK (Link);
> >  if (RootBridgeHandle == RootBridge->Handle) {
> >DEBUG ((DEBUG_INFO, "PciHostBridge: SubmitResources for %s\n",
> RootBridge->DevicePathStr));
> >//
> >// Check the resource descriptors.
> >// If the Configuration includes one or more invalid resource
> descriptors, all the resource
> >// descriptors are ignored and the function returns
> EFI_INVALID_PARAMETER.
> >//
> >for (Descriptor = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR
> *)Configuration; Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR;
> Descriptor++) {
> > -if (Descriptor->ResType > ACPI_ADDRESS_SPACE_TYPE_BUS) {
> > +if ((Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_MEM) &&
> (Descriptor->ResType != ACPI_ADDRESS_SPACE_TYPE_IO)) {
> >return EFI_INVALID_PARAMETER;
> >  }
>
> This is a slight logic change.
>
> Previously, the code did the following:
>
> - Any resource types that were different from MEM, IO, and BUS, would be
>   rejected with EFI_INVALID_PARAMETER.
>
> - MEM and IO would be handled.
>
> - BUS resource types would trigger an ASSERT().
>
> In effect, the code claimed that BUS resource types were *impossible*
> here, due to prior filtering or whatever.
>
> The logic change is that now we reject BUS resource types explicitly.
>
> I think that's not ideal. Here's why

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

[Ranbir Singh]

On Mon, Nov 13, 2023 at 9:42 PM Laszlo Ersek  wrote:

> On 11/10/23 05:07, Ranbir Singh wrote:
> > Options before us till now -
> >
> > 1. Add array overrun check and Debug statement before CpuDeadLoop within
>
> This would be useless.
>
> Your current patch implements the following pattern:
>
>   ASSERT (X);
>   if (!X) {
> CpuDeadLoop ();
>   }
>
> Option#1 would mean
>
>   ASSERT (X);
>   if (!X) {
> DEBUG ((DEBUG_ERROR, "%a: condition X failed\n", __func__));
> CpuDeadLoop ();
>   }
>
> Now compare the behavior of both code snippets.
>
> - In DEBUG and NOOPT builds, if X evaluated to FALSE, then the ASSERT()
> would fire. A DEBUG_ERROR message would be logged, including "X", the
> file name, and the line number. ASSERT() would then enter CpuDeadLoop()
> internally, or invoke CpuBreakpoint() -- dependent on platform PCD
> configuration. This applies to both snippets. In particular, the
> explicit DEBUG and CpuDeadLoop() would not be reached, in the second
> snippet. In other words, in DEBUG and NOOPT builds, the difference is
> unobservable.
>
> - In RELEASE builds, the ASSERT() is compiled out of both snippets.
> Furthermore, the explicit DEBUG is compiled out of the second snippet.
> That is, both code snippets would silently enter CpuDeadLoop(). In other
> words, the behavior of both snippets is undistinguishable in RELEASE
> builds too.
>
> In my opinion, the current patch is right.
>
> Reviewed-by: Laszlo Ersek 
>
>
> To elaborate on that more generally:
>
> Core edk2 code has so consistently and so broadly *abused* and *misused*
> ASSERT() for error checking, that now we fail to recognize an *actual
> valid use* of ASSERT() for what it is. For illustration, compare the
> following two code snippets:
>
> (a)
>
>   UINTN Val;
>
>   Val = 1 + 2;
>   ASSERT (Val == 3);
>
> (b)
>
>   VOID *Ptr;
>
>   Ptr = AllocatePool (1024);
>   ASSERT (Ptr != NULL);
>
> Snippet (a) is a *valid* use of an assert. It encapsulates a logical
> predicate about the program's state, based on algorithmic and
> mathematical reasoning. If the predicate turns out not to hold in
> practice, at runtime,that means the programmer has retroactively caught
> an issue in their own thinking, the algorithm is incorrectly designed or
> implemented, and the program's state is effectively unknown at this
> point (the internal invariant in question is broken), and so we must
> stop immediately, because we don't know what the program is going to do
> next. Given that what we thought about the program *thus far* has now
> turned out to be false.
>
> Snippet (b) is an abuse of assert. AllocatePool()'s result depends on
> the environment. Available memory, input data seen from the user or the
> disk or the network thus far, and a bunch of other things not under our
> control. Therefore, we must explicitly deal with AllocatePool() failing.
> Claiming that AllocatePool() will succeed is wrong because we generally
> cannot even *attempt* to prove it.
>
> In case (a) however, we can actually make a plausible attempt at
> *proving* the predicate. The assert is there just in case our proof is
> wrong.
>
> There's an immense difference between situations (a) and (b). I cannot
> emphasize that enough. Case (a) is a provable statement about the
> behavior of an algorithm. Case (b) is a *guess* at input data and
> environment.
>
> The problem with edk2 core is that it has so consistently abused
> ASSERT() for case (b) that now, when we occasionally see a *valid
> instance* of (a), we mistake it for (b).
>
> That's the case here. The ASSERT() seen in this patch is case (a). It's
> just that Coverity cannot prove it itself, because the predicate we
> assert is much more complicated than (1 + 2 == 3). But that doesn't
> change the fact that we have a proof for the invariant in question.
>
> Therefore, the solution is not to try to handle an impossible error
> gracefully. The solution is two-fold:
>
> - Tell coverity that we have a proof, in terms that it understands, to
> shut it up. The terminology for this is CpuDeadLoop(). We're willing to
> hang at runtime even in RELEASE builds, if the condition fails.
>
> - If, at runtime, our proof turns out to be wrong indeed, then we must
> stop immediately (because if 1+2 stops equalling 3, then we can't trust
> *anything else* that our program would do).
>
> (The more generic corollary of my last point, by the way, is that
> ASSERT()s should NEVER be compiled out of programs. And if we actually
> did that, like many other projects do, then this Coverity warning
> wouldn't ev

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

On Tue, Nov 14, 2023 at 9:51 PM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:

> Hi Ranbir,
>
>
>
> First I want to recognize your efforts to collect Coverity issues and
> propose changes to address
>
> them.
>

Thanks Mike.


>
>
> I still disagree with adding CpuDealLoop() for any static analysis issues.
>

A bit of correction here.
CpuDeadLoop() is not exactly an addition for static analysis issues. For
that, the NULL pointer check and return statement in the if block are
sufficient.
However, CpuDeadLoop() is added as suggested by Laszlo to introduce a hang
behaviour in RELEASE builds as well when the situation is anyway not safe
to progress normally. That said, it may not still be required at every
point and hence needs to be assessed on a case to case basis.


>
> There have been previous discussions about adding a PANIC() or FATAL()
> macro that would
>
> perform platform specific actions if a condition is detected where the
> boot of the platform
>
> can not continue.  A platform get to make the choice to log or reboot or
> hang, etc.  Not the
>
> code that detected the condition.
>
>
>
> https://edk2.groups.io/g/devel/message/86597
>
>
>
> Unfortunately, in order to fix some of these static analysis issues
> correctly, we are going
>
> to have to identify the use of ASSERT() that really is a fatal condition
> that can not continue
>
> and introduce an implementation approach that provides a platform handler
> and
>
> satisfies the static analysis tools.
>
>
>
> We also have to evaluate if a return error status with a DEBUG_ERROR
> message would be a better
>
> choice than an ASSERT() that can be filtered out by build options.
>

Note that the existing ASSERT will give a DEBUG message even when
CpuDeadLoop is added in the code later.


>
>
> Best regards,
>
>
>
> Mike
>
>
>

Generally speaking, there now seems to be different views coming from you
and Laszlo. We might have to wait for some sort of agreement to be reached.


>
>
> *From:* devel@edk2.groups.io  * On Behalf Of *Ranbir
> Singh
> *Sent:* Tuesday, November 14, 2023 7:08 AM
> *To:* Laszlo Ersek 
> *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli <
> veeresh.sango...@dellteam.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>
>
>
>
>
>
>
> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek  wrote:
>
> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
>
> See, Does the following change look acceptable to you ?
>
>
>
> ASSERT (RootBridge != NULL);
> +  if (RootBridge == NULL) {
>
> +CpuDeadLoop ();
> +return EFI_NOT_READY;
> +  }
>
> +
>
>
>
> which retains the existing assert, adds the NULL pointer check and
> includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (),
> the return statement afterwards will never reach execution (=> no need to
> handle this return value at the call sites), but will satisfy static
> analysis tools as the "RootBridge" dereference scenario doesn't arise
> thereafter.
>
>
>
>
> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek  > <mailto:ler...@redhat.com>> wrote:
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh 
> > >
> > > The function StartPciDevices has a check
> > >
> > > ASSERT (RootBridge != NULL);
> > >
> > > but this comes into play only in DEBUG mode. In Release mode

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek  wrote:

> On 11/14/23 17:21, Kinney, Michael D wrote:
> > Hi Ranbir,
> >
> >
> >
> > First I want to recognize your efforts to collect Coverity issues and
> > propose changes to address
> > them.
> >
> > I still disagree with adding CpuDealLoop() for any static analysis
> issues.
> >
> > There have been previous discussions about adding a PANIC() or FATAL()
> > macro that would
> > perform platform specific actions if a condition is detected where the
> > boot of the platform
> > can not continue.  A platform get to make the choice to log or reboot or
> > hang, etc.  Not the
> > code that detected the condition.
> >
> > https://edk2.groups.io/g/devel/message/86597
> > 
>
> This is indeed a great idea.
>
> I didn't know about that discussion. Perhaps a new DebugLib API would
> handle this (i.e., "panic").
>
> I've been certainly proposing CpuDeadLoop() as a means to panic.
>
> Did the thread conclude anything tangible?
>
> > Unfortunately, in order to fix some of these static analysis issues
> > correctly, we are going
> > to have to identify the use of ASSERT() that really is a fatal condition
> > that can not continue
>
> Absolutely.
>
> > and introduce an implementation approach that provides a platform
> > handler and
> > satisfies the static analysis tools.
>
> The "platform handler" is the difficult part. If the above-noted
> discussion from 2022 didn't produce an agreement, should we really block
> the static analyzer fixes on an agreed-upon panic API? I'm concerned
> that would just cause these fixes to get stuck. And I don't consider
> CpuDeadLoop()s added for this purpose serious technical debt. They are
> easy to find and update later, assuming we also add comments.
>
>
I agree with the approach to not gate current fixes adding CpuDeadLoop().
Later on, it can be updated with the desired panic API and I can contribute
for those required changes related to patches submitted by me.

I can update current patches to carry additional comment in suffix form to
ease later search like
CpuDeadLoop (); // TBD: replace with Panic API in future

Laszlo, Mike - Let me know if that works for now.


> > We also have to evaluate if a return error status with a DEBUG_ERROR
> > message would be a better
> > choice than an ASSERT() that can be filtered out by build options.
>
> I agree 100% that this would be better for the codebase, but the work
> needed for this is (IMO) impossible to contain. ASSERT() has been abused
> for a long time *because* it seemed to allow the programmer to ignore
> any related error paths. If we now want to implement those error paths
> retroactively (which would be absolutely the right thing to do from a
> software engineering perspective), then immense amounts of work are
> going to be needed (patch review and regression testing), and I think
> it's just not practical to dump all that on someone that wants to
> improve the status quo. Replacing an invalid ASSERT() with a panic is
> honest about the current situation, is safer regarding RELEASE builds,
> and its work demand (regression testing, patch review) is tolerable.
>
> I do agree that, if the error path mostly exists already, then returning
> errors for data/environment-based error conditions (i.e., not for
> algorithmic invariant failures) is best.
>
> Where we need to be extremely vigilant is new patches. We must
> uncompromisingly reject *new* abuses of ASSERT(), in new patches.
>
> Anyway, it seems that we've been trying to steer Ranbir in opposite
> directions. I'll let you take the lead on this; for one, I've not been
> aware of the panic api discussion for 2022!
>
> (I don't feel especially pushy about fixing coverity issues, it's just
> that Ranbir has been contributing such patches, which I've found very
> welcome, and I wanted to help out with reviews.)
>
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111444): https://edk2.groups.io/g/devel/message/111444
Mute This Topic: https://groups.io/mt/102438320/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

[Ranbir Singh]

On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A  wrote:

> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ; Veeresh
> > Sangolli 
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The function UhciConvertPollRate has a check
> >
> > ASSERT (Interval != 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.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > index c08f9496969b..8ddef4b68ccf 100644
> > --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > @@ -214,6 +214,10 @@ UhciConvertPollRate (
> >
> >
> >ASSERT (Interval != 0);
> >
> >
> >
> > +  if (Interval == 0) {
> >
> > +return 0;
>
>
> Return 0 will cause further issues within UhciLinkQhToFrameList() &
> UhciUnlinkQhFromFrameList() where the returned value is being used in the
> below
> 'for' statement:
>
> for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) {
>
>
One way is to prohibit UhciCreateQh() to proceed normally by checking if
Interval parameter value is 0 at the very beginning and may be add a DEBUG
statement and/or an ASSERT as well - return value then has to be NULL from
this function for this specific case of invalid Interval parameter value
being received as 0. That should take care of the issue Hao pointed above
in for loop.

UhciCreateAsyncReq() may also need to introduce a Polling Interval
parameter value check similar to as it exists in
Uhci2AsyncInterruptTransfer() for a future direct call scenario.


> My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for
> 'Interval' (which is 1) is being passed into this UhciConvertPollRate
> function.
>

If we choose to modify UhciCreateQh(), then we may have status quo
in UhciConvertPollRate(). However, we can still pursue the return 1 (i.e. 1
<< 0) option here in which case I guess I should update the function header
as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is
wrongly sent, it will be treated as 1 only. *When we do that in RELEASE
mode, should we still retain ASSERT ?* I think NO.

If we choose to simply modify UhciConvertPollRate() as stated, then we
can have the status quo in UhciCreateQh() and UhciCreateAsyncReq().


> Best Regards,
> Hao Wu
>
>
> >
> > +  }
> >
> > +
> >
> >//
> >
> >// Find the index (1 based) of the highest non-zero bit
> >
> >//
> >
> > --
> > 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107727): https://edk2.groups.io/g/devel/message/107727
Mute This Topic: https://groups.io/mt/100212109/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

[Ranbir Singh]

When code is compiled in RELEASE mode, the ASSERT (Interval != 0);
statement gets NULL'ified. Hence for Coverity it simply doesn't exist.
Further, IMO Coverity seems to look at a function in isolation whether it
is safe or not. It is however not necessary to fix all issues pointed out
by Coverity if something looks to be a false positive or something is done
deliberately.

I will go by returning 1 and can still keep ASSERT if you prefer that way.
I will put a change post ASSERT statement only, so that DEBUG mode still
catches if wrongly '0' is sent.

Should I do this or not - update the function header as well to reflect the
minimum 'Interval' valid value as 1 and that if 0 is wrongly sent, it will
be treated as 1 only in RELEASE mode ?

On Mon, Aug 14, 2023 at 2:09 PM Wu, Hao A  wrote:

> My take is that:
>
> For all the possible calling scenario of UhciConvertPollRate() in current
>
> UhciDxe driver implementation, it is guaranteed that the input parameter
>
> 'Interval' will not be 0.
>
>
>
> I think this is why the "ASSERT (Interval != 0);" statement is put here to
>
> indicate such case will never happen and notify developers for future
> changes
>
> in this driver that something is wrong.
>
>
>
> I do not know why Coverity is unable to figure this out.
>
>
>
> My personal preference is to return 1 (i.e. 1 << 0) as if the minimum
> allowed
>
> value for 'Interval' (which is 1) is being passed into this
> UhciConvertPollRate
>
> function if anything does go wrong brought by future changes.
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Ranbir Singh 
> *Sent:* Monday, August 14, 2023 3:49 PM
> *To:* Wu, Hao A 
> *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli <
> veeresh.sango...@dellteam.com>
> *Subject:* Re: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> Coverity issue
>
>
>
>
>
> On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A  wrote:
>
> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ; Veeresh
> > Sangolli 
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The function UhciConvertPollRate has a check
> >
> > ASSERT (Interval != 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.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > index c08f9496969b..8ddef4b68ccf 100644
> > --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
> > @@ -214,6 +214,10 @@ UhciConvertPollRate (
> >
> >
> >ASSERT (Interval != 0);
> >
> >
> >
> > +  if (Interval == 0) {
> >
> > +return 0;
>
>
> Return 0 will cause further issues within UhciLinkQhToFrameList() &
> UhciUnlinkQhFromFrameList() where the returned value is being used in the
> below
> 'for' statement:
>
> for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) {
>
>
>
> One way is to prohibit UhciCreateQh() to proceed normally by checking if
> Interval parameter value is 0 at the very beginning and may be add a DEBUG
> statement and/or an ASSERT as well - return value then has to be NULL from
> this function for this specific case of invalid Interval parameter value
> being received as 0. That should take care of the issue Hao pointed above
> in for loop.
>
>
>
> UhciCreateAsyncReq() may also need to introduce a Polling Interval
> parameter value check similar to as it exists in
> Uhci2AsyncInterruptTransfer() for a future direct call scenario.
>
>
>
> My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for
> 'Interval' (which is 1) is being passed into this UhciConvertPollRate
> function.
>
>
>
> If we choose to modify UhciCrea

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

[Ranbir Singh]

v1 -> v2:
  - Update patch wrt BAD_SHIFT changes as per review comments

Ranbir Singh (2):
  MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
  MdeModulePkg/Bus/Pci/UhciDxe: Fix FORWARD_NULL Coverity issues

 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 10 +-
 MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c  |  9 +
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107788): https://edk2.groups.io/g/devel/message/107788
Mute This Topic: https://groups.io/mt/100774223/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function UhciConvertPollRate has a check

ASSERT (Interval != 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 treating it same as if 1 is sent.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c 
b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
index c08f9496969b..408e7d5ab7f3 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c
@@ -197,7 +197,7 @@ UhciDestoryFrameList (
 }
 
 /**
-  Convert the poll rate to the maxium 2^n that is smaller
+  Convert the poll rate to the maximum 2^n that is smaller
   than Interval.
 
   @param  Interval   The poll rate to convert.
@@ -214,6 +214,14 @@ UhciConvertPollRate (
 
   ASSERT (Interval != 0);
 
+  //
+  // To safeguard RELEASE mode wherein ASSERT is effectively not there,
+  // if inadvertently Interval is still 0 here, treat it the same as 1.
+  //
+  if (Interval == 0) {
+Interval = 1;
+  }
+
   //
   // Find the index (1 based) of the highest non-zero bit
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107789): https://edk2.groups.io/g/devel/message/107789
Mute This Topic: https://groups.io/mt/100774224/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

The function UsbHcGetPciAddressForHostMem has

  ASSERT ((Block != NULL)); and

and the function UsbHcFreeMem has

  ASSERT (Block != NULL);

statement 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=4211

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
Reviewed-by: Hao A Wu 
---
 MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c 
b/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c
index c3d46f60bed5..3794f888e132 100644
--- a/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c
@@ -250,6 +250,11 @@ UsbHcGetPciAddressForHostMem (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -536,6 +541,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 (#107790): https://edk2.groups.io/g/devel/message/107790
Mute This Topic: https://groups.io/mt/100774225/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix issues pointed by Coverity

[Ranbir Singh]

Soft reminder to have a look at the series

On Mon, Jul 17, 2023 at 5:24 PM Ranbir Singh 
wrote:

> Ranbir Singh (2):
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix OVERRUN Coverity issues
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe: Fix MISSING_BREAK Coverity
> issue
>
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 9 +
>  1 file changed, 9 insertions(+)
>
> --
> 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109036): https://edk2.groups.io/g/devel/message/109036
Mute This Topic: https://groups.io/mt/100212111/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 0/5] BZ 4219: MdeModulePkg/Core/Dxe: Fix issues

[Ranbir Singh]

Ranbir Singh (5):
  MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues
  MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue
  MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues
  MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c |  2 ++
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +
 MdeModulePkg/Core/Dxe/Event/Event.c   |  4 +--
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   | 19 +++
 MdeModulePkg/Core/Dxe/Image/Image.c   |  3 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 35 +---
 6 files changed, 57 insertions(+), 21 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109059): https://edk2.groups.io/g/devel/message/109059
Mute This Topic: https://groups.io/mt/101590752/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/5] MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The functions CoreConvertSpace and CoreAllocateSpace in

MdeModulePkg/Core/Dxe/Gcd/Gcd.c has

ASSERT (FALSE); at lines 755 and 1155 which gets hit when

Operation neither include GCD_MEMORY_SPACE_OPERATION nor include
GCD_IO_SPACE_OPERATION but this comes into play only in DEBUG mode.
In Release mode, the code continues to proceed in this undesirable
case with Map variable still set to NULL and hence dereferencing
"Map" will lead to CRASH.

It is safer to add a debug message in this scenario and return from
the function with EFI_INVALID_PARAMETER; The existing ASSERT may be
retained or may be deleted whatever is deemed more appropriate.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 792cd2e0af23..39fa2adf9366 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -752,7 +752,9 @@ CoreConvertSpace (
 CoreAcquireGcdIoLock ();
 Map = &mGcdIoSpaceMap;
   } else {
+DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
 ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1152,7 +1154,9 @@ CoreAllocateSpace (
 CoreAcquireGcdIoLock ();
 Map = &mGcdIoSpaceMap;
   } else {
+DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
 ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
   }
 
   Found = FALSE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109060): https://edk2.groups.io/g/devel/message/109060
Mute This Topic: https://groups.io/mt/101590754/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/5] MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function CoreIsSchedulable has switch-case code in which

   case EFI_DEP_BEFORE:
   case EFI_DEP_AFTER:

has a comment that

// For a well-formed Dependency Expression, the code should never get here.

It also has an ASSERT (FALSE); but this is applicable only in DEBUG
mode. Seemingly, for RELEASE mode, code should not be allowed to fall
through to case EFI_DEP_SOR: which is below the above two.

Hence, add return FALSE at the end.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c 
b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
index acbf68b700fb..9799ec9ca097 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
@@ -265,6 +265,8 @@ CoreIsSchedulable (
 //
 DEBUG ((DEBUG_DISPATCH, "  RESULT = FALSE (Unexpected BEFORE or AFTER 
opcode)\n"));
 ASSERT (FALSE);
+return FALSE;
+
   case EFI_DEP_SOR:
 //
 // These opcodes can only appear once as the first opcode.  If it is 
found
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109061): https://edk2.groups.io/g/devel/message/109061
Mute This Topic: https://groups.io/mt/101590755/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 3/5] MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

In the function PromoteGuardedFreePages(), the value of AvailablePages
cannot be ZERO at the condition check point

  if (AvailablePages != 0) {

as the code can come out of the while loop above only when AvailablePages
is non-ZERO.

Hence, remove the redundant condition check and the return FALSE;
DEADCODE statement at the end of the function.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 35 +---
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 0c0ca61872b4..016791ee002b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -1568,28 +1568,25 @@ PromoteGuardedFreePages (
 }
   }
 
-  if (AvailablePages != 0) {
-DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, 
(UINT64)AvailablePages));
-ClearGuardedMemoryBits (Start, AvailablePages);
+  DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, 
(UINT64)AvailablePages));
+  ClearGuardedMemoryBits (Start, AvailablePages);
 
-if (gCpu != NULL) {
-  //
-  // Set flag to make sure allocating memory without GUARD for page table
-  // operation; otherwise infinite loops could be caused.
-  //
-  mOnGuarding = TRUE;
-  Status  = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE 
(AvailablePages), 0);
-  ASSERT_EFI_ERROR (Status);
-  mOnGuarding = FALSE;
-}
-
-mLastPromotedPage = Start;
-*StartAddress = Start;
-*EndAddress   = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
-return TRUE;
+  if (gCpu != NULL) {
+//
+// Set flag to make sure allocating memory without GUARD for page table
+// operation; otherwise infinite loops could be caused.
+//
+mOnGuarding = TRUE;
+Status  = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE 
(AvailablePages), 0);
+ASSERT_EFI_ERROR (Status);
+mOnGuarding = FALSE;
   }
 
-  return FALSE;
+  mLastPromotedPage = Start;
+  *StartAddress = Start;
+  *EndAddress   = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
+
+  return TRUE;
 }
 
 /**
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109062): https://edk2.groups.io/g/devel/message/109062
Mute This Topic: https://groups.io/mt/101590756/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 4/5] MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

"1 << Priority" / "1 << Event->NotifyTpl" are potentially overflowing
expressions with type "int" (32 bits, signed) evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "UINTN" (64 bits, unsigned).

To avoid overflow, cast "1" to type "UINTN" before << operation.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Event/Event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c 
b/MdeModulePkg/Core/Dxe/Event/Event.c
index dc82abb02130..6cf93f7f562d 100644
--- a/MdeModulePkg/Core/Dxe/Event/Event.c
+++ b/MdeModulePkg/Core/Dxe/Event/Event.c
@@ -191,7 +191,7 @@ CoreDispatchEventNotifies (
 CoreAcquireEventLock ();
   }
 
-  gEventPending &= ~(UINTN)(1 << Priority);
+  gEventPending &= ~(UINTN)((UINTN)1 << Priority);
   CoreReleaseEventLock ();
 }
 
@@ -225,7 +225,7 @@ CoreNotifyEvent (
   //
 
   InsertTailList (&gEventQueue[Event->NotifyTpl], &Event->NotifyLink);
-  gEventPending |= (UINTN)(1 << Event->NotifyTpl);
+  gEventPending |= (UINTN)((UINTN)1 << Event->NotifyTpl);
 }
 
 /**
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109063): https://edk2.groups.io/g/devel/message/109063
Mute This Topic: https://groups.io/mt/101590757/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 5/5] MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

The return value after calls to functions CoreProcessFvImageFile,
CoreStartImage,  CoreGetDepexSectionAndPreProccess,
CoreInternalAddMemorySpace, CoreAddIoSpace, CoreAllocateMemorySpace
and CoreCloseProtocol is stored in Status, but it is not made of any
use and later Status gets overridden.

One option assuming this is deliberate, would be to remove the return
value storage in Status.
Otherwise, simply add appropriate debug messages conditionally.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   | 15 +++
 MdeModulePkg/Core/Dxe/Image/Image.c   |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c 
b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
index cf9d55687766..f53a2513457a 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
@@ -506,6 +506,10 @@ CoreDispatcher (
 // Produce a firmware volume block protocol for FvImage so it gets 
dispatched from.
 //
 Status = CoreProcessFvImageFile (DriverEntry->Fv, 
DriverEntry->FvHandle, &DriverEntry->FileName);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_DISPATCH, "Failed to produce a FVB protocol for Fv %p 
FvHandle %p and FileName %g.\n",
+DriverEntry->Fv, DriverEntry->FvHandle, &DriverEntry->FileName));
+}
   } else {
 REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
   EFI_PROGRESS_CODE,
@@ -516,6 +520,10 @@ CoreDispatcher (
 ASSERT (DriverEntry->ImageHandle != NULL);
 
 Status = CoreStartImage (DriverEntry->ImageHandle, NULL, NULL);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_DISPATCH, "Failed to transfer control to a loaded 
image's entry point for ImageHandle %p.\n",
+DriverEntry->ImageHandle));
+}
 
 REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
   EFI_PROGRESS_CODE,
@@ -549,6 +557,13 @@ CoreDispatcher (
 // If Section Extraction Protocol did not let the Depex be read before 
retry the read
 //
 Status = CoreGetDepexSectionAndPreProccess (DriverEntry);
+if (EFI_ERROR (Status)) {
+  if (Status == EFI_PROTOCOL_ERROR) {
+DEBUG ((DEBUG_DISPATCH, "Section extraction protocol failure for 
DriverEntry %p.\n", DriverEntry));
+  } else {
+DEBUG ((DEBUG_DISPATCH, "No Depex, assume UEFI 2.0 driver model 
for DriverEntry %p.\n", DriverEntry));
+  }
+}
   }
 
   if (DriverEntry->Dependent) {
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 39fa2adf9366..384fee600d85 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2638,6 +2638,9 @@ CoreInitializeGcdServices (
ResourceHob->ResourceLength,
Capabilities
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to add a segment of memory to GCD map, 
Status = %r\n", Status));
+}
   }
 
   if (GcdIoType != EfiGcdIoTypeNonExistent) {
@@ -2646,6 +2649,9 @@ CoreInitializeGcdServices (
ResourceHob->PhysicalStart,
ResourceHob->ResourceLength
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to add reserved I/O or I/O resources, 
Status = %r\n", Status));
+}
   }
 }
   }
@@ -2668,6 +2674,9 @@ CoreInitializeGcdServices (
gDxeCoreImageHandle,
NULL
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+}
   }
 
   //
@@ -2715,6 +2724,9 @@ CoreInitializeGcdServices (
 gDxeCoreImageHandle,
 NULL
 );
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+  }
 }
   }
 
@@ -2763,6 +2775,9 @@ CoreInitializeGcdServices (
gDxeCoreImageHandle,
NULL
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+}
   }
 }
   }
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 9dbfb2a1fad2..769e2d379051 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1010,6 +1010,9 @@ CoreUnloadAndCloseImage (
  Image->Handle,

[edk2-devel] [PATCH v2 0/5] BZ 4219: MdeModulePkg/Core/Dxe: Fix issues

[Ranbir Singh]

v1 -> v2:
  - Rebased
  - Commit message update wrt Fix UNUSED_VALUE Coverity issues

Ranbir Singh (5):
  MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues
  MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue
  MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues
  MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c |  2 ++
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +
 MdeModulePkg/Core/Dxe/Event/Event.c   |  4 +--
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   | 19 +++
 MdeModulePkg/Core/Dxe/Image/Image.c   |  3 ++
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 35 +---
 6 files changed, 57 insertions(+), 21 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109101): https://edk2.groups.io/g/devel/message/109101
Mute This Topic: https://groups.io/mt/101612675/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/5] MdeModulePkg/Core/Dxe: Fix MISSING_BREAK Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function CoreIsSchedulable has switch-case code in which

   case EFI_DEP_BEFORE:
   case EFI_DEP_AFTER:

has a comment that

// For a well-formed Dependency Expression, the code should never get here.

It also has an ASSERT (FALSE); but this is applicable only in DEBUG
mode. Seemingly, for RELEASE mode, code should not be allowed to fall
through to case EFI_DEP_SOR: which is below the above two.

Hence, add return FALSE at the end.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c 
b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
index acbf68b700fb..9799ec9ca097 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dependency.c
@@ -265,6 +265,8 @@ CoreIsSchedulable (
 //
 DEBUG ((DEBUG_DISPATCH, "  RESULT = FALSE (Unexpected BEFORE or AFTER 
opcode)\n"));
 ASSERT (FALSE);
+return FALSE;
+
   case EFI_DEP_SOR:
 //
 // These opcodes can only appear once as the first opcode.  If it is 
found
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109103): https://edk2.groups.io/g/devel/message/109103
Mute This Topic: https://groups.io/mt/101612677/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/5] MdeModulePkg/Core/Dxe: Fix FORWARD_NULL Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The functions CoreConvertSpace and CoreAllocateSpace in

MdeModulePkg/Core/Dxe/Gcd/Gcd.c has

ASSERT (FALSE); at lines 755 and 1155 which gets hit when

Operation neither include GCD_MEMORY_SPACE_OPERATION nor include
GCD_IO_SPACE_OPERATION but this comes into play only in DEBUG mode.
In Release mode, the code continues to proceed in this undesirable
case with Map variable still set to NULL and hence dereferencing
"Map" will lead to CRASH.

It is safer to add a debug message in this scenario and return from
the function with EFI_INVALID_PARAMETER; The existing ASSERT may be
retained or may be deleted whatever is deemed more appropriate.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 792cd2e0af23..39fa2adf9366 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -752,7 +752,9 @@ CoreConvertSpace (
 CoreAcquireGcdIoLock ();
 Map = &mGcdIoSpaceMap;
   } else {
+DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
 ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
   }
 
   //
@@ -1152,7 +1154,9 @@ CoreAllocateSpace (
 CoreAcquireGcdIoLock ();
 Map = &mGcdIoSpaceMap;
   } else {
+DEBUG ((DEBUG_GCD, "  Status = %r\n", EFI_INVALID_PARAMETER));
 ASSERT (FALSE);
+return EFI_INVALID_PARAMETER;
   }
 
   Found = FALSE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109102): https://edk2.groups.io/g/devel/message/109102
Mute This Topic: https://groups.io/mt/101612676/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/5] MdeModulePkg/Core/Dxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

In the function PromoteGuardedFreePages(), the value of AvailablePages
cannot be ZERO at the condition check point

  if (AvailablePages != 0) {

as the code can come out of the while loop above only when AvailablePages
is non-ZERO.

Hence, remove the redundant condition check and the return FALSE;
DEADCODE statement at the end of the function.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 35 +---
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 0c0ca61872b4..016791ee002b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -1568,28 +1568,25 @@ PromoteGuardedFreePages (
 }
   }
 
-  if (AvailablePages != 0) {
-DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, 
(UINT64)AvailablePages));
-ClearGuardedMemoryBits (Start, AvailablePages);
+  DEBUG ((DEBUG_INFO, "Promoted pages: %lX (%lx)\r\n", Start, 
(UINT64)AvailablePages));
+  ClearGuardedMemoryBits (Start, AvailablePages);
 
-if (gCpu != NULL) {
-  //
-  // Set flag to make sure allocating memory without GUARD for page table
-  // operation; otherwise infinite loops could be caused.
-  //
-  mOnGuarding = TRUE;
-  Status  = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE 
(AvailablePages), 0);
-  ASSERT_EFI_ERROR (Status);
-  mOnGuarding = FALSE;
-}
-
-mLastPromotedPage = Start;
-*StartAddress = Start;
-*EndAddress   = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
-return TRUE;
+  if (gCpu != NULL) {
+//
+// Set flag to make sure allocating memory without GUARD for page table
+// operation; otherwise infinite loops could be caused.
+//
+mOnGuarding = TRUE;
+Status  = gCpu->SetMemoryAttributes (gCpu, Start, EFI_PAGES_TO_SIZE 
(AvailablePages), 0);
+ASSERT_EFI_ERROR (Status);
+mOnGuarding = FALSE;
   }
 
-  return FALSE;
+  mLastPromotedPage = Start;
+  *StartAddress = Start;
+  *EndAddress   = Start + EFI_PAGES_TO_SIZE (AvailablePages) - 1;
+
+  return TRUE;
 }
 
 /**
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109104): https://edk2.groups.io/g/devel/message/109104
Mute This Topic: https://groups.io/mt/101612679/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/5] MdeModulePkg/Core/Dxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

"1 << Priority" / "1 << Event->NotifyTpl" are potentially overflowing
expressions with type "int" (32 bits, signed) evaluated using 32-bit
arithmetic, and then used in a context that expects an expression of
type "UINTN" (64 bits, unsigned).

To avoid overflow, cast "1" to type "UINTN" before << operation.

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

Cc: Dandan Bi 
Cc: Liming Gao 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Event/Event.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c 
b/MdeModulePkg/Core/Dxe/Event/Event.c
index dc82abb02130..6cf93f7f562d 100644
--- a/MdeModulePkg/Core/Dxe/Event/Event.c
+++ b/MdeModulePkg/Core/Dxe/Event/Event.c
@@ -191,7 +191,7 @@ CoreDispatchEventNotifies (
 CoreAcquireEventLock ();
   }
 
-  gEventPending &= ~(UINTN)(1 << Priority);
+  gEventPending &= ~(UINTN)((UINTN)1 << Priority);
   CoreReleaseEventLock ();
 }
 
@@ -225,7 +225,7 @@ CoreNotifyEvent (
   //
 
   InsertTailList (&gEventQueue[Event->NotifyTpl], &Event->NotifyLink);
-  gEventPending |= (UINTN)(1 << Event->NotifyTpl);
+  gEventPending |= (UINTN)((UINTN)1 << Event->NotifyTpl);
 }
 
 /**
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109105): https://edk2.groups.io/g/devel/message/109105
Mute This Topic: https://groups.io/mt/101612681/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 5/5] MdeModulePkg/Core/Dxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

The return value after calls to functions CoreProcessFvImageFile,
CoreStartImage,  CoreGetDepexSectionAndPreProccess,
CoreInternalAddMemorySpace, CoreAddIoSpace, CoreAllocateMemorySpace
and CoreCloseProtocol is stored in Status, but it is not made of any
use and later Status gets overridden.

An option assuming this no Status check is deliberate, would be to
remove the return value storage in Status. Otherwise, simply add
Status check and appropriate debug messages (the patch does this).

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

Cc: Dandan Bi 
Cc: Liming Gao 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c | 15 +++
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c   | 15 +++
 MdeModulePkg/Core/Dxe/Image/Image.c   |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c 
b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
index cf9d55687766..f53a2513457a 100644
--- a/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
+++ b/MdeModulePkg/Core/Dxe/Dispatcher/Dispatcher.c
@@ -506,6 +506,10 @@ CoreDispatcher (
 // Produce a firmware volume block protocol for FvImage so it gets 
dispatched from.
 //
 Status = CoreProcessFvImageFile (DriverEntry->Fv, 
DriverEntry->FvHandle, &DriverEntry->FileName);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_DISPATCH, "Failed to produce a FVB protocol for Fv %p 
FvHandle %p and FileName %g.\n",
+DriverEntry->Fv, DriverEntry->FvHandle, &DriverEntry->FileName));
+}
   } else {
 REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
   EFI_PROGRESS_CODE,
@@ -516,6 +520,10 @@ CoreDispatcher (
 ASSERT (DriverEntry->ImageHandle != NULL);
 
 Status = CoreStartImage (DriverEntry->ImageHandle, NULL, NULL);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_DISPATCH, "Failed to transfer control to a loaded 
image's entry point for ImageHandle %p.\n",
+DriverEntry->ImageHandle));
+}
 
 REPORT_STATUS_CODE_WITH_EXTENDED_DATA (
   EFI_PROGRESS_CODE,
@@ -549,6 +557,13 @@ CoreDispatcher (
 // If Section Extraction Protocol did not let the Depex be read before 
retry the read
 //
 Status = CoreGetDepexSectionAndPreProccess (DriverEntry);
+if (EFI_ERROR (Status)) {
+  if (Status == EFI_PROTOCOL_ERROR) {
+DEBUG ((DEBUG_DISPATCH, "Section extraction protocol failure for 
DriverEntry %p.\n", DriverEntry));
+  } else {
+DEBUG ((DEBUG_DISPATCH, "No Depex, assume UEFI 2.0 driver model 
for DriverEntry %p.\n", DriverEntry));
+  }
+}
   }
 
   if (DriverEntry->Dependent) {
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index 39fa2adf9366..384fee600d85 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -2638,6 +2638,9 @@ CoreInitializeGcdServices (
ResourceHob->ResourceLength,
Capabilities
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to add a segment of memory to GCD map, 
Status = %r\n", Status));
+}
   }
 
   if (GcdIoType != EfiGcdIoTypeNonExistent) {
@@ -2646,6 +2649,9 @@ CoreInitializeGcdServices (
ResourceHob->PhysicalStart,
ResourceHob->ResourceLength
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to add reserved I/O or I/O resources, 
Status = %r\n", Status));
+}
   }
 }
   }
@@ -2668,6 +2674,9 @@ CoreInitializeGcdServices (
gDxeCoreImageHandle,
NULL
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+}
   }
 
   //
@@ -2715,6 +2724,9 @@ CoreInitializeGcdServices (
 gDxeCoreImageHandle,
 NULL
 );
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+  }
 }
   }
 
@@ -2763,6 +2775,9 @@ CoreInitializeGcdServices (
gDxeCoreImageHandle,
NULL
);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_GCD, "Failed to allocate memory space, Status = %r\n", 
Status));
+}
   }
 }
   }
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c 
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 9dbfb2a1fad2..769e2d379051 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1010,6 +1010,9 @@ CoreUnloadAndCloseImage (
  Image->Handle,

[edk2-devel] [PATCH v1 0/5] BZ 4239: Fix PciBusDxe issues pointed by

[Ranbir Singh]

Ranbir Singh (5):
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 73 +++-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c|  1 -
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 48 +
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   | 45 ++--
 4 files changed, 114 insertions(+), 53 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109107): https://edk2.groups.io/g/devel/message/109107
Mute This Topic: https://groups.io/mt/101612806/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function PciHotPlugRequestNotify has two if blocks towards the end
of function both containing return. Due to the parameter checks ensured
at the beginning of the function, one of the two if blocks is bound to
come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
redundant/deadcode.

To fix it, either of line 2109 or 2112 can be deleted.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
index 3f8c6e6da7dc..5b71e152f3ea 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
@@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
 //
 // End for
 //
-return EFI_SUCCESS;
   }
 
   return EFI_SUCCESS;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109108): https://edk2.groups.io/g/devel/message/109108
Mute This Topic: https://groups.io/mt/101612807/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly intentional,
it is not evident to any static analyzer tool. Just adding

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

as explicit comment in between makes a reader as well as Coverity happy.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6594b8eae83f..eda97285ee18 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1428,6 +1428,9 @@ UpdatePciInfo (
   switch (Ptr->AddrSpaceGranularity) {
 case 32:
   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypeMem32;
+  //
+  // No break; here as this is an intentional fall through.
+  //
 case 64:
   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
   break;
@@ -1440,6 +1443,9 @@ UpdatePciInfo (
   switch (Ptr->AddrSpaceGranularity) {
 case 32:
   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
+  //
+  // No break; here as this is an intentional fall through.
+  //
 case 64:
   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
   break;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109109): https://edk2.groups.io/g/devel/message/109109
Mute This Topic: https://groups.io/mt/101612808/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function PciHostBridgeResourceAllocator is not making use of the
generic approach as is used in one of the other function namely -
DumpResourceMap. As a result, the following warnings can be seen as
reported by Coverity e.g.

(30) Event address_of:  Taking address with "&IoBridge" yields a
 singleton pointer.
(31) Event callee_ptr_arith:Passing "&IoBridge" to function
 "FindResourceNode" which uses it as an array. This might corrupt
 or misinterpret adjacent memory locations.

Hence, adopt the generic approach to fix the issues at relevant points.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 84fc0161a19c..71767d3793d4 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
   UINT64 Mem64ResStatus;
   UINT64 PMem64ResStatus;
   UINT32 MaxOptionRomSize;
+  PCI_RESOURCE_NODE  **ChildResources;
+  UINTN  ChildResourceCount;
   PCI_RESOURCE_NODE  *IoBridge;
   PCI_RESOURCE_NODE  *Mem32Bridge;
   PCI_RESOURCE_NODE  *PMem32Bridge;
@@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
 // Create the entire system resource map from the information collected by
 // enumerator. Several resource tree was created
 //
-FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
-FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
-FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
-FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
-FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
-
+ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
+IoBridge = ChildResources[0];
 ASSERT (IoBridge != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
+Mem32Bridge = ChildResources[0];
 ASSERT (Mem32Bridge  != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
+PMem32Bridge = ChildResources[0];
 ASSERT (PMem32Bridge != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
+Mem64Bridge = ChildResources[0];
 ASSERT (Mem64Bridge  != NULL);
+
+ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL);
+ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * 
ChildResourceCount);
+ASSERT (ChildResources != NULL);
+FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
+PMem64Bridge = ChildResources[0];
 ASSERT (PMem64Bridge != NULL);
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109110): https://edk2.groups.io/g/devel/message/109110
Mute This Topic: https://groups.io/mt/101612809/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function StartPciDevices has a check

ASSERT (RootBridge != NULL);

but this comes into play only in DEBUG mode. In Release mode, there
is no handling if the RootBridge value is NULL and the code proceeds
to unconditionally dereference "RootBridge" which will lead to CRASH.

Hence, for safety add NULL pointer checks always and return
EFI_NOT_READY if RootBridge value is NULL which is one of the return
values as mentioned in the function description header.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index 581e9075ad41..f43f10325f16 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -773,6 +773,11 @@ StartPciDevices (
 
   RootBridge = GetRootBridgeByHandle (Controller);
   ASSERT (RootBridge != NULL);
+
+  if (RootBridge == NULL) {
+return EFI_NOT_READY;
+  }
+
   ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
 
   CurrentLink = mPciDevicePool.ForwardLink;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109111): https://edk2.groups.io/g/devel/message/109111
Mute This Topic: https://groups.io/mt/101612810/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

The return value after calls to functions
gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge,
PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write,
gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is
stored in Status, but it is not made of any use and later Status
gets overridden.

Remove the return value storage in Status or add Status check as
seems appropriate at a particular point.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 +++-
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c   |  8 +++
 3 files changed, 72 insertions(+), 46 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index f43f10325f16..ae770d766381 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -544,12 +544,12 @@ DeRegisterPciDevice (
   EFI_OPEN_PROTOCOL_TEST_PROTOCOL
   );
   if (!EFI_ERROR (Status)) {
-Status = gBS->UninstallMultipleProtocolInterfaces (
-Handle,
-&gEfiLoadFile2ProtocolGuid,
-&PciIoDevice->LoadFile2,
-NULL
-);
+gBS->UninstallMultipleProtocolInterfaces (
+   Handle,
+   &gEfiLoadFile2ProtocolGuid,
+   &PciIoDevice->LoadFile2,
+   NULL
+   );
   }
 
   //
@@ -678,19 +678,21 @@ StartPciDevicesOnBridge (
ChildHandleBuffer
);
 
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationSupported,
- 0,
- &Supports
- );
-Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationEnable,
- Supports,
- NULL
- );
+if (!EFI_ERROR (Status)) {
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationSupported,
+   0,
+   &Supports
+   );
+  Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationEnable,
+   Supports,
+   NULL
+   );
+}
 
 return Status;
   } else {
@@ -726,19 +728,21 @@ StartPciDevicesOnBridge (
ChildHandleBuffer
);
 
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationSupported,
- 0,
- &Supports
- );
-Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
-PciIoDevice->PciIo.Attributes (
- &(PciIoDevice->PciIo),
- EfiPciIoAttributeOperationEnable,
- Supports,
- NULL
- );
+if (!EFI_ERROR (Status)) {
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationSupported,
+   0,
+   &Supports
+   );
+  Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+  PciIoDevice->PciIo.Attributes (
+   &(PciIoDevice->PciIo),
+   EfiPciIoAttributeOperationEnable,
+   Supports,
+   NULL
+   );
+}
   }
 
   CurrentLink = CurrentLink->ForwardLink;
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index eda97285ee18..636885dd189d 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -2796,6 +2796,20 @@ 

[edk2-devel] [PATCH v1 0/2] BZ 4249: Fix FatPkg/EnhancedFatDxe issues pointed by Coverity

[Ranbir Singh]

Ranbir Singh (2):
  FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues
  FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 FatPkg/EnhancedFatDxe/DiskCache.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109138): https://edk2.groups.io/g/devel/message/109138
Mute This Topic: https://groups.io/mt/101633787/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/2] FatPkg/EnhancedFatDxe: Fix SIGN_EXTENSION Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The functions FatGetDirEntInfo and FatOpenDirEnt contains the code
statements

  Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
  and
  OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) | 
(DirEnt->Entry.FileCluster);

respectively. As per Coverity report, in both these statements, there is
an "Operand1" with type "UINT16" (16 bits, unsigned) which is promoted in
"(Operand1 << 16) | Operand2" to type "int" (32 bits, signed), then sign-
extended to type "unsigned long long" (64 bits, unsigned). If the result
of "(Operand1 << 16) | Operand2" is greater than 0x7FFF, the upper
bits of the result will all be 1.

So to avoid sign-extension, typecast the Operand1 and then the inter-
-mediate result after << 16 operation with UINTN. Note - UINTN is the
data type of the variable on the LHS of the assignment.

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 FatPkg/EnhancedFatDxe/DirectoryManage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/FatPkg/EnhancedFatDxe/DirectoryManage.c 
b/FatPkg/EnhancedFatDxe/DirectoryManage.c
index 723fc35f38db..a21b7973cd21 100644
--- a/FatPkg/EnhancedFatDxe/DirectoryManage.c
+++ b/FatPkg/EnhancedFatDxe/DirectoryManage.c
@@ -474,7 +474,7 @@ FatGetDirEntInfo (
 Info   = Buffer;
 Info->Size = ResultSize;
 if ((Entry->Attributes & FAT_ATTRIBUTE_DIRECTORY) != 0) {
-  Cluster= (Entry->FileClusterHigh << 16) | Entry->FileCluster;
+  Cluster= (UINTN)((UINTN)(Entry->FileClusterHigh) << 16) | 
Entry->FileCluster;
   Info->PhysicalSize = FatPhysicalDirSize (Volume, Cluster);
   Info->FileSize = Info->PhysicalSize;
 } else {
@@ -1167,7 +1167,7 @@ FatOpenDirEnt (
   //
   Volume = Parent->Volume;
   OFile->FullPathLen = Parent->FullPathLen + 1 + StrLen 
(DirEnt->FileString);
-  OFile->FileCluster = ((DirEnt->Entry.FileClusterHigh) << 16) | 
(DirEnt->Entry.FileCluster);
+  OFile->FileCluster = (UINTN)((UINTN)(DirEnt->Entry.FileClusterHigh) << 
16) | (DirEnt->Entry.FileCluster);
   InsertTailList (&Parent->ChildHead, &OFile->ChildLink);
 } else {
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109139): https://edk2.groups.io/g/devel/message/109139
Mute This Topic: https://groups.io/mt/101633788/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/2] FatPkg/EnhancedFatDxe: Fix OVERFLOW_BEFORE_WIDEN Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function FatInitializeDiskCache evaluates an expression

FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment

and assigns it to DataCacheSize which is of type UINTN.

As per Coverity report,
FAT_DATACACHE_GROUP_COUNT << DiskCache[CacheData].PageAlignment is a
potentially overflowing expression with type "int" (32 bits, signed)
evaluated using 32-bit arithmetic, and then used in a context that
expects an expression of type "UINTN" (64 bits, unsigned).

To avoid overflow, cast "FAT_DATACACHE_GROUP_COUNT" to type "UINTN".

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

Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 FatPkg/EnhancedFatDxe/DiskCache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/FatPkg/EnhancedFatDxe/DiskCache.c 
b/FatPkg/EnhancedFatDxe/DiskCache.c
index d1a34a6a646f..d56e338586d8 100644
--- a/FatPkg/EnhancedFatDxe/DiskCache.c
+++ b/FatPkg/EnhancedFatDxe/DiskCache.c
@@ -477,7 +477,7 @@ FatInitializeDiskCache (
   DiskCache[CacheFat].BaseAddress   = Volume->FatPos;
   DiskCache[CacheFat].LimitAddress  = Volume->FatPos + Volume->FatSize;
   FatCacheSize  = FatCacheGroupCount << 
DiskCache[CacheFat].PageAlignment;
-  DataCacheSize = FAT_DATACACHE_GROUP_COUNT << 
DiskCache[CacheData].PageAlignment;
+  DataCacheSize = (UINTN)FAT_DATACACHE_GROUP_COUNT << 
DiskCache[CacheData].PageAlignment;
   //
   // Allocate the Fat Cache buffer
   //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109140): https://edk2.groups.io/g/devel/message/109140
Mute This Topic: https://groups.io/mt/101633789/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

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 (#109272): https://edk2.groups.io/g/devel/message/109272
Mute This Topic: https://groups.io/mt/101729055/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 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 (#109273): https://edk2.groups.io/g/devel/message/109273
Mute This Topic: https://groups.io/mt/101729056/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

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 as well as any static analyzer tool. Just adding

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

as comment in between makes any reader as well as Coverity happy.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 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 (#109274): https://edk2.groups.io/g/devel/message/109274
Mute This Topic: https://groups.io/mt/101729058/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity

[Ranbir Singh]

Ranbir Singh (2):
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 
 2 files changed, 12 insertions(+)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109307): https://edk2.groups.io/g/devel/message/109307
Mute This Topic: https://groups.io/mt/101750272/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function USBMouseDriverBindingStart do have

ASSERT (UsbMouseDevice != NULL);

after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference "UsbMouseDevice"
afterwards which will lead to CRASH.

Hence, for safety add NULL pointer checks always.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 451d4b934f4c..621d09713b24 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -161,6 +161,10 @@ USBMouseDriverBindingStart (
 
   UsbMouseDevice = AllocateZeroPool (sizeof (USB_MOUSE_DEV));
   ASSERT (UsbMouseDevice != NULL);
+  if (UsbMouseDevice == NULL) {
+Status = EFI_OUT_OF_RESOURCES;
+goto ErrorExit;
+  }
 
   UsbMouseDevice->UsbIo = UsbIo;
   UsbMouseDevice->Signature = USB_MOUSE_DEV_SIGNATURE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109308): https://edk2.groups.io/g/devel/message/109308
Mute This Topic: https://groups.io/mt/101750273/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function GetNextHidItem has a switch-case code in which the
case 1: falls through to case 2: and then case 2: falls through
to case 3: in the block.

While this may be intentional, it is not evident to any general
code reader as well as any static analyzer tool. Just adding

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

as comment in between makes a reader as well as Coverity happy.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
index acc19acd98e0..bc9a4824208b 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
@@ -89,6 +89,10 @@ GetNextHidItem (
   return StartPos;
 }
 
+//
+// No break; here as this is an intentional fallthrough
+//
+
   case 2:
 //
 // 2-byte data
@@ -99,6 +103,10 @@ GetNextHidItem (
   return StartPos;
 }
 
+//
+// No break; here as this is an intentional fallthrough
+//
+
   case 3:
 //
 // 4-byte data, adjust size
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109309): https://edk2.groups.io/g/devel/message/109309
Mute This Topic: https://groups.io/mt/101750274/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

Hi Mike,

I earlier assumed that both the fallthrough were intentional and just made
it evident by writing the comment. It was hence written actually for the
code reader and not the static analyzer. The comment was similar in nature
and style to existing No/1-byte/... data comments. *Incidentally, the
comment satisfied the static analyzer as well. *It was beyond my
expectation and I was pleasantly surprised too. Without digging deep into
the particular code and to avoid any functional code change, I just thought
that's it.

Your comment forced me to come back specifically to the particular code and
to analyze it a bit more.

My analysis now says that the fallthrough doesn't serve any real functional
purpose as there is absolutely no chance of *if* blocks of the fallthrough
*case(*s) being executed later and not executed in the original *case:* and
hence the fallthrough if it happens still leads to execution of *return
NULL; *at the end of the function only. This is better communicated by
adding *break;* statement in the original *case:* and in a way, one may
agree with the static analyzer that there was a MISSING_BREAK. I should
hence follow it up with a v2 patch for this.

Ranbir Singh

On Thu, Oct 5, 2023 at 12:17 AM Kinney, Michael D <
michael.d.kin...@intel.com> wrote:

> I do not prefer special comments for one static analyzer.
>
> Is there an alternative design/implementation of this code
> to make it more readable and not trigger any static analysis
> false positives?
>
> Mike
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Ranbir
> > Singh
> > Sent: Tuesday, October 3, 2023 10:48 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray ;
> > Veeresh Sangolli 
> > Subject: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> > Fix MISSING_BREAK Coverity issues
> >
> > From: Ranbir Singh 
> >
> > The function GetNextHidItem has a switch-case code in which the
> > case 1: falls through to case 2: and then case 2: falls through
> > to case 3: in the block.
> >
> > While this may be intentional, it is not evident to any general
> > code reader as well as any static analyzer tool. Just adding
> >
> > // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes a reader as well as Coverity happy.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > Co-authored-by: Veeresh Sangolli 
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > index acc19acd98e0..bc9a4824208b 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > @@ -89,6 +89,10 @@ GetNextHidItem (
> >return StartPos;
> >
> >  }
> >
> >
> >
> > +//
> >
> > +// No break; here as this is an intentional fallthrough
> >
> > +//
> >
> > +
> >
> >case 2:
> >
> >  //
> >
> >  // 2-byte data
> >
> > @@ -99,6 +103,10 @@ GetNextHidItem (
> >return StartPos;
> >
> >  }
> >
> >
> >
> > +//
> >
> > +// No break; here as this is an intentional fallthrough
> >
> > +//
> >
> > +
> >
> >case 3:
> >
> >  //
> >
> >  // 4-byte data, adjust size
> >
> > --
> > 2.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#109309):
> > https://edk2.groups.io/g/devel/message/109309
> > Mute This Topic: https://groups.io/mt/101750274/1643496
> > Group Owner: devel+ow...@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kin...@intel.com]
> > -=-=-=-=-=-=
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109342): https://edk2.groups.io/g/devel/message/109342
Mute This Topic: https://groups.io/mt/101750274/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD
  - Changed the solution wrt MISSING_BREAK issues

Ranbir Singh (2):
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 
 2 files changed, 10 insertions(+)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109455): https://edk2.groups.io/g/devel/message/109455
Mute This Topic: https://groups.io/mt/101849996/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The function USBMouseDriverBindingStart do have

ASSERT (UsbMouseDevice != NULL);

after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference "UsbMouseDevice"
afterwards which will lead to CRASH.

Hence, for safety add NULL pointer checks always.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Co-authored-by: Veeresh Sangolli 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 451d4b934f4c..621d09713b24 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -161,6 +161,10 @@ USBMouseDriverBindingStart (
 
   UsbMouseDevice = AllocateZeroPool (sizeof (USB_MOUSE_DEV));
   ASSERT (UsbMouseDevice != NULL);
+  if (UsbMouseDevice == NULL) {
+Status = EFI_OUT_OF_RESOURCES;
+goto ErrorExit;
+  }
 
   UsbMouseDevice->UsbIo = UsbIo;
   UsbMouseDevice->Signature = USB_MOUSE_DEV_SIGNATURE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109456): https://edk2.groups.io/g/devel/message/109456
Mute This Topic: https://groups.io/mt/101849997/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

The function GetNextHidItem has a switch-case block in which the case 1:
falls through to case 2: and then case 2: falls through to case 3:.

There is no possibility of the if blocks within case 2: and case 3: to
succeed later and not succeed in the original case and hence the fall
throughs even if it hypothetically happens are redundant as the code
still will eventually return NULL only at the function end point.

Better introduce straight forward break; statement within actual cases.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c 
b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
index acc19acd98e0..f07e48774a34 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
@@ -89,6 +89,8 @@ GetNextHidItem (
   return StartPos;
 }
 
+break;
+
   case 2:
 //
 // 2-byte data
@@ -99,6 +101,8 @@ GetNextHidItem (
   return StartPos;
 }
 
+break;
+
   case 3:
 //
 // 4-byte data, adjust size
@@ -109,6 +113,8 @@ GetNextHidItem (
   StartPos += 4;
   return StartPos;
 }
+
+break;
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109457): https://edk2.groups.io/g/devel/message/109457
Mute This Topic: https://groups.io/mt/101849998/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] IntelFsp2Pkg/Library/BaseFspCommonLib: Fix OVERRUN Coverity issue

[Ranbir Singh]

FspData->PerfIdx is getting increased for every call unconditionally
in the function SetFspMeasurePoint and hence memory access can happen
for out of bound FspData->PerfData[] array entries also.

Example -
   FspData->PerfData is an array of 32 UINT64 entries. Assume a call
   is made to SetFspMeasurePoint function when the FspData->PerfIdx
   last value is 31. It gets incremented to 32 at line 400.
   Any subsequent call to SetFspMeasurePoint functions leads to
   FspData->PerfData[32] getting accessed which is out of the PerfData
   array as well as the FSP_GLOBAL_DATA structure boundary.

Hence keep array access and index increment inside if block only and
return invalid performance timestamp when PerfIdx is invalid.

Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Star Zeng 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4200
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c 
b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
index a22b0e7825ad..cda2a7b2478e 100644
--- a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
+++ b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
@@ -377,7 +377,8 @@ GetFspSiliconInitUpdDataPointer (
 
   @param[in] Id   Measurement point ID.
 
-  @return performance timestamp.
+  @return performance timestamp if current PerfIdx is valid,
+  else return 0 as invalid performance timestamp
 **/
 UINT64
 EFIAPI
@@ -395,9 +396,10 @@ SetFspMeasurePoint (
   if (FspData->PerfIdx < sizeof (FspData->PerfData) / sizeof 
(FspData->PerfData[0])) {
 FspData->PerfData[FspData->PerfIdx]  = AsmReadTsc ();
 ((UINT8 *)(&FspData->PerfData[FspData->PerfIdx]))[7] = Id;
+return FspData->PerfData[(FspData->PerfIdx)++];
   }
 
-  return FspData->PerfData[(FspData->PerfIdx)++];
+  return (UINT64)0x;
 }
 
 /**
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105032): https://edk2.groups.io/g/devel/message/105032
Mute This Topic: https://groups.io/mt/98993286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] IntelFsp2Pkg/Library/BaseFspCommonLib: Fix OVERRUN Coverity issue

[Ranbir Singh]

Yes Chasel - please do so while merging, thanks!

On Tue, May 30, 2023 at 8:58 AM Chiu, Chasel  wrote:

>
> That’s good suggestion Pedro!
> Ranbir, would you like me to modify your patch to "return 0" during
> merging?
>
> Thanks,
> Chasel
>
>
> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Pedro
> > Falcato
> > Sent: Friday, May 19, 2023 5:29 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Chiu, Chasel ; Desimone, Nathaniel L
> > ; Zeng, Star ;
> Ranbir
> > Singh 
> > Subject: Re: [edk2-devel] [PATCH 1/1]
> IntelFsp2Pkg/Library/BaseFspCommonLib:
> > Fix OVERRUN Coverity issue
> >
> > On Thu, May 18, 2023 at 4:16 PM Ranbir Singh 
> > wrote:
> > >
> > > FspData->PerfIdx is getting increased for every call unconditionally
> > > in the function SetFspMeasurePoint and hence memory access can happen
> > > for out of bound FspData->PerfData[] array entries also.
> > >
> > > Example -
> > >FspData->PerfData is an array of 32 UINT64 entries. Assume a call
> > >is made to SetFspMeasurePoint function when the FspData->PerfIdx
> > >last value is 31. It gets incremented to 32 at line 400.
> > >Any subsequent call to SetFspMeasurePoint functions leads to
> > >FspData->PerfData[32] getting accessed which is out of the PerfData
> > >array as well as the FSP_GLOBAL_DATA structure boundary.
> > >
> > > Hence keep array access and index increment inside if block only and
> > > return invalid performance timestamp when PerfIdx is invalid.
> > >
> > > Cc: Chasel Chiu 
> > > Cc: Nate DeSimone 
> > > Cc: Star Zeng 
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4200
> > > Signed-off-by: Ranbir Singh 
> > > Signed-off-by: Ranbir Singh 
> > > ---
> > >  IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c | 6 --
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > > b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > > index a22b0e7825ad..cda2a7b2478e 100644
> > > --- a/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > > +++ b/IntelFsp2Pkg/Library/BaseFspCommonLib/FspCommonLib.c
> > > @@ -377,7 +377,8 @@ GetFspSiliconInitUpdDataPointer (
> > >
> > >@param[in] Id   Measurement point ID.
> > >
> > > -  @return performance timestamp.
> > > +  @return performance timestamp if current PerfIdx is valid,
> > > +  else return 0 as invalid performance timestamp
> > >  **/
> > >  UINT64
> > >  EFIAPI
> > > @@ -395,9 +396,10 @@ SetFspMeasurePoint (
> > >if (FspData->PerfIdx < sizeof (FspData->PerfData) / sizeof (FspData-
> > >PerfData[0])) {
> > >  FspData->PerfData[FspData->PerfIdx]  = AsmReadTsc
> ();
> > >  ((UINT8 *)(&FspData->PerfData[FspData->PerfIdx]))[7] = Id;
> > > +return FspData->PerfData[(FspData->PerfIdx)++];
> > >}
> > >
> > > -  return FspData->PerfData[(FspData->PerfIdx)++];
> > > +  return (UINT64)0x;
> >
> > return 0;
> >
> > Works just as well. You also don't need a cast.
> >
> > https://godbolt.org/z/e5vvGcWWo
> >
> > --
> > Pedro
> >
> >
> > 
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105454): https://edk2.groups.io/g/devel/message/105454
Mute This Topic: https://groups.io/mt/98993286/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.
Assuming, this non-usage is deliberate, the storage in Status can be
done away with.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..c6d637afa989 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
   DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->heads - 1);
   DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
 
-  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, 
&DriveParameters, NULL);
+  SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, 
NULL);
 }
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105670): https://edk2.groups.io/g/devel/message/105670
Mute This Topic: https://groups.io/mt/99293623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..70c4ca27dc68 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
 // Check logical block size
 //
 if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
-  BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) 
| IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+  BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) 
| IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105669): https://edk2.groups.io/g/devel/message/105669
Mute This Topic: https://groups.io/mt/99293622/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/1] MdeModulePkg/Bus/Ata/AtaBusDxe: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

Line number 365 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4209
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c 
b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
index a77852bae054..ccd4c5f05b59 100644
--- a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
+++ b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
@@ -362,7 +362,7 @@ IdentifyAtaDevice (
 // Check logical block size
 //
 if ((PhyLogicSectorSupport & BIT12) != 0) {
-  BlockMedia->BlockSize = (UINT32)(((IdentifyData->logic_sector_size_hi << 
16) | IdentifyData->logic_sector_size_lo) * sizeof (UINT16));
+  BlockMedia->BlockSize = (((UINT32)(IdentifyData->logic_sector_size_hi << 
16) | IdentifyData->logic_sector_size_lo) * sizeof (UINT16));
 }
 
 AtaDevice->BlockIo.Revision = EFI_BLOCK_IO_PROTOCOL_REVISION2;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105672): https://edk2.groups.io/g/devel/message/105672
Mute This Topic: https://groups.io/mt/99293624/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

Please read the Coverity report attached in BZ 4204 for more details on
sign-extension issue.

The data types of logic_sector_size_hi and logic_sector_size_lo are UINT16
and Isn't the return type of *sizeof* already unsigned ?

Now I have no means to run Coverity and test further changes.
Anyway, my understanding back then was that the sign-extension was
primarily happening because of the 16 bits left shift operation. I did test
the patch for coverity warning resolution back in Jan'23 which went off.


On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel  wrote:

> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh  wrote:
> >
> > From: Ranbir Singh 
> >
> > Line number 1348 does contain a typecast with UINT32, but it is after
> > all the operations (16-bit left shift followed by OR'ing) are over.
> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
> > 16-bit left shift operation immediately with UINT32.
> >
>
> What is wrong with sign extension?
>
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > index 50406fe0270d..70c4ca27dc68 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
> >  // Check logical block size
> >  //
> >  if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
> > -  BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi
> << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> > +  BlockSize = (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi
> << 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
>
>
> The outer parens are now redundant, which means you're assigning
> something to BlockSize whose type is based on the type of 
> * sizeof(UINT16), which is unsigned long not unsigned int, so this
> will produce a truncation warning on some compilers.
>
> If you want to suppress the coverity warning without introducing new
> ones, better to cast the sizeof() to (UINT32).
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105719): https://edk2.groups.io/g/devel/message/105719
Mute This Topic: https://groups.io/mt/99293622/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

I counted myself as not the right person to decide what all to do if
*Status* is not successful. Adding the DEBUG statement from the Coverity
aspect doesn't count as a fix as RELEASE mode behavior remains the same.

In the comments / description, I already mentioned - Assuming, this
non-usage is deliberate, so as such I did not intend to hide anything - and
left it in the status quo.

The patch proposed may not be appropriate, but should now give a thinking
point to active module developers / owners / maintainers if they indeed
feel that there is a bug here and it needs to be fixed.

On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel  wrote:

> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato 
> wrote:
> >
> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh 
> wrote:
> > >
> > > From: Ranbir Singh 
> > >
> > > The return value stored in Status after call to SetDriveParameters
> > > is not made of any use thereafter and hence it remains as UNUSED.
> > > Assuming, this non-usage is deliberate, the storage in Status can be
> > > done away with.
> > >
> > > Cc: Hao A Wu 
> > > Cc: Ray Ni 
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > > Signed-off-by: Ranbir Singh 
> > > ---
> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > index 75403886e44a..c6d637afa989 100644
> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> > >DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->heads - 1);
> > >DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> > >
> > > -  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> > > +  SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
> >
> > I'm /fairly/ sure this is wrong and that you need to use Status.
> >
>
> Yeah, removing the assignment fixes the coverity warning, but now you
> are hiding a bug instead of fixing it.
>
> SetDriveParameters () can apparently fail, and this is being ignored.
> At the very least, we should emit a diagnostic here in DEBUG mode to
> log this. E.g.,
>
> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> __func__, Status));
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105720): https://edk2.groups.io/g/devel/message/105720
Mute This Topic: https://groups.io/mt/99293623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

Thanks Ard for adding the explanation.

Ok, so retaining outer cast along with intermediate casting

BlockSize = (UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi
<< 16) | IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));

should be acceptable. I considered outer cast is being implicitly done as
per the data type of *BlockSize *and also there was no Coverity warning
after the removal*.*




On Mon, Jun 5, 2023 at 2:14 PM Ard Biesheuvel  wrote:

> On Mon, 5 Jun 2023 at 09:54, Ranbir Singh  wrote:
> >
> > Please read the Coverity report attached in BZ 4204 for more details on
> sign-extension issue.
> >
>
> I did read it - explanation below.
>
> > The data types of logic_sector_size_hi and logic_sector_size_lo are
> UINT16 and Isn't the return type of sizeof already unsigned ?
> >
>
> Yes, it is unsigned long, so 64 bits wide on LP64 architectures
>
> > Now I have no means to run Coverity and test further changes.
> > Anyway, my understanding back then was that the sign-extension was
> primarily happening because of the 16 bits left shift operation. I did test
> the patch for coverity warning resolution back in Jan'23 which went off.
> >
>
> The warning actually describes exactly what is happening:
> - The type of a UINT16 shifted left by 16 is promoted to signed integer.
> - The multiplication by sizeof() converts the former result to
> unsigned integer before widening to unsigned long integer.
>
> This means that the widening does not perform any sign extension,
> which is what Coverity warns about.
>
> None of this actually matters, given that those upper bits get
> truncated again anyway when we assign to BlockSize, which is only 32
> bits wide.
>
> Removing the outer (UINT32) cast is not the correct solution, as it
> may result in spurious truncation warnings on some toolchains.
>
> If you want to silence this warning (I wouldn't call it a fix), you
> need to either prevent the widening, or cast the intermediate result
> to (UINT32), and the latter is what your patch does. So it silences
> the warning, but now, the type of the right hand side of the
> assignment is UINTN not UINT32.
>
> So either leave the outer (UINT32) cast in place, or move it to before
> the sizeof() as I suggested before: in both cases, there is no longer
> any promotion to unsigned long, which should make the warning go away.
>
>
>
> >
> > On Mon, Jun 5, 2023 at 4:28 AM Ard Biesheuvel  wrote:
> >>
> >> On Fri, 2 Jun 2023 at 21:42, Ranbir Singh 
> wrote:
> >> >
> >> > From: Ranbir Singh 
> >> >
> >> > Line number 1348 does contain a typecast with UINT32, but it is after
> >> > all the operations (16-bit left shift followed by OR'ing) are over.
> >> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
> >> > 16-bit left shift operation immediately with UINT32.
> >> >
> >>
> >> What is wrong with sign extension?
> >>
> >> > Cc: Hao A Wu 
> >> > Cc: Ray Ni 
> >> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> >> > Signed-off-by: Ranbir Singh 
> >> > ---
> >> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > index 50406fe0270d..70c4ca27dc68 100644
> >> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
> >> > @@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
> >> >  // Check logical block size
> >> >  //
> >> >  if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) !=
> 0) {
> >> > -  BlockSize =
> (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) |
> IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> >> > +  BlockSize =
> (((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) |
> IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
> >>
> >>
> >> The outer parens are now redundant, which means you're assigning
> >> something to BlockSize whose type is based on the type of 
> >> * sizeof(UINT16), which is unsigned long not unsigned int, so this
> >> will produce a truncation warning on some compilers.
> >>
> >> If you want to suppress the coverity warning without introducing new
> >> ones, better to cast the sizeof() to (UINT32).
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105721): https://edk2.groups.io/g/devel/message/105721
Mute This Topic: https://groups.io/mt/99293622/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] MdeModulePkg/Bus/Ata/AtaBusDxe: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

Yes, I have already noted to update this patch on the same lines as
discussed in context of https://edk2.groups.io/g/devel/topic/99293622

On Thu, Jun 8, 2023 at 12:48 PM Wu, Hao A  wrote:

> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Saturday, June 3, 2023 12:09 AM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray 
> > Subject: [PATCH 1/1] MdeModulePkg/Bus/Ata/AtaBusDxe: Fix
> > SIGN_EXTENSION Coverity issue
> >
> > From: Ranbir Singh 
> >
> > Line number 365 does contain a typecast with UINT32, but it is after
> > all the operations (16-bit left shift followed by OR'ing) are over.
> > To avoid any SIGN_EXTENSION, typecast the intermediate result after
> > 16-bit left shift operation immediately with UINT32.
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4209
> > Signed-off-by: Ranbir Singh 
> > ---
> >  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
> > b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
> > index a77852bae054..ccd4c5f05b59 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c
> > @@ -362,7 +362,7 @@ IdentifyAtaDevice (
> >  // Check logical block size
> >
> >  //
> >
> >  if ((PhyLogicSectorSupport & BIT12) != 0) {
> >
> > -  BlockMedia->BlockSize =
> (UINT32)(((IdentifyData->logic_sector_size_hi
> > << 16) | IdentifyData->logic_sector_size_lo) * sizeof (UINT16));
> >
> > +  BlockMedia->BlockSize =
> (((UINT32)(IdentifyData->logic_sector_size_hi
> > << 16) | IdentifyData->logic_sector_size_lo) * sizeof (UINT16));
> >
> >  }
>
>
> This patch seems to have the same issue with the concern raised in
> https://edk2.groups.io/g/devel/topic/99293622.
>
> Best Regards,
> Hao Wu
>
>
> >
> >
> >
> >  AtaDevice->BlockIo.Revision = EFI_BLOCK_IO_PROTOCOL_REVISION2;
> >
> > --
> > 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105933): https://edk2.groups.io/g/devel/message/105933
Mute This Topic: https://groups.io/mt/99293624/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

I mentioned similar approach in
https://bugzilla.tianocore.org/show_bug.cgi?id=4204#c1

Let me know if I should update the patch as Hao proposed -

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n",
Status));
continue;
  }

On Thu, Jun 8, 2023 at 12:25 PM Wu, Hao A  wrote:

> > -Original Message-
> > From: devel@edk2.groups.io  On Behalf Of Ard
> > Biesheuvel
> > Sent: Monday, June 5, 2023 4:32 PM
> > To: Ranbir Singh 
> > Cc: devel@edk2.groups.io; pedro.falc...@gmail.com; Wu, Hao A
> > ; Ni, Ray 
> > Subject: Re: [edk2-devel] [PATCH 2/2]
> > MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity
> > issue
> >
> > On Mon, 5 Jun 2023 at 10:10, Ranbir Singh 
> > wrote:
> > >
> > > I counted myself as not the right person to decide what all to do if
> Status is
> > not successful. Adding the DEBUG statement from the Coverity aspect
> > doesn't count as a fix as RELEASE mode behavior remains the same.
> > >
> > > In the comments / description, I already mentioned - Assuming, this
> non-
> > usage is deliberate, so as such I did not intend to hide anything - and
> left it in
> > the status quo.
> > >
> > > The patch proposed may not be appropriate, but should now give a
> > thinking point to active module developers / owners / maintainers if they
> > indeed feel that there is a bug here and it needs to be fixed.
> > >
> >
> > Thanks - I agree that it is a good thing that people are aware of this
> now.
>
>
> Judging from the context in DetectAndConfigIdeDevice(), I think a fix can
> be:
>
>   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>   if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_ERROR, "Set drive parameters Fail, Status = %r\n",
> Status));
> continue;
>   }
>
> But I do not have the device and platform environment to verify the change.
> Really sorry for this. I am not sure on the potential risk of making this
> change without at least some kind of 'no-break' tests.
>
> Best Regards,
> Hao Wu
>
>
> >
> > > On Mon, Jun 5, 2023 at 4:33 AM Ard Biesheuvel  wrote:
> > >>
> > >> On Sat, 3 Jun 2023 at 18:04, Pedro Falcato 
> > wrote:
> > >> >
> > >> > On Fri, Jun 2, 2023 at 8:42 PM Ranbir Singh <
> rsi...@ventanamicro.com>
> > wrote:
> > >> > >
> > >> > > From: Ranbir Singh 
> > >> > >
> > >> > > The return value stored in Status after call to SetDriveParameters
> > >> > > is not made of any use thereafter and hence it remains as UNUSED.
> > >> > > Assuming, this non-usage is deliberate, the storage in Status can
> be
> > >> > > done away with.
> > >> > >
> > >> > > Cc: Hao A Wu 
> > >> > > Cc: Ray Ni 
> > >> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > >> > > Signed-off-by: Ranbir Singh 
> > >> > > ---
> > >> > >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
> > >> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> > >
> > >> > > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > index 75403886e44a..c6d637afa989 100644
> > >> > > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > >> > > @@ -2555,7 +2555,7 @@ DetectAndConfigIdeDevice (
> > >> > >DriveParameters.Heads  =
> (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> > >> > >DriveParameters.MultipleSector =
> (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> > >> > >
> > >> > > -  Status = SetDriveParameters (Instance, IdeChannel,
> IdeDevice,
> > &DriveParameters, NULL);
> > >> > > +  SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> > >> >
> > >> > I'm /fairly/ sure this is wrong and that you need to use Status.
> > >> >
> > >>
> > >> Yeah, removing the assignment fixes the coverity warning, but now you
> > >> are hiding a bug instead of fixing it.
> > >>
> > >> SetDriveParameters () can apparently fail, and this is being ignored.
> > >> At the very least, we should emit a diagnostic here in DEBUG mode to
> > >> log this. E.g.,
> > >>
> > >> DEBUG ((DEBUG_WARN, "%a: SetDriveParameters () failed - %r\n",
> > >> __func__, Status));
> >
> >
> > 
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105934): https://edk2.groups.io/g/devel/message/105934
Mute This Topic: https://groups.io/mt/99293623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---

Notes:
Retain outer cast

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..f39c909d0631 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
 // Check logical block size
 //
 if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
-  BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) 
| IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+  BlockSize = 
(UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | 
IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105981): https://edk2.groups.io/g/devel/message/105981
Mute This Topic: https://groups.io/mt/99432079/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.

Add error check as is done after calls to SetDeviceTransferMode.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---

Notes:
Add error check instead of Status storage removal

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..d04b1d95a7f5 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -2549,13 +2549,18 @@ DetectAndConfigIdeDevice (
 //
 if (DeviceType == EfiIdeHarddisk) {
   //
-  // Init driver parameters
+  // Init drive parameters
   //
   DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->sectors_per_track;
   DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->heads - 1);
   DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
 
   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, 
&DriveParameters, NULL);
+
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n", 
Status));
+continue;
+  }
 }
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105982): https://edk2.groups.io/g/devel/message/105982
Mute This Topic: https://groups.io/mt/99432080/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

[Ranbir Singh]

v1 -> v2:
  - Retain outer cast
  - Add error check instead of Status storage removal

Ranbir Singh (2):
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  | 7 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105980): https://edk2.groups.io/g/devel/message/105980
Mute This Topic: https://groups.io/mt/99432078/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] MdeModulePkg/Bus/Pci/EhciDxe: Fix FORWARD_NULL Coverity issues

[Ranbir Singh]

From: Ranbir Singh 

The function UsbHcGetPciAddressForHostMem has

ASSERT ((Block != NULL));

and the UsbHcFreeMem has

ASSERT (Block != NULL);

statement 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.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4210
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c 
b/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
index 0a3ceb9f711a..623cca0d7eb3 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
@@ -250,6 +250,12 @@ UsbHcGetPciAddressForHostMem (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+DEBUG ((DEBUG_INFO, "Given memory block not present in host controller's 
pool\n"));
+return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -536,6 +542,11 @@ UsbHcFreeMem (
   //
   ASSERT (Block != NULL);
 
+  if (Block == NULL) {
+DEBUG ((DEBUG_INFO, "Memory to free not present in host controller's 
pool\n"));
+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 (#106086): https://edk2.groups.io/g/devel/message/106086
Mute This Topic: https://groups.io/mt/99529907/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh 

The function UsbHcGetPciAddressForHostMem has

ASSERT ((Block != NULL));

and the UsbHcFreeMem has

ASSERT (Block != NULL);

statement after for loop, but these are applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons there is no match inside the
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.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4210
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c 
b/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
index 0a3ceb9f711a..79575b6f6304 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/UsbHcMem.c
@@ -250,6 +250,11 @@ UsbHcGetPciAddressForHostMem (
   }
 
   ASSERT ((Block != NULL));
+
+  if (Block == NULL) {
+return 0;
+  }
+
   //
   // calculate the pci memory address for host memory address.
   //
@@ -536,6 +541,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 (#106618): https://edk2.groups.io/g/devel/message/106618
Mute This Topic: https://groups.io/mt/99936708/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

Agreed! Will update accordingly.

On Wed, Jul 12, 2023 at 12:36 PM Wu, Hao A  wrote:

> It works for me, better to override by:
>
>   Status = EFI_SUCCESS;
>
>
>
> Best Regards,
>
> Hao Wu
>
>
>
> *From:* Ranbir Singh 
> *Sent:* Wednesday, July 12, 2023 3:01 PM
> *To:* Wu, Hao A 
> *Cc:* devel@edk2.groups.io; Ni, Ray 
> *Subject:* Re: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
> UNUSED_VALUE Coverity issue
>
>
>
> Thanks Hao for digging deeper into this.
>
>
>
> The if block itself might get knocked off in Release mode when there is
> only a DEBUG statement inside it and hence Coverity might still complain.
> So, we can override the Status value in this scenario inside the if block
> and then proceed normally - let me know if this is acceptable and I will
> update the patch as below then
>
>   if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
> Status));
>
> /* Ignore Warning and proceed normally */
>
> Status = 0;
>   }
>
>
>
> Best Regards,
>
> Ranbir Singh
>
>
>
> On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A  wrote:
>
> Really sorry,
>
> After referring to the Information Technology - AT Attachment with Packet
> Interface (ATA/ATAPI) Specification,
> It seems to me that the commands being executed in function
> SetDriveParameters() are not mandatory during device initialization.
>
> 1. INITIALIZE DEVICE PARAMETERS command (ID 0x91h):
> This command got obsoleted since ATA/ATAPI-6 spec version.
> Also, the return status of SetDriveParameters() is irrelevant with the
> execution result of this command.
>
> 2. SET MULTIPLE MODE command (ID 0xC6h):
> My take is that this command is necessary if there is subsequent usage of
> command READ MULTIPLE, READ MULTIPLE EXT, WRITE MULTIPLE, or WRITE MULTIPLE
> EXT.
> I do not find usage of the above 4 commands within edk2, so I think the
> successful execution of SET MULTIPLE MODE command is not mandatory for
> detecting hard disk device.
>
> Based on the above findings, could you help to update the patch to:
>   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
>   if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
> Status));
>   }
>
> Will doing so still please Coverity?
>
> Best Regards,
> Hao Wu
>
> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Friday, June 9, 2023 8:33 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray 
> > Subject: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
> > UNUSED_VALUE Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The return value stored in Status after call to SetDriveParameters
> > is not made of any use thereafter and hence it remains as UNUSED.
> >
> > Add error check as is done after calls to SetDeviceTransferMode.
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >
> > Notes:
> > Add error check instead of Status storage removal
> >
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > index 75403886e44a..d04b1d95a7f5 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > @@ -2549,13 +2549,18 @@ DetectAndConfigIdeDevice (
> >  //
> >
> >  if (DeviceType == EfiIdeHarddisk) {
> >
> >//
> >
> > -  // Init driver parameters
> >
> > +  // Init drive parameters
> >
> >//
> >
> >DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->sectors_per_track;
> >
> >DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> >
> >DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> >
> >
> >Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> >
> > +
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> > Status));
> >
> > +continue;
> >
> > +  }
> >
> >  }
> >
> >
> >
> >  //
> >
> > --
> > 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106871): https://edk2.groups.io/g/devel/message/106871
Mute This Topic: https://groups.io/mt/99432080/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

Thanks Hao for digging deeper into this.

The if block itself might get knocked off in Release mode when there is
only a DEBUG statement inside it and hence Coverity might still complain.
So, we can override the Status value in this scenario inside the if block
and then proceed normally - let me know if this is acceptable and I will
update the patch as below then

  if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
Status));
/* Ignore Warning and proceed normally */
Status = 0;
  }

Best Regards,
Ranbir Singh

On Wed, Jul 12, 2023 at 10:08 AM Wu, Hao A  wrote:

> Really sorry,
>
> After referring to the Information Technology - AT Attachment with Packet
> Interface (ATA/ATAPI) Specification,
> It seems to me that the commands being executed in function
> SetDriveParameters() are not mandatory during device initialization.
>
> 1. INITIALIZE DEVICE PARAMETERS command (ID 0x91h):
> This command got obsoleted since ATA/ATAPI-6 spec version.
> Also, the return status of SetDriveParameters() is irrelevant with the
> execution result of this command.
>
> 2. SET MULTIPLE MODE command (ID 0xC6h):
> My take is that this command is necessary if there is subsequent usage of
> command READ MULTIPLE, READ MULTIPLE EXT, WRITE MULTIPLE, or WRITE MULTIPLE
> EXT.
> I do not find usage of the above 4 commands within edk2, so I think the
> successful execution of SET MULTIPLE MODE command is not mandatory for
> detecting hard disk device.
>
> Based on the above findings, could you help to update the patch to:
>   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> &DriveParameters, NULL);
>
>   if (EFI_ERROR (Status)) {
> DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n",
> Status));
>   }
>
> Will doing so still please Coverity?
>
> Best Regards,
> Hao Wu
>
> > -Original Message-
> > From: Ranbir Singh 
> > Sent: Friday, June 9, 2023 8:33 PM
> > To: devel@edk2.groups.io; rsi...@ventanamicro.com
> > Cc: Wu, Hao A ; Ni, Ray 
> > Subject: [PATCH v2 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix
> > UNUSED_VALUE Coverity issue
> >
> > From: Ranbir Singh 
> >
> > The return value stored in Status after call to SetDriveParameters
> > is not made of any use thereafter and hence it remains as UNUSED.
> >
> > Add error check as is done after calls to SetDeviceTransferMode.
> >
> > Cc: Hao A Wu 
> > Cc: Ray Ni 
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
> > Signed-off-by: Ranbir Singh 
> > Signed-off-by: Ranbir Singh 
> > ---
> >
> > Notes:
> > Add error check instead of Status storage removal
> >
> >  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > index 75403886e44a..d04b1d95a7f5 100644
> > --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> > @@ -2549,13 +2549,18 @@ DetectAndConfigIdeDevice (
> >  //
> >
> >  if (DeviceType == EfiIdeHarddisk) {
> >
> >//
> >
> > -  // Init driver parameters
> >
> > +  // Init drive parameters
> >
> >//
> >
> >DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->sectors_per_track;
> >
> >DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->heads - 1);
> >
> >DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA
> > *)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
> >
> >
> >
> >Status = SetDriveParameters (Instance, IdeChannel, IdeDevice,
> > &DriveParameters, NULL);
> >
> > +
> >
> > +  if (EFI_ERROR (Status)) {
> >
> > +DEBUG ((DEBUG_ERROR, "Set Drive Parameters Fail, Status = %r\n",
> > Status));
> >
> > +continue;
> >
> > +  }
> >
> >  }
> >
> >
> >
> >  //
> >
> > --
> > 2.34.1
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106870): https://edk2.groups.io/g/devel/message/106870
Mute This Topic: https://groups.io/mt/99432080/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

[Ranbir Singh]

v2 -> v3:
  - [Patch 2] Update as per review comments

v1 -> v2:
  - Retain outer cast
  - Add error check instead of Status storage removal

Ranbir Singh (2):
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c |  2 +-
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  | 10 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106917): https://edk2.groups.io/g/devel/message/106917
Mute This Topic: https://groups.io/mt/100124814/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
Reviewed-by: Hao A Wu 
---

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..f39c909d0631 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
 // Check logical block size
 //
 if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
-  BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) 
| IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+  BlockSize = 
(UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | 
IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106918): https://edk2.groups.io/g/devel/message/106918
Mute This Topic: https://groups.io/mt/100124816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.

Add error check as is done after calls to SetDeviceTransferMode.

Cc: Hao A Wu 
Cc: Ray Ni 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4204
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..af022139cf02 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -2549,13 +2549,21 @@ DetectAndConfigIdeDevice (
 //
 if (DeviceType == EfiIdeHarddisk) {
   //
-  // Init driver parameters
+  // Init drive parameters
   //
   DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->sectors_per_track;
   DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->heads - 1);
   DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
 
   Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, 
&DriveParameters, NULL);
+
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_WARN, "Set Drive Parameters Fail, Status = %r\n", 
Status));
+//
+// Ignore warning and proceed normally
+//
+Status = EFI_SUCCESS;
+  }
 }
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106919): https://edk2.groups.io/g/devel/message/106919
Mute This Topic: https://groups.io/mt/100124817/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

>
> To please both sides, how about:
> 1. Remove the 'Status' assignment of the return value from
> SetDriveParameters()
> Based on my findings (https://edk2.groups.io/g/devel/message/106844), the
> successful execution of SetDriveParameters() is not mandatory for
> initializing
> IDE mode hard disk device.
>

Interestingly, my very first patch for this began with this approach only.


> 2. Add DEBUG_WARN level debug message within SetDriveParameters() function
> In function SetDriveParameters, for the 2 calls of AtaNonDataCommandIn (one
> for the INITIALIZE DEVICE PARAMETERS command and the other for SET MULTIPLE
> MODE command), if the return status is not EFI_SUCCESS, add debug message
> to
> display the information.
>

These can be added if desired so.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106935): https://edk2.groups.io/g/devel/message/106935
Mute This Topic: https://groups.io/mt/100124817/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 0/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix

[Ranbir Singh]

v3 -> v4:
  - [Patch 2] Further update as per review comments
  - Status storage removal at call point
  - Error checks moved inside SetDriveParameters function 

v2 -> v3:
  - [Patch 2] Update as per review comments

v1 -> v2:
  - Retain outer cast
  - Add error check instead of Status storage removal

Ranbir Singh (2):
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity
issue
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c |  2 +-
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c  | 12 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106988): https://edk2.groups.io/g/devel/message/106988
Mute This Topic: https://groups.io/mt/100212103/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 0/1] MdeModulePkg/Bus/Ata/AtaBusDxe: Fix issues

[Ranbir Singh]

v1 -> v2:
  - Retain outer cast

Ranbir Singh (1):
  MdeModulePkg/Bus/Ata/AtaBusDxe: Fix SIGN_EXTENSION Coverity issue

 MdeModulePkg/Bus/Ata/AtaBusDxe/AtaPassThruExecute.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106991): https://edk2.groups.io/g/devel/message/106991
Mute This Topic: https://groups.io/mt/100212106/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 1/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix SIGN_EXTENSION Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

Line number 1348 does contain a typecast with UINT32, but it is after
all the operations (16-bit left shift followed by OR'ing) are over.
To avoid any SIGN_EXTENSION, typecast the intermediate result after
16-bit left shift operation immediately with UINT32.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
Reviewed-by: Hao A Wu 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 50406fe0270d..f39c909d0631 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -1345,7 +1345,7 @@ AtaPassThruPassThru (
 // Check logical block size
 //
 if ((IdentifyData->AtaData.phy_logic_sector_support & BIT12) != 0) {
-  BlockSize = (UINT32)(((IdentifyData->AtaData.logic_sector_size_hi << 16) 
| IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
+  BlockSize = 
(UINT32)(((UINT32)(IdentifyData->AtaData.logic_sector_size_hi << 16) | 
IdentifyData->AtaData.logic_sector_size_lo) * sizeof (UINT16));
 }
   }
 
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106989): https://edk2.groups.io/g/devel/message/106989
Mute This Topic: https://groups.io/mt/100212104/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v4 2/2] MdeModulePkg/Bus/Ata/AtaAtapiPassThru: Fix UNUSED_VALUE Coverity issue

[Ranbir Singh]

From: Ranbir Singh 

The return value stored in Status after call to SetDriveParameters
is not made of any use thereafter and hence it remains as UNUSED.

Based on Hao's findings (https://edk2.groups.io/g/devel/message/106844),
the successful execution of SetDriveParameters() is not mandatory for
initializing IDE mode of a hard disk device. Hence remove the 'Status'
assignment of the return value from SetDriveParameters() and instead add
error checks & DEBUG_WARN level messages within SetDriveParameters()
function after sending INIT_DRIVE_PARAM & SET_MULTIPLE_MODE ATA commands.

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

Cc: Hao A Wu 
Cc: Ray Ni 
Signed-off-by: Ranbir Singh 
Signed-off-by: Ranbir Singh 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index 75403886e44a..19d7b4930cb7 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -1992,6 +1992,10 @@ SetDriveParameters (
  NULL
  );
 
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_WARN, "Init Drive Parameters Fail, Status = %r\n", Status));
+  }
+
   //
   // Send Set Multiple parameters
   //
@@ -2008,6 +2012,10 @@ SetDriveParameters (
  NULL
  );
 
+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_WARN, "Set Multiple Mode Parameters Fail, Status = %r\n", 
Status));
+  }
+
   return Status;
 }
 
@@ -2549,13 +2557,13 @@ DetectAndConfigIdeDevice (
 //
 if (DeviceType == EfiIdeHarddisk) {
   //
-  // Init driver parameters
+  // Init drive parameters
   //
   DriveParameters.Sector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->sectors_per_track;
   DriveParameters.Heads  = (UINT8)(((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->heads - 1);
   DriveParameters.MultipleSector = (UINT8)((ATA5_IDENTIFY_DATA 
*)(&Buffer.AtaData))->multi_sector_cmd_max_sct_cnt;
 
-  Status = SetDriveParameters (Instance, IdeChannel, IdeDevice, 
&DriveParameters, NULL);
+  SetDriveParameters (Instance, IdeChannel, IdeDevice, &DriveParameters, 
NULL);
 }
 
 //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106990): https://edk2.groups.io/g/devel/message/106990
Mute This Topic: https://groups.io/mt/100212105/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-