From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 5829E7803CF for ; Mon, 14 Aug 2023 09:22:06 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ZCKkxgIuz+SXMs8ClU4CBdCotU5ZnjZWw5h2sKQLRaw=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1692004924; v=1; b=E4/x3rN5Exn4LWmv22JS5ixwl0geTlEo4gwBNeukryryhf0ZfxL7isaIwMpGHMsxqFFeRN39 pxqIFBPXbuHjRyc6KJrVIeSGXHn3YMPgAdmV++z3qvJEl2pyiKJKYxhIF7EF01K0skjEiN+Nnpm POYPnPVYvfvBJto2Lv67Kkf4= X-Received: by 127.0.0.2 with SMTP id fpIaYY7687511x0ulSlx2ixu; Mon, 14 Aug 2023 02:22:04 -0700 X-Received: from mail-oa1-f43.google.com (mail-oa1-f43.google.com [209.85.160.43]) by mx.groups.io with SMTP id smtpd.web10.103840.1692004924204645753 for ; Mon, 14 Aug 2023 02:22:04 -0700 X-Received: by mail-oa1-f43.google.com with SMTP id 586e51a60fabf-1c4d5cc56e2so347205fac.2 for ; Mon, 14 Aug 2023 02:22:04 -0700 (PDT) X-Gm-Message-State: 8Sgc29GMOzsT6k4GR3vyknC6x7686176AA= X-Google-Smtp-Source: AGHT+IH/KE4PCkxEhlqp8RcXqfP4kcKYPqSywPwGMhX4Z+IBOByhdq7gPGYX8VqAYbD4x0re6tV1mGqqB8dFaGc2NjY= X-Received: by 2002:a05:6870:e2d6:b0:1bf:fd8a:826e with SMTP id w22-20020a056870e2d600b001bffd8a826emr7681455oad.55.1692004923322; Mon, 14 Aug 2023 02:22:03 -0700 (PDT) MIME-Version: 1.0 References: <20230717113831.2290882-1-rsingh@ventanamicro.com> <20230717113831.2290882-2-rsingh@ventanamicro.com> In-Reply-To: From: "Ranbir Singh" Date: Mon, 14 Aug 2023 14:51:49 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT Coverity issue To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , "Ni, Ray" , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="00000000000069c8440602de9897" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="E4/x3rN5"; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --00000000000069c8440602de9897 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable When code is compiled in RELEASE mode, the ASSERT (Interval !=3D 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=E2=80=AFPM Wu, Hao A wrot= e: > 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 !=3D 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 > *Sent:* Monday, August 14, 2023 3:49 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray ; 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=E2=80=AFAM Wu, Hao A wr= ote: > > > -----Original Message----- > > From: Ranbir Singh > > Sent: Monday, July 17, 2023 7:39 PM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Wu, Hao A ; Ni, Ray ; Veeresh > > Sangolli > > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT > > Coverity issue > > > > From: Ranbir Singh > > > > The function UhciConvertPollRate has a check > > > > ASSERT (Interval !=3D 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=3D4211 > > > > Cc: Hao A Wu > > Cc: Ray Ni > > Co-authored-by: Veeresh Sangolli > > Signed-off-by: Ranbir Singh > > Signed-off-by: Ranbir Singh > > --- > > 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 !=3D 0); > > > > > > > > + if (Interval =3D=3D 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 =3D 0; Index < UHCI_FRAME_NUM; Index +=3D 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 DEBU= G > statement and/or an ASSERT as well - return value then has to be NULL fro= m > 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 f= or > '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 head= er > 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 > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --00000000000069c8440602de9897 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
When code is compiled in RELEASE mode, the ASSERT (Interva= l !=3D 0); statement gets NULL'ified. Hence for Coverity it simply does= n't exist. Further,=C2=A0IMO Coverity seems to look at a function in is= olation whether it is safe or not. It is however not necessary to fix all i= ssues pointed out by Coverity if something looks to be a false positive or = something is done deliberately.

I will go=C2=A0by return= ing 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 &#= 39;0' is sent.

Should I do this or = not - update the function header as well to reflect the minimum 'Interv= al' valid value as 1 and that if 0 is wrongly sent, it will be treated = as 1 only in RELEASE mode ?

<= div dir=3D"ltr" class=3D"gmail_attr">On Mon, Aug 14, 2023 at 2:09=E2=80=AFP= M Wu, Hao A <hao.a.wu@intel.com> wrote:

My take is that:

For all the possible calling scenario of UhciConvert= PollRate() in current

UhciDxe driver implementation, it is guaranteed that= the input parameter

'Interval' will not be 0.

=C2=A0

I think this is why the "ASSERT (Interval !=3D = 0);" statement is put here to

indicate such case will never happen and notify deve= lopers for future changes

in this driver that something is wrong.

=C2=A0

I do not know why Coverity is unable to figure this = out.

=C2=A0

My personal preference is to return 1 (i.e. 1 <&l= t; 0) as if the minimum allowed

value for 'Interval' (which is 1) is being p= assed into this UhciConvertPollRate

function if anything does go wrong brought by future= changes.

=C2=A0

Best Regards,

Hao Wu

=C2=A0

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@delltea= m.com>
Subject: Re: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SH= IFT Coverity issue

=C2=A0

=C2=A0

On Thu, Aug 10, 2023 at 8:09=E2=80=AFAM Wu, Hao A &l= t;hao.a.wu@intel.co= m> wrote:

> -----Original Mess= age-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@ed= k2.groups.io; rsingh@ventana= micro.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
>
>=C2=A0 =C2=A0 =C2=A0ASSERT (Interval !=3D 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,<= br> > it is better to handle it as well by simply returning ZERO.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4211
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <r= ay.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>
> ---
>=C2=A0 MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 ++++
>=C2=A0 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 (
>
>
>=C2=A0 =C2=A0 ASSERT (Interval !=3D 0);
>
>
>
> +=C2=A0 if (Interval =3D=3D 0) {
>
> +=C2=A0 =C2=A0 return 0;


Return 0 will cause further issues within UhciLinkQhToFrameList() &
UhciUnlinkQhFromFrameList() where the returned value is being used in the b= elow
'for' statement:

for (Index =3D 0; Index < UHCI_FRAME_NUM; Index +=3D Qh->Interval) {<= u>

=C2=A0

One way is to prohibit UhciCreateQh() to proceed nor= mally by checking if Interval parameter=C2=A0value is 0 at the very beginni= ng and may be add a DEBUG statement=C2=A0and/or an ASSERT as well - return = value then has to be NULL from this function for this specific case of invalid Interval parameter=C2=A0value being rece= ived as 0. That should take care of the issue Hao pointed above in for loop= .

=C2=A0

UhciCreateAsyncReq() may also need to introduce a Po= lling Interval parameter value check similar to as it exists in Uhci2AsyncI= nterruptTransfer() for a future direct call scenario.

=C2=A0

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 UhciConvertPollRa= te function.

=C2=A0

If we choose to modify UhciCreateQh(), then we may h= ave status quo in=C2=A0UhciConvertPollRate(). However, we can still pursue = the return 1 (i.e. 1 << 0)=C2=A0option here in which case I guess I s= hould update the function header as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is wrongly s= ent, it will be treated as 1 only. When we do that in RELEASE mode, should we still retain ASSERT ? I t= hink NO.

=C2=A0

If we choose to simply modify UhciConvertPollRate() = as stated, then we can=C2=A0have the status quo in=C2=A0UhciCreateQh() and = UhciCreateAsyncReq().

=C2=A0


Best Regards,
Hao Wu


>
> +=C2=A0 }
>
> +
>
>=C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 // Find the index (1 based) of the highest non-zero bit >
>=C2=A0 =C2=A0 //
>
> --
> 2.34.1

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#107730) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--00000000000069c8440602de9897--