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 <rsi...@ventanamicro.com> Sent: Monday, August 14, 2023 3:49 PM To: Wu, Hao A <hao.a...@intel.com> Cc: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Veeresh Sangolli <veeresh.sango...@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...@intel.com<mailto:hao.a...@intel.com>> wrote: > -----Original Message----- > From: Ranbir Singh <rsi...@ventanamicro.com<mailto:rsi...@ventanamicro.com>> > Sent: Monday, July 17, 2023 7:39 PM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; > rsi...@ventanamicro.com<mailto:rsi...@ventanamicro.com> > Cc: Wu, Hao A <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>>; Veeresh > Sangolli <veeresh.sango...@dellteam.com<mailto:veeresh.sango...@dellteam.com>> > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT > Coverity issue > > From: Ranbir Singh <ranbir.sin...@dell.com<mailto:ranbir.sin...@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...@intel.com<mailto:hao.a...@intel.com>> > Cc: Ray Ni <ray...@intel.com<mailto:ray...@intel.com>> > Co-authored-by: Veeresh Sangolli > <veeresh.sango...@dellteam.com<mailto:veeresh.sango...@dellteam.com>> > Signed-off-by: Ranbir Singh > <ranbir.sin...@dell.com<mailto:ranbir.sin...@dell.com>> > Signed-off-by: Ranbir Singh > <rsi...@ventanamicro.com<mailto:rsi...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
