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