From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f177.google.com (mail-lj1-f177.google.com [209.85.208.177]) by mx.groups.io with SMTP id smtpd.web08.7277.1616164795317811776 for ; Fri, 19 Mar 2021 07:39:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=rorP5Ymk; spf=pass (domain: gmail.com, ip: 209.85.208.177, mailfrom: martin.b.radev@gmail.com) Received: by mail-lj1-f177.google.com with SMTP id f16so12329615ljm.1 for ; Fri, 19 Mar 2021 07:39:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=U9yS/EqHTkc38Q9DWjqtC98aR0AmnptHrcM1AsV6AkU=; b=rorP5YmktlxFpnzoj5kiHtZCiofrnEUtEg68qaGQMICvp9WcSP4PmdUyew6BsATd+r HEzk4ySFS8VHWIm3CxNT7IZxLOk7akkDzgQxA2ShnzH4+iNNgLI1Ymm70FkSh4a3euE6 PBM44he6gg3MmoVg6NUEjLabO/2qS+9jwYnETKwIKXf26jCHXZCP0P496rMZKNWF++iL SJs+lXBDBKpTgRqKPXy51meh2Gb7drwqEHSKOcc9E2d5Ns6OMQMU6XFKsE9f16gE9u2X hf16lFJ4EwrFmKEO5ngxY9tWbUQQgAts3+YihwU143WARwaWXIgNe1Csmt4StkgYTA4k BGsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=U9yS/EqHTkc38Q9DWjqtC98aR0AmnptHrcM1AsV6AkU=; b=sbgYcNVcKQ34JE7c8pIz4xB836Fg5Wvsl2J2Fs31r5odogB2cM9v18EQdXOwvVCLOX nI+JoFhoPSg0u3REYFxt2MIJPnOyyfppMj3/lxf22e+RIwQlctsrCdvaITnQDWh95YA3 TMPjHQmOsimTO0EcrVdS/n/pNJ8ZWtPBJI6mHXgDyXUPSoz5O2lxE73XQTFkUULdg0/P vR1do7NvQFG1zj2mJdMNtre8Qqqh5EtflRu+z4yWMZjiTKq8DUCliUtOW6Bas/AHZxDO zhzBlgzS+FWiQhFvFOdscc/gFW4kiZYXkd0Tecmlf9I9bgBK1HYI78x4KAySryqOp2qz e3tw== X-Gm-Message-State: AOAM530m4zwdnNVPXr3C3N2G0ucNbamqLjMoEGPCJjnHfMgkRi/J84p2 E4x9jkr2MQ/+ldcPKDW4Vio= X-Google-Smtp-Source: ABdhPJzsRRjLGfjIOIgBgWjjHwxZgRLHNJ3Wd0D9A981g2gV4QGVNO1rY0qhqCg1OU76M+POveLd1g== X-Received: by 2002:a2e:b16b:: with SMTP id a11mr1180351ljm.132.1616164793547; Fri, 19 Mar 2021 07:39:53 -0700 (PDT) Return-Path: Received: from martin-ThinkPad-T440p (88-115-234-9.elisa-laajakaista.fi. [88.115.234.9]) by smtp.gmail.com with ESMTPSA id c6sm642589lfb.204.2021.03.19.07.39.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Mar 2021 07:39:53 -0700 (PDT) Date: Fri, 19 Mar 2021 15:39:51 +0100 From: Martin Radev To: Ard Biesheuvel Cc: devel@edk2.groups.io, Laszlo Ersek , Ard Biesheuvel , Jordan Justen , Tom Lendacky Subject: Re: [PATCH v1] OvmfPkg/X86QemuLoadImageLib: Handle allocation failure for CommandLine Message-ID: References: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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.