[edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

[edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

[Santhosh Kumar V via groups.io]

Resolved INTEGER_OVERFLOW Coverity Issues in MNP Dxe
1.MnpStop,MnpInstanceDeliverPacket
Expression "MnpDeviceData->ConfiguredChildrenNumber--" and Instance->RcvdPacketQueueSize--  where Both variables are known to be equal to 0, underflows the type that receives it.


Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Signed-off-by: SanthoshKumarV <santhoshkumarv@ami.com>
---
 NetworkPkg/MnpDxe/MnpConfig.c | 3 ++-
 NetworkPkg/MnpDxe/MnpIo.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c
index 93587d53aa..80253e88de 100644
--- a/NetworkPkg/MnpDxe/MnpConfig.c
+++ b/NetworkPkg/MnpDxe/MnpConfig.c
@@ -1313,7 +1313,8 @@ MnpStop (
   //

   // Decrease the children number.

   //

-  MnpDeviceData->ConfiguredChildrenNumber--;

+  if (MnpDeviceData->ConfiguredChildrenNumber > 0)

+      MnpDeviceData->ConfiguredChildrenNumber--;



   if (MnpDeviceData->ConfiguredChildrenNumber > 0) {

     //

diff --git a/NetworkPkg/MnpDxe/MnpIo.c b/NetworkPkg/MnpDxe/MnpIo.c
index 087c879c46..492edd9b66 100644
--- a/NetworkPkg/MnpDxe/MnpIo.c
+++ b/NetworkPkg/MnpDxe/MnpIo.c
@@ -351,7 +351,8 @@ MnpInstanceDeliverPacket (
   // All resources are OK, remove the packet from the queue.

   //

   NetListRemoveHead (&Instance->RcvdPacketQueue);

-  Instance->RcvdPacketQueueSize--;

+  if (Instance->RcvdPacketQueueSize != 0)

+      Instance->RcvdPacketQueueSize--;



   RxData  = &RxDataWrap->RxData;

   SnpMode = MnpDeviceData->Snp->Mode;

--
2.42.0.windows.2
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


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

Re: [edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

[Saloni Kasbekar]

Both the cases have ASSERTs checking that the value is greater than 0. Move the checks to the same location as the ASSERT. I'd also prefer if we return with a failure instead of forcing continuation of the function with an invalid value.

Thanks,
Saloni

-----Original Message-----
From: Santhosh Kumar V <santhoshkumarv@ami.com> 
Sent: Friday, May 10, 2024 3:05 AM
To: devel@edk2.groups.io; Santhosh Kumar V <santhoshkumarv@ami.com>
Cc: Sivaraman Nainar <sivaramann@ami.com>; Raj V Akilan <rajva@ami.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>
Subject: [PATCH] NetworkPkg:MnpDxe:CoverityIssues

Resolved INTEGER_OVERFLOW Coverity Issues in MNP Dxe 1.MnpStop,MnpInstanceDeliverPacket
Expression "MnpDeviceData->ConfiguredChildrenNumber--" and Instance->RcvdPacketQueueSize--  where Both variables are known to be equal to 0, underflows the type that receives it.


Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Signed-off-by: SanthoshKumarV <santhoshkumarv@ami.com>
---
 NetworkPkg/MnpDxe/MnpConfig.c | 3 ++-
 NetworkPkg/MnpDxe/MnpIo.c     | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c index 93587d53aa..80253e88de 100644
--- a/NetworkPkg/MnpDxe/MnpConfig.c
+++ b/NetworkPkg/MnpDxe/MnpConfig.c
@@ -1313,7 +1313,8 @@ MnpStop (
   //

   // Decrease the children number.

   //

-  MnpDeviceData->ConfiguredChildrenNumber--;

+  if (MnpDeviceData->ConfiguredChildrenNumber > 0)

+      MnpDeviceData->ConfiguredChildrenNumber--;



   if (MnpDeviceData->ConfiguredChildrenNumber > 0) {

     //

diff --git a/NetworkPkg/MnpDxe/MnpIo.c b/NetworkPkg/MnpDxe/MnpIo.c index 087c879c46..492edd9b66 100644
--- a/NetworkPkg/MnpDxe/MnpIo.c
+++ b/NetworkPkg/MnpDxe/MnpIo.c
@@ -351,7 +351,8 @@ MnpInstanceDeliverPacket (
   // All resources are OK, remove the packet from the queue.

   //

   NetListRemoveHead (&Instance->RcvdPacketQueue);

-  Instance->RcvdPacketQueueSize--;

+  if (Instance->RcvdPacketQueueSize != 0)

+      Instance->RcvdPacketQueueSize--;



   RxData  = &RxDataWrap->RxData;

   SnpMode = MnpDeviceData->Snp->Mode;

--
2.42.0.windows.2
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


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

[edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

[Santhosh Kumar V via groups.io]

Both the cases have ASSERTs checking that the value is greater than 0. Move the checks to the same location as the ASSERT. I'd also prefer if we return with a failure instead of forcing continuation of the function with an invalid value.

Thanks,
Saloni


Addressed above comments and made changes, please review the latest changes below and kindly review coverity changes for other modules too.

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Signed-off-by: SanthoshKumarV <santhoshkumarv@ami.com>
---
 NetworkPkg/MnpDxe/MnpConfig.c | 3 +++
 NetworkPkg/MnpDxe/MnpIo.c     | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c
index 93587d53aa..12fc80ad0c 100644
--- a/NetworkPkg/MnpDxe/MnpConfig.c
+++ b/NetworkPkg/MnpDxe/MnpConfig.c
@@ -1304,6 +1304,9 @@ MnpStop (
   NET_CHECK_SIGNATURE (MnpServiceData, MNP_SERVICE_DATA_SIGNATURE);

   MnpDeviceData = MnpServiceData->MnpDeviceData;

   ASSERT (MnpDeviceData->ConfiguredChildrenNumber > 0);

+

+  if (MnpDeviceData->ConfiguredChildrenNumber <= 0)

+    return EFI_OUT_OF_RESOURCES;



   //

   // Configure the receive filters.

diff --git a/NetworkPkg/MnpDxe/MnpIo.c b/NetworkPkg/MnpDxe/MnpIo.c
index 087c879c46..24dfad10fa 100644
--- a/NetworkPkg/MnpDxe/MnpIo.c
+++ b/NetworkPkg/MnpDxe/MnpIo.c
@@ -326,6 +326,9 @@ MnpInstanceDeliverPacket (


   ASSERT (Instance->RcvdPacketQueueSize != 0);



+  if (Instance->RcvdPacketQueueSize == 0)

+    return EFI_OUT_OF_RESOURCES;

+

   RxDataWrap = NET_LIST_HEAD (&Instance->RcvdPacketQueue, MNP_RXDATA_WRAP, WrapEntry);

   if (RxDataWrap->Nbuf->RefCnt > 2) {

     //

--
2.42.0.windows.2
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


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

Re: [edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

[Saloni Kasbekar]

Could you please update the commit message for the patch below? Also, all your patches seem to result in an error after applying them. Could you help resend them with the fix?

warning: quoted CRLF detected
error: corrupt patch at line 19

Thanks,
Saloni

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Santhosh Kumar V via groups.io
Sent: Tuesday, May 21, 2024 7:14 AM
To: devel@edk2.groups.io; Santhosh Kumar V <santhoshkumarv@ami.com>
Cc: Sivaraman Nainar <sivaramann@ami.com>; Raj V Akilan <rajva@ami.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>
Subject: [edk2-devel] [PATCH] NetworkPkg:MnpDxe:CoverityIssues

Both the cases have ASSERTs checking that the value is greater than 0. Move the checks to the same location as the ASSERT. I'd also prefer if we return with a failure instead of forcing continuation of the function with an invalid value.

Thanks,
Saloni


Addressed above comments and made changes, please review the latest changes below and kindly review coverity changes for other modules too.

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Signed-off-by: SanthoshKumarV <santhoshkumarv@ami.com>
---
 NetworkPkg/MnpDxe/MnpConfig.c | 3 +++
 NetworkPkg/MnpDxe/MnpIo.c     | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/NetworkPkg/MnpDxe/MnpConfig.c b/NetworkPkg/MnpDxe/MnpConfig.c index 93587d53aa..12fc80ad0c 100644
--- a/NetworkPkg/MnpDxe/MnpConfig.c
+++ b/NetworkPkg/MnpDxe/MnpConfig.c
@@ -1304,6 +1304,9 @@ MnpStop (
   NET_CHECK_SIGNATURE (MnpServiceData, MNP_SERVICE_DATA_SIGNATURE);

   MnpDeviceData = MnpServiceData->MnpDeviceData;

   ASSERT (MnpDeviceData->ConfiguredChildrenNumber > 0);

+

+  if (MnpDeviceData->ConfiguredChildrenNumber <= 0)

+    return EFI_OUT_OF_RESOURCES;



   //

   // Configure the receive filters.

diff --git a/NetworkPkg/MnpDxe/MnpIo.c b/NetworkPkg/MnpDxe/MnpIo.c index 087c879c46..24dfad10fa 100644
--- a/NetworkPkg/MnpDxe/MnpIo.c
+++ b/NetworkPkg/MnpDxe/MnpIo.c
@@ -326,6 +326,9 @@ MnpInstanceDeliverPacket (


   ASSERT (Instance->RcvdPacketQueueSize != 0);



+  if (Instance->RcvdPacketQueueSize == 0)

+    return EFI_OUT_OF_RESOURCES;

+

   RxDataWrap = NET_LIST_HEAD (&Instance->RcvdPacketQueue, MNP_RXDATA_WRAP, WrapEntry);

   if (RxDataWrap->Nbuf->RefCnt > 2) {

     //

--
2.42.0.windows.2
-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.







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