[edk2-devel] [PATCH v1] OvmfPkg/X86QemuLoadImageLib: Handle allocation failure for CommandLine

Fri Mar 19 19:17:38 UTC 2021

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 at 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 at gmail.com>
>>
>> This seems reasonable to me.
>>
>> Acked-by: Ard Biesheuvel <ardb at 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. 
 # 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

-=-=-=-=-=-=-=-=-=-=-=-
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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-