1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v1 0/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix issues pointed by Coverity
  4. View patch

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

Ranbir Singh posted 2 patches 2 years, 6 months ago
There is a newer version of this series
[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
Posted by Ranbir Singh 2 years, 6 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
Posted by Wu, Hao A 2 years, 6 months ago 
> -----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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
Posted by Ranbir Singh 2 years, 5 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
Posted by Wu, Hao A 2 years, 5 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue
Posted by Ranbir Singh 2 years, 5 months ago 
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.