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 62386740038 for ; Mon, 14 Aug 2023 07:49:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KyZhzy9unhCd+EXg0VFua3FkF1O0+FBrIbPYdD9ZRXo=; 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=1691999364; v=1; b=IDHdSFgG5GX6pkYg8o99ZF8vQnPx2YLQGAtdWAKGQzQrU+mvixv2glb2SNzd3hfsfU7vRhdn jAwTB0P0nyXPOmsFasq2oVFhS0f6Tz0W6l3XUdcIksJQTRE7cUw2of/usV5ycpLsG7//JGmlSmG 8hTdxH66b+Ug/WaVepcEpS9E= X-Received: by 127.0.0.2 with SMTP id FZ7eYY7687511x5W5GtKuoBh; Mon, 14 Aug 2023 00:49:24 -0700 X-Received: from mail-ot1-f44.google.com (mail-ot1-f44.google.com [209.85.210.44]) by mx.groups.io with SMTP id smtpd.web10.102951.1691999363281924765 for ; Mon, 14 Aug 2023 00:49:23 -0700 X-Received: by mail-ot1-f44.google.com with SMTP id 46e09a7af769-6bcaa6d5e2cso3578028a34.3 for ; Mon, 14 Aug 2023 00:49:23 -0700 (PDT) X-Gm-Message-State: y0lb9TqyNKtpWGeuXnzQP5o9x7686176AA= X-Google-Smtp-Source: AGHT+IECyOXIH1UItxxC7iNHebkpXBAc8xEAPJ6pGVF99YdcHhPUVS+6wnoZ/TtqNlJ3SVxSRZegwQEojjCoDyBgbqE= X-Received: by 2002:a9d:740a:0:b0:6b9:ed64:1423 with SMTP id n10-20020a9d740a000000b006b9ed641423mr8859406otk.2.1691999362439; Mon, 14 Aug 2023 00:49:22 -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 13:19:11 +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="000000000000f56d0a0602dd4cef" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=IDHdSFgG; 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 --000000000000f56d0a0602dd4cef Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Aug 10, 2023 at 8:09=E2=80=AFAM Wu, Hao A wrot= e: > > -----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 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 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 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 > > -=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 (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000f56d0a0602dd4cef Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Thu, Aug 10, 2023 at 8:09=E2=80=AFAM W= u, Hao A <hao.a.wu@intel.com&g= t; wrote:
> -= ----Original Message-----
> From: Ranbir Singh <rsingh@ventanamicro.com>
> Sent: Monday, July 17, 2023 7:39 PM
> To: devel@ed= k2.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
>
>=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_b= ug.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) {<= br>

One way is to prohibit UhciCreateQh() = to proceed normally by checking if Interval parameter=C2=A0value is 0 at th= e very beginning 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 specifi= c case of invalid Interval parameter=C2=A0value being received as 0. That s= hould take care of the issue Hao pointed above in for loop.

<= /div>
UhciCreateAsyncReq() may also need to introduce a Polling Interva= l parameter value check similar to as it exists in Uhci2AsyncInterruptTrans= fer() for a future direct call scenario.
=C2=A0
My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed val= ue for
'Interval' (which is 1) is being passed into this UhciConvertPollRa= te function.

If we choose to modify Uhc= iCreateQh(), then we may have status quo in=C2=A0UhciConvertPollRate(). How= ever, we can still pursue the return 1 (i.e. 1 << 0)=C2=A0option here= in which case I guess I should update the function header as well to refle= ct 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, sh= ould we still retain ASSERT ? I think NO.
=C2=A0
If we choose to simply modify UhciConvertPollRate() as stated, then we ca= n=C2=A0have the status quo in=C2=A0UhciCreateQh() and UhciCreateAsyncReq().=


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 (#107727) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000f56d0a0602dd4cef--