[edk2-devel] [PATCH v1 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity

[edk2-devel] [PATCH v1 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity

[Ranbir Singh]

Ranbir Singh (2):
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 ++++
 2 files changed, 12 insertions(+)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109307): https://edk2.groups.io/g/devel/message/109307
Mute This Topic: https://groups.io/mt/101750272/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function USBMouseDriverBindingStart do have

    ASSERT (UsbMouseDevice != NULL);

after AllocateZeroPool, but it is applicable only in DEBUG mode.
In RELEASE mode, if for whatever reasons UsbMouseDevice is NULL
at this point, the code proceeds to dereference "UsbMouseDevice"
afterwards which will lead to CRASH.

Hence, for safety add NULL pointer checks always.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222

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/Usb/UsbMouseDxe/UsbMouse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
index 451d4b934f4c..621d09713b24 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c
@@ -161,6 +161,10 @@ USBMouseDriverBindingStart (
 
   UsbMouseDevice = AllocateZeroPool (sizeof (USB_MOUSE_DEV));
   ASSERT (UsbMouseDevice != NULL);
+  if (UsbMouseDevice == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto ErrorExit;
+  }
 
   UsbMouseDevice->UsbIo     = UsbIo;
   UsbMouseDevice->Signature = USB_MOUSE_DEV_SIGNATURE;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109308): https://edk2.groups.io/g/devel/message/109308
Mute This Topic: https://groups.io/mt/101750273/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function GetNextHidItem has a switch-case code in which the
case 1: falls through to case 2: and then case 2: falls through
to case 3: in the block.

While this may be intentional, it is not evident to any general
code reader as well as any static analyzer tool. Just adding

    // No break; here as this is an intentional fallthrough.

as comment in between makes a reader as well as Coverity happy.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222

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/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
index acc19acd98e0..bc9a4824208b 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
@@ -89,6 +89,10 @@ GetNextHidItem (
           return StartPos;
         }
 
+        //
+        // No break; here as this is an intentional fallthrough
+        //
+
       case 2:
         //
         // 2-byte data
@@ -99,6 +103,10 @@ GetNextHidItem (
           return StartPos;
         }
 
+        //
+        // No break; here as this is an intentional fallthrough
+        //
+
       case 3:
         //
         // 4-byte data, adjust size
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109309): https://edk2.groups.io/g/devel/message/109309
Mute This Topic: https://groups.io/mt/101750274/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Michael D Kinney]

I do not prefer special comments for one static analyzer.

Is there an alternative design/implementation of this code
to make it more readable and not trigger any static analysis
false positives?

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> Singh
> Sent: Tuesday, October 3, 2023 10:48 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: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> Fix MISSING_BREAK Coverity issues
> 
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function GetNextHidItem has a switch-case code in which the
> case 1: falls through to case 2: and then case 2: falls through
> to case 3: in the block.
> 
> While this may be intentional, it is not evident to any general
> code reader as well as any static analyzer tool. Just adding
> 
>     // No break; here as this is an intentional fallthrough.
> 
> as comment in between makes a reader as well as Coverity happy.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> 
> 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/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> index acc19acd98e0..bc9a4824208b 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> @@ -89,6 +89,10 @@ GetNextHidItem (
>            return StartPos;
> 
>          }
> 
> 
> 
> +        //
> 
> +        // No break; here as this is an intentional fallthrough
> 
> +        //
> 
> +
> 
>        case 2:
> 
>          //
> 
>          // 2-byte data
> 
> @@ -99,6 +103,10 @@ GetNextHidItem (
>            return StartPos;
> 
>          }
> 
> 
> 
> +        //
> 
> +        // No break; here as this is an intentional fallthrough
> 
> +        //
> 
> +
> 
>        case 3:
> 
>          //
> 
>          // 4-byte data, adjust size
> 
> --
> 2.34.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#109309):
> https://edk2.groups.io/g/devel/message/109309
> Mute This Topic: https://groups.io/mt/101750274/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109333): https://edk2.groups.io/g/devel/message/109333
Mute This Topic: https://groups.io/mt/101750274/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

Hi Mike,

I earlier assumed that both the fallthrough were intentional and just made
it evident by writing the comment. It was hence written actually for the
code reader and not the static analyzer. The comment was similar in nature
and style to existing No/1-byte/... data comments. *Incidentally, the
comment satisfied the static analyzer as well. *It was beyond my
expectation and I was pleasantly surprised too. Without digging deep into
the particular code and to avoid any functional code change, I just thought
that's it.

Your comment forced me to come back specifically to the particular code and
to analyze it a bit more.

