[PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

[PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

[Ard Biesheuvel]

When attempting to perform page allocations using AllocateAddress, we
fail to check whether the entire region is free before splitting the
region. This may lead to memory being leaked further into the routine,
when it turns out that one of the memory map entries intersected by the
region is already occupied. In this case, prior conversions are not rolled
back.

For instance, starting from this situation

0x000040000000-0x00004007ffff [ConventionalMemory ]
0x000040080000-0x00004009ffff [Boot Data          ]
0x0000400a0000-0x000047ffffff [ConventionalMemory ]

a failed EfiLoaderData allocation @ 0x40000000 that covers the BootData
region will fail, but leave the first part of the allocation converted,
so we end up with

0x000040000000-0x00004007ffff [Loader Data        ]
0x000040080000-0x00004009ffff [Boot Data          ]
0x0000400a0000-0x000047ffffff [ConventionalMemory ]

even though the AllocatePages() call returned an error.

So let's check beforehand that AllocateAddress allocations are covered
by a single memory map entry, so that it either succeeds or fails
completely, rather than leaking allocations.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/Page.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 260a30a214c7..92306b2f1b45 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -755,6 +755,17 @@ CoreConvertPagesEx (
     }
 
     //
+    // If we are converting the type of the range from EfiConventionalMemory to
+    // another type, we have to ensure that the entire range is covered by a
+    // single entry.
+    //
+    if (ChangingType && (NewType != EfiConventionalMemory)) {
+      if (Entry->Start > End || Entry->End < End) {
+        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: range %lx - %lx covers multiple entries\n", Start, End));
+        return EFI_NOT_FOUND;
+      }
+    }
+    //
     // Convert range to the end, or to the end of the descriptor
     // if that's all we've got
     //
-- 
2.7.4

Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

[Ard Biesheuvel]

On 16 March 2017 at 11:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When attempting to perform page allocations using AllocateAddress, we
> fail to check whether the entire region is free before splitting the
> region. This may lead to memory being leaked further into the routine,
> when it turns out that one of the memory map entries intersected by the
> region is already occupied. In this case, prior conversions are not rolled
> back.
>
> For instance, starting from this situation
>
> 0x000040000000-0x00004007ffff [ConventionalMemory ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> a failed EfiLoaderData allocation @ 0x40000000 that covers the BootData
> region will fail, but leave the first part of the allocation converted,
> so we end up with
>
> 0x000040000000-0x00004007ffff [Loader Data        ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> even though the AllocatePages() call returned an error.
>
> So let's check beforehand that AllocateAddress allocations are covered
> by a single memory map entry, so that it either succeeds or fails
> completely, rather than leaking allocations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 260a30a214c7..92306b2f1b45 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -755,6 +755,17 @@ CoreConvertPagesEx (
>      }
>
>      //
> +    // If we are converting the type of the range from EfiConventionalMemory to
> +    // another type, we have to ensure that the entire range is covered by a
> +    // single entry.
> +    //
> +    if (ChangingType && (NewType != EfiConventionalMemory)) {
> +      if (Entry->Start > End || Entry->End < End) {

I guess this expression is slightly bogus: we know entry
[Entry->Start, Entry->End) covers Start, and so the only way the entry
could fail to cover [Start, End) is when Entry->End < End. The first
half of the expression can be dropped since it can never be true


> +        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: range %lx - %lx covers multiple entries\n", Start, End));
> +        return EFI_NOT_FOUND;
> +      }
> +    }
> +    //
>      // Convert range to the end, or to the end of the descriptor
>      // if that's all we've got
>      //
> --
> 2.7.4
>

Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

[Zeng, Star]

Ard,

In fact the first half should be like below if it needs to be there, right?
Entry->Start > End   =>   Entry->Start > (End + 1)

Anyway, I agree the comments "The first half of the expression can be dropped" you supplemented.
With the first half of the expression dropped, Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Thursday, March 16, 2017 9:19 PM
To: edk2-devel@lists.01.org
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Cohen, Eugene <eugene@hp.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

On 16 March 2017 at 11:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> When attempting to perform page allocations using AllocateAddress, we 
> fail to check whether the entire region is free before splitting the 
> region. This may lead to memory being leaked further into the routine, 
> when it turns out that one of the memory map entries intersected by 
> the region is already occupied. In this case, prior conversions are 
> not rolled back.
>
> For instance, starting from this situation
>
> 0x000040000000-0x00004007ffff [ConventionalMemory ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> a failed EfiLoaderData allocation @ 0x40000000 that covers the 
> BootData region will fail, but leave the first part of the allocation 
> converted, so we end up with
>
> 0x000040000000-0x00004007ffff [Loader Data        ]
> 0x000040080000-0x00004009ffff [Boot Data          ]
> 0x0000400a0000-0x000047ffffff [ConventionalMemory ]
>
> even though the AllocatePages() call returned an error.
>
> So let's check beforehand that AllocateAddress allocations are covered 
> by a single memory map entry, so that it either succeeds or fails 
> completely, rather than leaking allocations.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c 
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 260a30a214c7..92306b2f1b45 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -755,6 +755,17 @@ CoreConvertPagesEx (
>      }
>
>      //
> +    // If we are converting the type of the range from EfiConventionalMemory to
> +    // another type, we have to ensure that the entire range is covered by a
> +    // single entry.
> +    //
> +    if (ChangingType && (NewType != EfiConventionalMemory)) {
> +      if (Entry->Start > End || Entry->End < End) {

I guess this expression is slightly bogus: we know entry [Entry->Start, Entry->End) covers Start, and so the only way the entry could fail to cover [Start, End) is when Entry->End < End. The first half of the expression can be dropped since it can never be true


> +        DEBUG ((DEBUG_ERROR | DEBUG_PAGE, "ConvertPages: range %lx - %lx covers multiple entries\n", Start, End));
> +        return EFI_NOT_FOUND;
> +      }
> +    }
> +    //
>      // Convert range to the end, or to the end of the descriptor
>      // if that's all we've got
>      //
> --
> 2.7.4
>

Re: [PATCH] MdeModulePkg/DxeCore: deal with allocations spanning several memmap entries

[Ard Biesheuvel]

On 17 March 2017 at 03:45, Zeng, Star <star.zeng@intel.com> wrote:
> Ard,
>
> In fact the first half should be like below if it needs to be there, right?
> Entry->Start > End   =>   Entry->Start > (End + 1)
>

Ehm, I guess?

> Anyway, I agree the comments "The first half of the expression can be dropped" you supplemented.
> With the first half of the expression dropped, Reviewed-by: Star Zeng <star.zeng@intel.com>
>

Pushed, thanks.