OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
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 <martin.b.radev@gmail.com>
---
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73028): https://edk2.groups.io/g/devel/message/73028
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, 18 Mar 2021 at 22:44, Martin Radev <martin.b.radev@gmail.com> 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 <martin.b.radev@gmail.com>
This seems reasonable to me.
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73042): https://edk2.groups.io/g/devel/message/73042
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Fri, Mar 19, 2021 at 03:27:00PM +0100, Ard Biesheuvel wrote:
> On Thu, 18 Mar 2021 at 22:44, Martin Radev <martin.b.radev@gmail.com> 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 <martin.b.radev@gmail.com>
>
> This seems reasonable to me.
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> > ---
> > 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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73050): https://edk2.groups.io/g/devel/message/73050
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Fri, 19 Mar 2021 at 15:39, Martin Radev <martin.b.radev@gmail.com> wrote:
>
> On Fri, Mar 19, 2021 at 03:27:00PM +0100, Ard Biesheuvel wrote:
> > On Thu, 18 Mar 2021 at 22:44, Martin Radev <martin.b.radev@gmail.com> 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 <martin.b.radev@gmail.com>
> >
> > This seems reasonable to me.
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > > ---
> > > 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73044): https://edk2.groups.io/g/devel/message/73044
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <martin.b.radev@gmail.com> 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 <martin.b.radev@gmail.com>
>>
>> This seems reasonable to me.
>>
>> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>>
>>> ---
>>> 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.<BR><BR>
# BIT0 - Enable NULL pointer detection for UEFI.<BR>
# BIT1 - Enable NULL pointer detection for SMM.<BR>
# BIT2..5 - Reserved for future uses.<BR>
# BIT6 - Enable non-stop mode.<BR>
# BIT7 - Disable NULL pointer detection just after EndOfDxe. <BR>
# 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.<BR>
# @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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73053): https://edk2.groups.io/g/devel/message/73053
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 3/18/21 4:44 PM, 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 <martin.b.radev@gmail.com>
Looks good to me. The two other LoadLinuxAllocate...() calls check for
NULL, so it's reasonable that these should as well.
Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> 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 ..."));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73046): https://edk2.groups.io/g/devel/message/73046
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 03/18/21 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
>
(1) I've dropped this reference from the commit message, as the
referred-to object appears ephemeral.
> Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
> ---
> 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));
> +
(2) I've dropped this empty line too.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Merged as commit ca3188827140, via
<https://github.com/tianocore/edk2/pull/1507>, with Ard's and Tom's ACKs.
Thanks for the fix!
Laszlo
> + 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 ..."));
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73052): https://edk2.groups.io/g/devel/message/73052
Mute This Topic: https://groups.io/mt/81445621/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.