My analysis now says that the fallthrough doesn't serve any real functional
purpose as there is absolutely no chance of *if* blocks of the fallthrough
*case(*s) being executed later and not executed in the original *case:* and
hence the fallthrough if it happens still leads to execution of *return
NULL; *at the end of the function only. This is better communicated by
adding *break;* statement in the original *case:* and in a way, one may
agree with the static analyzer that there was a MISSING_BREAK. I should
hence follow it up with a v2 patch for this.

Ranbir Singh

On Thu, Oct 5, 2023 at 12:17 AM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> I do not prefer special comments for one static analyzer.
>
> Is there an alternative design/implementation of this code
> to make it more readable and not trigger any static analysis
> false positives?
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> > Singh
> > Sent: Tuesday, October 3, 2023 10:48 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: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> > Fix MISSING_BREAK Coverity issues
> >
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function GetNextHidItem has a switch-case code in which the
> > case 1: falls through to case 2: and then case 2: falls through
> > to case 3: in the block.
> >
> > While this may be intentional, it is not evident to any general
> > code reader as well as any static analyzer tool. Just adding
> >
> >     // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes a reader as well as Coverity happy.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> >
> > 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/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > index acc19acd98e0..bc9a4824208b 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > @@ -89,6 +89,10 @@ GetNextHidItem (
> >            return StartPos;
> >
> >          }
> >
> >
> >
> > +        //
> >
> > +        // No break; here as this is an intentional fallthrough
> >
> > +        //
> >
> > +
> >
> >        case 2:
> >
> >          //
> >
> >          // 2-byte data
> >
> > @@ -99,6 +103,10 @@ GetNextHidItem (
> >            return StartPos;
> >
> >          }
> >
> >
> >
> > +        //
> >
> > +        // No break; here as this is an intentional fallthrough
> >
> > +        //
> >
> > +
> >
> >        case 3:
> >
> >          //
> >
> >          // 4-byte data, adjust size
> >
> > --
> > 2.34.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#109309):
> > https://edk2.groups.io/g/devel/message/109309
> > Mute This Topic: https://groups.io/mt/101750274/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109342): https://edk2.groups.io/g/devel/message/109342
Mute This Topic: https://groups.io/mt/101750274/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: 7446 bytes --]

Re: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues

[Leif Lindholm]

On Wed, Oct 04, 2023 at 18:46:58 +0000, Michael D Kinney wrote:
> I do not prefer special comments for one static analyzer.
> 
> Is there an alternative design/implementation of this code
> to make it more readable and not trigger any static analysis
> false positives?

Not the tidiest, but could rewrite as:

    case 1:
    case 2:
    	 if (HidItem->Size == 1) {
	   <1 block>
	 }
	 <2 block>

That way we're making the code more expressive rather than keeping it
convoluted and adding a comment saying "code is convoluted".

Regards,

Leif

> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> > Singh
> > Sent: Tuesday, October 3, 2023 10:48 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: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:
> > Fix MISSING_BREAK Coverity issues
> > 
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > 
> > The function GetNextHidItem has a switch-case code in which the
> > case 1: falls through to case 2: and then case 2: falls through
> > to case 3: in the block.
> > 
> > While this may be intentional, it is not evident to any general
> > code reader as well as any static analyzer tool. Just adding
> > 
> >     // No break; here as this is an intentional fallthrough.
> > 
> > as comment in between makes a reader as well as Coverity happy.
> > 
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4222
> > 
> > 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/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > index acc19acd98e0..bc9a4824208b 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> > @@ -89,6 +89,10 @@ GetNextHidItem (
> >            return StartPos;
> > 
> >          }
> > 
> > 
> > 
> > +        //
> > 
> > +        // No break; here as this is an intentional fallthrough
> > 
> > +        //
> > 
> > +
> > 
> >        case 2:
> > 
> >          //
> > 
> >          // 2-byte data
> > 
> > @@ -99,6 +103,10 @@ GetNextHidItem (
> >            return StartPos;
> > 
> >          }
> > 
> > 
> > 
> > +        //
> > 
> > +        // No break; here as this is an intentional fallthrough
> > 
> > +        //
> > 
> > +
> > 
> >        case 3:
> > 
> >          //
> > 
> >          // 4-byte data, adjust size
> > 
> > --
> > 2.34.1
> > 
> > 
> > 
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#109309):
> > https://edk2.groups.io/g/devel/message/109309
> > Mute This Topic: https://groups.io/mt/101750274/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> > 
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109350): https://edk2.groups.io/g/devel/message/109350
Mute This Topic: https://groups.io/mt/101750274/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-