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

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

[Ranbir Singh]

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 | 4 ++++
 MdeModulePkg/Bus/Pci/UhciDxe/UsbHcMem.c  | 9 +++++++++
 2 files changed, 13 insertions(+)

-- 
2.34.1



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

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

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

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 <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/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;
+  }
+
   //
   // 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 (#106994): https://edk2.groups.io/g/devel/message/106994
Mute This Topic: https://groups.io/mt/100212109/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

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 <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/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 (#106995): https://edk2.groups.io/g/devel/message/106995
Mute This Topic: https://groups.io/mt/100212110/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Wu, Hao A]

> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Veeresh
> Sangolli <veeresh.sangolli@dellteam.com>
> Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> Coverity issue
> 
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> 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 <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/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) {

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.

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 (#107678): https://edk2.groups.io/g/devel/message/107678
Mute This Topic: https://groups.io/mt/100212109/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Wu, Hao A]

Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Veeresh
> Sangolli <veeresh.sangolli@dellteam.com>
> Subject: [PATCH v1 2/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix
> FORWARD_NULL Coverity issues
> 
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> 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 <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/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 (#107680): https://edk2.groups.io/g/devel/message/107680
Mute This Topic: https://groups.io/mt/100212110/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]

On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A <hao.a.wu@intel.com> wrote:

> > -----Original Message-----
> > From: Ranbir Singh <rsingh@ventanamicro.com>
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Veeresh
> > Sangolli <veeresh.sangolli@dellteam.com>
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > 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 <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6164 bytes --]

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

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 5136 bytes --]

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 <rsingh@ventanamicro.com>
Sent: Monday, August 14, 2023 3:49 PM
To: Wu, Hao A <hao.a.wu@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@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 <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>> wrote:
> -----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
> Cc: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Veeresh
> Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> Coverity issue
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
>
> 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 <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> ---
>  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 (#107729): https://edk2.groups.io/g/devel/message/107729
Mute This Topic: https://groups.io/mt/100212109/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 10573 bytes --]

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

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 5969 bytes --]

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 <hao.a.wu@intel.com> 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 <rsingh@ventanamicro.com>
> *Sent:* Monday, August 14, 2023 3:49 PM
> *To:* Wu, Hao A <hao.a.wu@intel.com>
> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <
> veeresh.sangolli@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 <hao.a.wu@intel.com> wrote:
>
> > -----Original Message-----
> > From: Ranbir Singh <rsingh@ventanamicro.com>
> > Sent: Monday, July 17, 2023 7:39 PM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Veeresh
> > Sangolli <veeresh.sangolli@dellteam.com>
> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT
> > Coverity issue
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > 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 <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/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 (#107730): https://edk2.groups.io/g/devel/message/107730
Mute This Topic: https://groups.io/mt/100212109/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 10894 bytes --]