From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web11.7519.1616165742442644443 for ; Fri, 19 Mar 2021 07:55:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=F75ts56K; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id 809EA64ECF for ; Fri, 19 Mar 2021 14:55:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1616165741; bh=dUo4dShxPglx1QY/u57ZMorqaiuzv8XEH1E5YwYlaXI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=F75ts56KuBKspQQ3XY2NsvBkrmAIeDpOGRUzRDB7JndavYgD3sm2Sa8b9P0+47+7f ZjWmQkxBVDWjo7Hrz/oKhfOKuzXYkGveyqyt7HTG1QQ2npWSE6G99UNr90y9fNv051 5p10y8k2LQ6KxYIfUT8bT4E4ZbWG66sXOCXu0dBo/oRak9gEEIYY+V510IXb3bo7L/ 9KfjDwzwDb7ny0ooiV03SJ8blbiYTJh+B23jFkhffx88WuVFIBTqMv7LPVuz/DjP+m H9YgFNlOyQNJoxo2azMIc9sWykewiA+OELn4JiGZQ2/+czHYalM3rzXfFynPtYKRs1 WKslRvJRibw8w== Received: by mail-oi1-f173.google.com with SMTP id d12so5036555oiw.12 for ; Fri, 19 Mar 2021 07:55:41 -0700 (PDT) X-Gm-Message-State: AOAM533T4TlWsw/cAD3PInWsY9iQYVue+v+UqoNxRkkVPOAN1sKT2Y84 vY7jpRdrY0Y0nY/+Quc4t+BMLgs7KtIRYXsj5+8= X-Google-Smtp-Source: ABdhPJzNn1XN4d2+zEzrqYXQxOUOdzNVookhfsDhXLq8cCXMjI9SvY497H9LReYlBDjH1CTvX0BXv6cqNzgZ4gfbIO4= X-Received: by 2002:aca:538c:: with SMTP id h134mr1274040oib.174.1616165740833; Fri, 19 Mar 2021 07:55:40 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Fri, 19 Mar 2021 15:55:29 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v1] OvmfPkg/X86QemuLoadImageLib: Handle allocation failure for CommandLine To: Martin Radev Cc: devel@edk2.groups.io, Laszlo Ersek , Ard Biesheuvel , Jordan Justen , Tom Lendacky Content-Type: text/plain; charset="UTF-8" On Fri, 19 Mar 2021 at 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? > 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. I agree. Amusingly, we had to add a special case to the X86 option rom emulator [0] because many proprietary EFI drivers written for x86 contain NULL pointer dereferences that simply go unnoticed when executed natively, but under emulation on a AArch64 host, they trigger page faults as ARM systems usually don't have anything accessible at physical address 0x0. [0] https://github.com/ardbiesheuvel/X86EmulatorPkg