From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.221.65, mailfrom: philmd@redhat.com) Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by groups.io with SMTP; Mon, 15 Apr 2019 10:28:34 -0700 Received: by mail-wr1-f65.google.com with SMTP id t17so22967751wrw.13 for ; Mon, 15 Apr 2019 10:28:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3sC+QkjIIbAr/oOuN+9Qn/Ctb7ItagXWchoj+Ej78Tg=; b=O15kDz0S8eZDKAHUaOZKGaHuE2kmHQaOSyt3IZ4fL6IUBdY0jVDK4gaeMF8MF2ayfv 2ofXCRGddGJ2TNJ93+cNKQ8CrabHawvIpJWS4NjZi4pzNXQ2fLHOI/Z9/0brcQIepOIk 8x//Uw08m+n1Xmye85lLYU8qYIPdced3UFFGZc4ZGlEby6lxzFcLpsLSqNj8yXsu80aN TA64G3b7JE7FYaOGSe2CcY39pnimAJZFmHNgRcusiwg+N8qgKotDan8lfTZ9v9ZY1qlT kvvhDbbOgWnuR1YksI495bFugVuV76eqbW1/qshuPCr8yGYNqKOSa4zI8TQhju6d5aBT XfnQ== X-Gm-Message-State: APjAAAU/ar0lfk5zWCSqrQcuGvvZmbYpJ+g13NcSEfAYCuL05GKuX3L+ 28pa77HeInGBX5634WViKfeNjQ== X-Google-Smtp-Source: APXvYqxuj41UUN27IZPzNhcT/3DZ4oVSn/18mYMJWaY3JPXxVCoRV2XY8IrNbz7pEd26xIL2Xyg5Mg== X-Received: by 2002:adf:cc85:: with SMTP id p5mr45368480wrj.101.1555349312896; Mon, 15 Apr 2019 10:28:32 -0700 (PDT) Return-Path: Received: from [10.32.224.40] (red-hat-inc.vlan560.asr1.mad1.gblx.net. [159.63.51.90]) by smtp.gmail.com with ESMTPSA id t81sm36943819wmb.5.2019.04.15.10.28.31 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 15 Apr 2019 10:28:32 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 09/10] OvmfPkg/AcpiPlatformDxe: catch theoretical nullptr deref in Xen code To: devel@edk2.groups.io, lersek@redhat.com Cc: Anthony Perard , Ard Biesheuvel , Jordan Justen , Julien Grall References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-10-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: Date: Mon, 15 Apr 2019 19:28:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190412233128.4756-10-lersek@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 4/13/19 1:31 AM, Laszlo Ersek wrote: > 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 > Cc: Ard Biesheuvel > Cc: Jordan Justen > Cc: Julien Grall > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 > Issue: scan-0993.txt > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/AcpiPlatformDxe/Xen.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/OvmfPkg/AcpiPlatformDxe/Xen.c b/OvmfPkg/AcpiPlatformDxe/Xen.c > index c8f275a8ee84..b21d3db74dc8 100644 > --- a/OvmfPkg/AcpiPlatformDxe/Xen.c > +++ b/OvmfPkg/AcpiPlatformDxe/Xen.c > @@ -295,18 +295,25 @@ InstallXenTables ( > &TableHandle > ); > if (EFI_ERROR (Status)) { > return Status; > } > } > > // > - // 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, > DsdtTable->Length, > &TableHandle > ); > if (EFI_ERROR (Status)) { > return Status; > Reviewed-by: Philippe Mathieu-Daude