Browse Source

OvmfPkg/AcpiPlatformDxe: catch theoretical nullptr deref in Xen code

RH covscan justifiedly reports a path through InstallXenTables() where
DsdtTable can technically remain NULL.

If this occurs in practice, then the guest and the VMM are out of sync on
the interface contract. Catch the situation with a code snippet that halts
in RELEASE builds, and in DEBUG builds lets the platform DSC control the
assert disposition first (i.e. CPU exception, deadloop, or nothing).

> Error: CLANG_WARNING:
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:309:14: warning: Access
> to field 'Length' results in a dereference of a null pointer (loaded
> from variable 'DsdtTable')
> #             DsdtTable->Length,
> #             ^~~~~~~~~
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:154:3: note: Null
> pointer value stored to 'DsdtTable'
> #  DsdtTable   = NULL;
> #  ^~~~~~~~~~~~~~~~~~
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:162:3: note: Taking
> false branch
> #  if (EFI_ERROR (Status)) {
> #  ^
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:170:7: note: Assuming
> the condition is false
> #  if (XenAcpiRsdpStructurePtr->XsdtAddress) {
> #      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:170:3: note: Taking
> false branch
> #  if (XenAcpiRsdpStructurePtr->XsdtAddress) {
> #  ^
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:220:12: note: Assuming
> the condition is false
> #  else if (XenAcpiRsdpStructurePtr->RsdtAddress) {
> #           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:220:8: note: Taking
> false branch
> #  else if (XenAcpiRsdpStructurePtr->RsdtAddress) {
> #       ^
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:274:3: note: Taking
> false branch
> #  if (Fadt2Table) {
> #  ^
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:288:8: note: Taking
> false branch
> #  else if (Fadt1Table) {
> #       ^
> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/Xen.c:309:14: note: Access to
> field 'Length' results in a dereference of a null pointer (loaded from
> variable 'DsdtTable')
> #             DsdtTable->Length,
> #             ^~~~~~~~~
> #  307|                AcpiProtocol,
> #  308|                DsdtTable,
> #  309|->              DsdtTable->Length,
> #  310|                &TableHandle
> #  311|                );

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
Issue: scan-0993.txt
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Laszlo Ersek 5 years ago
parent
commit
e30991740d
1 changed files with 8 additions and 1 deletions
  1. 8 1
      OvmfPkg/AcpiPlatformDxe/Xen.c

+ 8 - 1
OvmfPkg/AcpiPlatformDxe/Xen.c

@@ -295,8 +295,15 @@ InstallXenTables (
   }
 
   //
-  // Install DSDT table.
+  // Install DSDT table. If we reached this point without finding the DSDT,
+  // then we're out of sync with the hypervisor, and cannot continue.
   //
+  if (DsdtTable == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: no DSDT found\n", __FUNCTION__));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
   Status = InstallAcpiTable (
              AcpiProtocol,
              DsdtTable,