public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v2 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity
@ 2023-10-09 11:28 Ranbir Singh
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue Ranbir Singh
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
  0 siblings, 2 replies; 5+ messages in thread
From: Ranbir Singh @ 2023-10-09 11:28 UTC (permalink / raw)
  To: devel, rsingh

v1 -> v2:
  - Rebased to latest master HEAD
  - Changed the solution wrt MISSING_BREAK issues

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 | 6 ++++++
 MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouse.c | 4 ++++
 2 files changed, 10 insertions(+)

-- 
2.34.1



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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  2023-10-09 11:28 [edk2-devel] [PATCH v2 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity Ranbir Singh
@ 2023-10-09 11:28 ` Ranbir Singh
  2023-10-09 11:37   ` Laszlo Ersek
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
  1 sibling, 1 reply; 5+ messages in thread
From: Ranbir Singh @ 2023-10-09 11:28 UTC (permalink / raw)
  To: devel, rsingh; +Cc: Hao A Wu, Ray Ni, Veeresh Sangolli

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 (#109456): https://edk2.groups.io/g/devel/message/109456
Mute This Topic: https://groups.io/mt/101849997/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues
  2023-10-09 11:28 [edk2-devel] [PATCH v2 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity Ranbir Singh
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue Ranbir Singh
@ 2023-10-09 11:28 ` Ranbir Singh
  2023-10-09 11:47   ` Laszlo Ersek
  1 sibling, 1 reply; 5+ messages in thread
From: Ranbir Singh @ 2023-10-09 11:28 UTC (permalink / raw)
  To: devel, rsingh; +Cc: Hao A Wu, Ray Ni

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

There is no possibility of the if blocks within case 2: and case 3: to
succeed later and not succeed in the original case and hence the fall
throughs even if it hypothetically happens are redundant as the code
still will eventually return NULL only at the function end point.

Better introduce straight forward break; statement within actual cases.

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>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
index acc19acd98e0..f07e48774a34 100644
--- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
+++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
@@ -89,6 +89,8 @@ GetNextHidItem (
           return StartPos;
         }
 
+        break;
+
       case 2:
         //
         // 2-byte data
@@ -99,6 +101,8 @@ GetNextHidItem (
           return StartPos;
         }
 
+        break;
+
       case 3:
         //
         // 4-byte data, adjust size
@@ -109,6 +113,8 @@ GetNextHidItem (
           StartPos += 4;
           return StartPos;
         }
+
+        break;
     }
   }
 
-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue Ranbir Singh
@ 2023-10-09 11:37   ` Laszlo Ersek
  0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2023-10-09 11:37 UTC (permalink / raw)
  To: devel, rsingh; +Cc: Hao A Wu, Ray Ni, Veeresh Sangolli

On 10/9/23 13:28, Ranbir Singh wrote:
> 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;

With this (good) fix, it's better to remove the ASSERT() altogether, in
my opinion! The assert was a cop-out, and now you are replacing it with
proper error checking (and cleanup, too), so there's no need for the
ASSERT(). It's not an "invariant" that AllocateZeroPool must succeed, so
whenever it fails, it's not an invariant violation (programming error).

With the ASSERT() dropped:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues
  2023-10-09 11:28 ` [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
@ 2023-10-09 11:47   ` Laszlo Ersek
  0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2023-10-09 11:47 UTC (permalink / raw)
  To: devel, rsingh; +Cc: Hao A Wu, Ray Ni

On 10/9/23 13:28, Ranbir Singh wrote:
> The function GetNextHidItem has a switch-case block in which the case 1:
> falls through to case 2: and then case 2: falls through to case 3:.
> 
> There is no possibility of the if blocks within case 2: and case 3: to
> succeed later and not succeed in the original case and hence the fall
> throughs even if it hypothetically happens are redundant as the code
> still will eventually return NULL only at the function end point.
> 
> Better introduce straight forward break; statement within actual cases.
> 
> 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>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> index acc19acd98e0..f07e48774a34 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> @@ -89,6 +89,8 @@ GetNextHidItem (
>            return StartPos;
>          }
>  
> +        break;
> +
>        case 2:
>          //
>          // 2-byte data
> @@ -99,6 +101,8 @@ GetNextHidItem (
>            return StartPos;
>          }
>  
> +        break;
> +
>        case 3:
>          //
>          // 4-byte data, adjust size
> @@ -109,6 +113,8 @@ GetNextHidItem (
>            StartPos += 4;
>            return StartPos;
>          }
> +
> +        break;
>      }
>    }
>  

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-10-09 11:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-09 11:28 [edk2-devel] [PATCH v2 0/2] BZ 4222: Fix MdeModulePkg/Bus/Usb/UsbMouseDxe issues pointed by Coverity Ranbir Singh
2023-10-09 11:28 ` [edk2-devel] [PATCH v2 1/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix REVERSE_INULL Coverity issue Ranbir Singh
2023-10-09 11:37   ` Laszlo Ersek
2023-10-09 11:28 ` [edk2-devel] [PATCH v2 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
2023-10-09 11:47   ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox