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

["Ranbir Singh" <rsingh@ventanamicro.com>]

[-- 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 --]