From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web10.2026.1616181467390870649 for ; Fri, 19 Mar 2021 12:17:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=U6tz21jv; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1616181466; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UoVnW3K4vDK6mePQu/35vF4IzUfnYLrEhdw0pRj7fuk=; b=U6tz21jvywdQgnh8wTAR3KuYvaoO8EnQWSmwOIDXT64uYbvTgjl+EaVLEmehgB5fM/mzsy 413pxBVVi5yNoKwe+x0Obok84gITyAkP/j6n6WcqsaHH35tcohnhhDC+6wEld0fr21uNM1 saghklMX1j9MyB91ovDFSGd+g1lQ++E= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-470-3KH9m-amMfeyn-Y5CWmDBA-1; Fri, 19 Mar 2021 15:17:42 -0400 X-MC-Unique: 3KH9m-amMfeyn-Y5CWmDBA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 03F151007467; Fri, 19 Mar 2021 19:17:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-31.ams2.redhat.com [10.36.114.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 72D2D19D9D; Fri, 19 Mar 2021 19:17:39 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1] OvmfPkg/X86QemuLoadImageLib: Handle allocation failure for CommandLine To: devel@edk2.groups.io, martin.b.radev@gmail.com, Ard Biesheuvel Cc: Ard Biesheuvel , Jordan Justen , Tom Lendacky References: From: "Laszlo Ersek" Message-ID: Date: Fri, 19 Mar 2021 20:17:38 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/19/21 15:39, Martin Radev wrote: > On Fri, Mar 19, 2021 at 03:27:00PM +0100, Ard Biesheuvel wrote: >> On Thu, 18 Mar 2021 at 22:44, Martin Radev wrote: >>> >>> The CommandLine and InitrdData may be set to NULL if the provided >>> size is too large. Because the zero page is mapped, this would not >>> cause an immediate crash but can lead to memory corruption instead. >>> This patch just adds validation and returns error if either allocation >>> has failed. >>> >>> Ref: https://github.com/martinradev/edk2/commit/6c0ce748b97393240c006e24b73652f30e597a05 >>> >>> Signed-off-by: Martin Radev >> >> This seems reasonable to me. >> >> Acked-by: Ard Biesheuvel >> >>> --- >>> OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c >>> index 931553c0c1..b983c4d7d0 100644 >>> --- a/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c >>> +++ b/OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c >>> @@ -161,6 +161,12 @@ QemuLoadLegacyImage ( >>> LoadedImage->CommandLine = LoadLinuxAllocateCommandLinePages ( >>> EFI_SIZE_TO_PAGES ( >>> LoadedImage->CommandLineSize)); >>> + >>> + if (LoadedImage->CommandLine == NULL) { >>> + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for kernel command line!\n")); >>> + Status = EFI_OUT_OF_RESOURCES; >>> + goto FreeImage; >>> + } >>> QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); >>> QemuFwCfgReadBytes (LoadedImage->CommandLineSize, LoadedImage->CommandLine); >>> } >>> @@ -178,6 +184,11 @@ QemuLoadLegacyImage ( >>> LoadedImage->InitrdData = LoadLinuxAllocateInitrdPages ( >>> LoadedImage->SetupBuf, >>> EFI_SIZE_TO_PAGES (LoadedImage->InitrdSize)); >>> + if (LoadedImage->InitrdData == NULL) { >>> + DEBUG ((DEBUG_ERROR, "Unable to allocate memory for initrd!\n")); >>> + Status = EFI_OUT_OF_RESOURCES; >>> + goto FreeImage; >>> + } >>> DEBUG ((DEBUG_INFO, "Initrd size: 0x%x\n", >>> (UINT32)LoadedImage->InitrdSize)); >>> DEBUG ((DEBUG_INFO, "Reading initrd image ...")); >>> -- >>> 2.17.1 >>> > > Thanks. I have a curiousity question: > Is there a good reason the zero page is kept mapped? Yes, please see commit 90803342b1b6 ("OvmfPkg: QemuVideoDxe: Int10h stub for Windows 7 & 2008 (stdvga, QXL)", 2014-05-20): The Int10h real-mode IVT entry is covered with a Boot Services Code page, making that too unaccessible to the rest of edk2. (Thus UEFI guest OSes different from the Windows 2008 family can reclaim the page. The Windows 2008 family accesses the page at zero regardless of the allocation type.) In fact, there have been calls for allocating page#0 with a UEFI memory type that's longer-lived than EfiBootServicesCode: https://bugs.launchpad.net/qemu/+bug/1593605/comments/7 That allows Hyper-V features to be enabled in multi-CPU Windows 7 guests, at the cost of losing page#0 for all other operating systems. So in upstream OVMF, we keep page#0 allocated as Boot Services Code, letting users boot Windows 7 VMs out of the box; however, if their Windows 7 VMs are multiprocessor ones, they have to disable HyperV features. > IMO, it makes sense to have the first page unmapped to avoid cases > when some piece of code returns NULL as a failure to an allocation, > but then later code uses it by mistake. Unmapping it could be a > security hardening. Another point that I should mention here is "PcdNullPointerDetectionPropertyMask": ## Mask to control the NULL address detection in code for different phases. # If enabled, accessing NULL address in UEFI or SMM code can be caught.

# BIT0 - Enable NULL pointer detection for UEFI.
# BIT1 - Enable NULL pointer detection for SMM.
# BIT2..5 - Reserved for future uses.
# BIT6 - Enable non-stop mode.
# BIT7 - Disable NULL pointer detection just after EndOfDxe.
# This is a workaround for those unsolvable NULL access issues in # OptionROM, boot loader, etc. It can also help to avoid unnecessary # exception caused by legacy memory (0-4095) access after EndOfDxe, # such as Windows 7 boot on Qemu.
# @Prompt Enable NULL address detection. gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask|0x0|UINT8|0x30001050 The code in OVMF that installs the VBE Shim (and allocates page#0) has the following check: if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7)) == BIT0) { DEBUG (( DEBUG_WARN, "%a: page 0 protected, not installing VBE shim\n", __FUNCTION__ )); DEBUG (( DEBUG_WARN, "%a: page 0 protection prevents Windows 7 from booting anyway\n", __FUNCTION__ )); return; } (See commit 90f3922b018e ("OvmfPkg/QemuVideoDxe: Bypass NULL pointer detection during VBE SHIM installing", 2017-10-11).) Which means that you *can* enable page#0 access trapping in the DXE phase with a build-time switch like: --pcd gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask=0x01 for all of the DXE phase, or with --pcd gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask=0x81 until EndOfDxe only. In the former case (value 0x01), the page#0 protection survives into OS boot. That breaks Windows 7 anyway, so there's no attempt to install the VBE shim then. In the latter case (value 0x81), Windows is permitted to boot... but I forget how QemuVideoDxe behaves. Maybe it will trigger the NULL pointer check itself -- I think that was the original bug report / motivation for commit 90f3922b018e. I'm not sure. So, if you don't care about Windows 7 guests, just set PcdNullPointerDetectionPropertyMask=0x01 on your build command line, and then NULL pointer dereferences should crash nicely. Thanks Laszlo