On 03/21/13 00:38, Kinney, Michael D wrote:
> Please see the attached patch that guarantees that free memory in the 4K
> page starting at address 0 is always cleared to 0. The algorithm is to
> clear page zero if it is registered with the DXE Core with type
> EfiConventionalMemory, and to also clear page zero if it is freed using
> the UEFI Boot Service FreePages(). The unit test for this patch exposed
> a bug in the DXE Core that generated an ASSERT() when the page at
> address zero is freed and DEBUG_CLEAR_MEMORY() macros are enabled. If
> DEBUG_CLEAR_MEMORY() is enabled and the page at address 0 is freed, then
> DEBUG_CLEAR_MEMORY() is invoked skipping over the first 4K page.
>
>
>
> This patch improves OS compatibility for OSes that may evaluate page 0
> for legacy data structures. Before this patch, free memory may contain
> random values which induces random boot failures for some OSes. This
> patch may also help find NULL pointer bugs sooner because all of the
> fields in a data structure dereferenced through NULL will also be NULL now.
I'm including the patch here for review:
> Index: Dxe/Mem/Page.c
> ===================================================================
> --- Dxe/Mem/Page.c (revision 14133)
> +++ Dxe/Mem/Page.c (working copy)
> @@ -1,7 +1,7 @@
> /** @file
> UEFI Memory page management functions.
>
> -Copyright (c) 2007 - 2012, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2013, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -177,8 +177,21 @@
> ASSERT_LOCKED (&gMemoryLock);
>
> DEBUG ((DEBUG_PAGE, "AddRange: %lx-%lx to %d\n", Start, End, Type));
> -
> +
> //
> + // If memory of type EfiConventionalMemory is being added that includes
> the page
> + // starting at address 0, then zero the page starting at address 0. This
> has
> + // two benifits. It helps find NULL pointer bugs and it also maximizes
> + // compatibility with operating systems that may evaluate memory in this
> page
> + // for legacy data structures. If memory of any other type is added
> starting
> + // at address 0, then do not zero the page at address 0 because the page
> is being
> + // used for other purposes.
> + //
> + if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE -
> 1)) {
> + SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> + }
> +
> + //
> // Memory map being altered so updated key
> //
> mMemoryMapKey += 1;
This hunk (for function CoreAddRange()) seems OK. "End" is inclusive,
hence we're checking whether the range being added covers the zero page
entirely.
> @@ -834,7 +847,18 @@
> //
> CoreAddRange (NewType, Start, RangeEnd, Attribute);
> if (NewType == EfiConventionalMemory) {
> - DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd - Start +
> 1));
> + //
> + // Avoid calling DEBUG_CLEAR_MEMORY() for an address of 0 because this
> + // macro will ASSERT() if address is 0. Instead, CoreAddRange()
> guarantees
> + // that the page starting at address 0 is always filled with zeros.
> + //
> + if (Start == 0) {
> + if (RangeEnd > EFI_PAGE_SIZE) {
> + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) EFI_PAGE_SIZE, (UINTN)
> (RangeEnd - EFI_PAGE_SIZE + 1));
> + }
> + } else {
> + DEBUG_CLEAR_MEMORY ((VOID *)(UINTN) Start, (UINTN) (RangeEnd - Start
> + 1));
> + }
> }
>
> //
This hunk (for CoreConvertPages()) immediately follows a call to
CoreAddRange(). The interface contract of CoreAddRange() requires
[Start, RangeEnd] (inclusive) to cover at least one page, and RangeEnd
to be the last (zero-based) offset within a page.
The (Start > 0) branch is clearly OK.
The (RangeEnd > EFI_PAGE_SIZE) controlling expression is off by one "in
theory". "RangeEnd" is inclusive, hence we should put the "touches
another page" condition as (RangeEnd >= EFI_PAGE_SIZE) instead.
The difference would only matter for (RangeEnd == EFI_PAGE_SIZE), in
which case the additional range length (RangeEnd - EFI_PAGE_SIZE + 1)
would evaluate to 1 precisely. However this case is impossible due to
the CoreAddRange() interface contract:
- end of page #0: EFI_PAGE_SIZE - 1 < EFI_PAGE_SIZE
- end of page #1: EFI_PAGE_SIZE + EFI_PAGE_SIZE - 1 > EFI_PAGE_SIZE
Reviewed-by: Laszlo Ersek <[email protected]>
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
