[PATCH v2 0/3] OvmfPkg: Check arguments for validity

[PATCH v2 0/3] OvmfPkg: Check arguments for validity

[Dimitrije Pavlov]

Some functions across OVMF don't check pointer arguments for
validity, which causes null pointer dereferences and crashes
in the SCT test suite.

This series adds checks to return EFI_INVALID_PARAMETER if a
pointer argument is NULL.

v2:
- Add Liming Gao to Cc [Ard]
- Turn individual patches into a series [Ard]
- Fix issue with corrupted patches [Ard]

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Dimitrije Pavlov (3):
  OvmfPkg/PlatformDxe: Check ExtractConfig and RouteConfig arguments
  OvmfPkg/VirtioGpuDxe: Check QueryMode arguments
  OvmfPkg/VirtioFsDxe: Check GetDriverName arguments

 OvmfPkg/PlatformDxe/Platform.c      | 8 ++++++++
 OvmfPkg/VirtioFsDxe/DriverBinding.c | 4 ++++
 OvmfPkg/VirtioGpuDxe/Gop.c          | 4 +++-
 3 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.37.2

[PATCH v2 1/3] OvmfPkg/PlatformDxe: Check ExtractConfig and RouteConfig arguments

[Dimitrije Pavlov]

The current implementation does not check if Progress or Results
pointers in ExtractConfig are NULL, or if Progress pointer in
RouteConfig is NULL. This causes the SCT test suite to crash.

Add a check to return EFI_INVALID_PARAMETER if any of these pointers
are NULL.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
---
 OvmfPkg/PlatformDxe/Platform.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 4bf22712c78f..d7be2ab65efa 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -232,6 +232,10 @@ ExtractConfig (
 
   DEBUG ((DEBUG_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request));
 
+  if (Progress == NULL || Results == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   Status = PlatformConfigToFormState (&MainFormState);
   if (EFI_ERROR (Status)) {
     *Progress = Request;
@@ -340,6 +344,10 @@ RouteConfig (
     Configuration
     ));
 
+  if (Progress == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   //
   // the "read" step in RMW
   //
-- 
2.37.2

[PATCH v2 2/3] OvmfPkg/VirtioGpuDxe: Check QueryMode arguments

[Dimitrije Pavlov]

The current implementation does not check if Info or SizeInfo
pointers are NULL. This causes the SCT test suite to crash.

Add a check to return EFI_INVALID_PARAMETER if any of these
pointers are NULL.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
---
 OvmfPkg/VirtioGpuDxe/Gop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/VirtioGpuDxe/Gop.c b/OvmfPkg/VirtioGpuDxe/Gop.c
index 401db47672ec..bb68b1cdc2bc 100644
--- a/OvmfPkg/VirtioGpuDxe/Gop.c
+++ b/OvmfPkg/VirtioGpuDxe/Gop.c
@@ -308,7 +308,9 @@ GopQueryMode (
 {
   EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *GopModeInfo;
 
-  if (ModeNumber >= This->Mode->MaxMode) {
+  if (Info == NULL ||
+      SizeOfInfo == NULL ||
+      ModeNumber >= This->Mode->MaxMode) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.37.2

[PATCH v2 3/3] OvmfPkg/VirtioFsDxe: Check GetDriverName arguments

[Dimitrije Pavlov]

The current implementation does not check if Language or DriverName
are NULL. This causes the SCT test suite to crash.

Add a check to return EFI_INVALID_PARAMETER if any of these pointers
are NULL.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Signed-off-by: Dimitrije Pavlov <Dimitrije.Pavlov@arm.com>
---
 OvmfPkg/VirtioFsDxe/DriverBinding.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/OvmfPkg/VirtioFsDxe/DriverBinding.c b/OvmfPkg/VirtioFsDxe/DriverBinding.c
index 86eb9cf0ba51..3d80ff0f91f5 100644
--- a/OvmfPkg/VirtioFsDxe/DriverBinding.c
+++ b/OvmfPkg/VirtioFsDxe/DriverBinding.c
@@ -218,6 +218,10 @@ VirtioFsGetDriverName (
   OUT CHAR16                        **DriverName
   )
 {
+  if (Language == NULL || DriverName == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   if (AsciiStrCmp (Language, "en") != 0) {
     return EFI_UNSUPPORTED;
   }
-- 
2.37.2

Re: [PATCH v2 0/3] OvmfPkg: Check arguments for validity

[Ard Biesheuvel]

On Mon, 15 Aug 2022 at 18:31, Dimitrije Pavlov <dimitrije.pavlov@arm.com> wrote:
>
> Some functions across OVMF don't check pointer arguments for
> validity, which causes null pointer dereferences and crashes
> in the SCT test suite.
>
> This series adds checks to return EFI_INVALID_PARAMETER if a
> pointer argument is NULL.
>
> v2:
> - Add Liming Gao to Cc [Ard]
> - Turn individual patches into a series [Ard]
> - Fix issue with corrupted patches [Ard]
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Sunny Wang <Sunny.Wang@arm.com>
> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>
> Dimitrije Pavlov (3):
>   OvmfPkg/PlatformDxe: Check ExtractConfig and RouteConfig arguments
>   OvmfPkg/VirtioGpuDxe: Check QueryMode arguments
>   OvmfPkg/VirtioFsDxe: Check GetDriverName arguments
>

Thank you for the resend.

@Liming: these are all bug fixes that affect SCT results, so unless
there are any objections, I intend to merge these tomorrow (Tuesday).

Thanks,
Ard.

Re: [PATCH v2 0/3] OvmfPkg: Check arguments for validity

[Sunny Wang]

The series looks good to me.
Reviewed-by: Sunny Wang <sunny.wang@arm.com>

-----Original Message-----
From: Dimitrije Pavlov <dimitrije.pavlov@arm.com>
Sent: 15 August 2022 17:31
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Sunny Wang <Sunny.Wang@arm.com>; Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: [PATCH v2 0/3] OvmfPkg: Check arguments for validity

Some functions across OVMF don't check pointer arguments for
validity, which causes null pointer dereferences and crashes
in the SCT test suite.

This series adds checks to return EFI_INVALID_PARAMETER if a
pointer argument is NULL.

v2:
- Add Liming Gao to Cc [Ard]
- Turn individual patches into a series [Ard]
- Fix issue with corrupted patches [Ard]

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Sunny Wang <Sunny.Wang@arm.com>
Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

Dimitrije Pavlov (3):
  OvmfPkg/PlatformDxe: Check ExtractConfig and RouteConfig arguments
  OvmfPkg/VirtioGpuDxe: Check QueryMode arguments
  OvmfPkg/VirtioFsDxe: Check GetDriverName arguments

 OvmfPkg/PlatformDxe/Platform.c      | 8 ++++++++
 OvmfPkg/VirtioFsDxe/DriverBinding.c | 4 ++++
 OvmfPkg/VirtioGpuDxe/Gop.c          | 4 +++-
 3 files changed, 15 insertions(+), 1 deletion(-)

--
2.37.2

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v2 0/3] OvmfPkg: Check arguments for validity

[Ard Biesheuvel]

On Tue, 16 Aug 2022 at 13:39, Sunny Wang <Sunny.Wang@arm.com> wrote:
>
> The series looks good to me.
> Reviewed-by: Sunny Wang <sunny.wang@arm.com>
>
> -----Original Message-----
> From: Dimitrije Pavlov <dimitrije.pavlov@arm.com>
> Sent: 15 August 2022 17:31
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Sunny Wang <Sunny.Wang@arm.com>; Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Subject: [PATCH v2 0/3] OvmfPkg: Check arguments for validity
>
> Some functions across OVMF don't check pointer arguments for
> validity, which causes null pointer dereferences and crashes
> in the SCT test suite.
>
> This series adds checks to return EFI_INVALID_PARAMETER if a
> pointer argument is NULL.
>
> v2:
> - Add Liming Gao to Cc [Ard]
> - Turn individual patches into a series [Ard]
> - Fix issue with corrupted patches [Ard]
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Sunny Wang <Sunny.Wang@arm.com>
> Cc: Jeff Booher-Kaeding <Jeff.Booher-Kaeding@arm.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>
> Dimitrije Pavlov (3):
>   OvmfPkg/PlatformDxe: Check ExtractConfig and RouteConfig arguments
>   OvmfPkg/VirtioGpuDxe: Check QueryMode arguments
>   OvmfPkg/VirtioFsDxe: Check GetDriverName arguments
>

I tried to push these but they failed in CI.

Could you please have a look and respin with the reported issues
addressed? Thanks.

https://github.com/tianocore/edk2/pull/3211