[PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[Ard Biesheuvel]

The page allocator code in CoreFindFreePagesI() uses a mask derived from
its UINTN Alignment argument to align the descriptor end address of a
MEMORY_MAP entry to the requested alignment, in order to check whether
the descriptor covers enough sufficiently aligned area to satisfy the
request.

However, on 32-bit architectures, 'Alignment' is a 32-bit type, whereas
DescEnd is a 64-bit type, and so the resulting operation performed on
the end address comes down to masking with 0xfffff000 instead of the
intended 0xffffffff_fffff000. Given the -1 at the end of the expression,
the resulting address is 0xffffffff_fffffffff for any descriptor that
ends on a 4G aligned boundary, and this is certainly not what was
intended.

So cast Alignment to UINT64 to ensure that the mask has the right size.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 160289c1f9ec..5903ce7ab525 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1097,7 +1097,7 @@ CoreFindFreePagesI (
       DescEnd = MaxAddress;
     }
 
-    DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1;
+    DescEnd = ((DescEnd + 1) & (~((UINT64)Alignment - 1))) - 1;
 
     // Skip if DescEnd is less than DescStart after alignment clipping
     if (DescEnd < DescStart) {
-- 
2.35.1

Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[Michael D Kinney]

Hi Ard,

Thank you.  This is a really good find.  How did you find it?

I am guessing this case may not have been noticed on IA32/X64 because
those archs tend to have FLASH device mapped just below 4GB and as a
result there is no system memory in the range just below 4GB. This 
means the case where a page allocation with an end descriptor at
4GB is not observed on IA32/X64.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, December 7, 2022 10:01 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>; Ard
> Biesheuvel <ardb@kernel.org>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask
> 
> The page allocator code in CoreFindFreePagesI() uses a mask derived from
> its UINTN Alignment argument to align the descriptor end address of a
> MEMORY_MAP entry to the requested alignment, in order to check whether
> the descriptor covers enough sufficiently aligned area to satisfy the
> request.
> 
> However, on 32-bit architectures, 'Alignment' is a 32-bit type, whereas
> DescEnd is a 64-bit type, and so the resulting operation performed on
> the end address comes down to masking with 0xfffff000 instead of the
> intended 0xffffffff_fffff000. Given the -1 at the end of the expression,
> the resulting address is 0xffffffff_fffffffff for any descriptor that
> ends on a 4G aligned boundary, and this is certainly not what was
> intended.
> 
> So cast Alignment to UINT64 to ensure that the mask has the right size.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 160289c1f9ec..5903ce7ab525 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1097,7 +1097,7 @@ CoreFindFreePagesI (
>        DescEnd = MaxAddress;
> 
>      }
> 
> 
> 
> -    DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1;
> 
> +    DescEnd = ((DescEnd + 1) & (~((UINT64)Alignment - 1))) - 1;
> 
> 
> 
>      // Skip if DescEnd is less than DescStart after alignment clipping
> 
>      if (DescEnd < DescStart) {
> 
> --
> 2.35.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#97101): https://edk2.groups.io/g/devel/message/97101
> Mute This Topic: https://groups.io/mt/95520976/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[Ard Biesheuvel]

On Wed, 7 Dec 2022 at 19:54, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Thank you.  This is a really good find.  How did you find it?
>

The 32-bit version of ArmVirtQemu got inadvertently broken by a change
that seemed unrelated, but resulted in the primary memory resource
descriptor hob to end exactly at 4 GB if the memory exceeds that
limit, with very confusing results (ASSERT()s on invalid magic numbers
in the MEMORY_MAP linked list)

> I am guessing this case may not have been noticed on IA32/X64 because
> those archs tend to have FLASH device mapped just below 4GB and as a
> result there is no system memory in the range just below 4GB. This
> means the case where a page allocation with an end descriptor at
> 4GB is not observed on IA32/X64.
>

Indeed. And it was not observed on ARM either, because we used to
place the PEI permanent memory (I think?) at the top of addressable
RAM, resulting in the region to be occupied by the time DXE core runs.
On 64-bit ARM, i changed that policy recently to use 128 MiB of system
memory that is described by initial page tables stored in flash, but
this move resulted in this weird behavior on 32-bit (which doesn't
have the initial page tables)

Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Wednesday, December 7, 2022 10:01 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>; Ard
> Biesheuvel <ardb@kernel.org>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask
> 
> The page allocator code in CoreFindFreePagesI() uses a mask derived from
> its UINTN Alignment argument to align the descriptor end address of a
> MEMORY_MAP entry to the requested alignment, in order to check whether
> the descriptor covers enough sufficiently aligned area to satisfy the
> request.
> 
> However, on 32-bit architectures, 'Alignment' is a 32-bit type, whereas
> DescEnd is a 64-bit type, and so the resulting operation performed on
> the end address comes down to masking with 0xfffff000 instead of the
> intended 0xffffffff_fffff000. Given the -1 at the end of the expression,
> the resulting address is 0xffffffff_fffffffff for any descriptor that
> ends on a 4G aligned boundary, and this is certainly not what was
> intended.
> 
> So cast Alignment to UINT64 to ensure that the mask has the right size.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 160289c1f9ec..5903ce7ab525 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1097,7 +1097,7 @@ CoreFindFreePagesI (
>        DescEnd = MaxAddress;
> 
>      }
> 
> 
> 
> -    DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1;
> 
> +    DescEnd = ((DescEnd + 1) & (~((UINT64)Alignment - 1))) - 1;
> 
> 
> 
>      // Skip if DescEnd is less than DescStart after alignment clipping
> 
>      if (DescEnd < DescStart) {
> 
> --
> 2.35.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#97101): https://edk2.groups.io/g/devel/message/97101
> Mute This Topic: https://groups.io/mt/95520976/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask

[Ard Biesheuvel]

On Thu, 8 Dec 2022 at 00:13, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>

Thanks

Merged as #3738

>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, December 7, 2022 10:01 AM
> > To: devel@edk2.groups.io
> > Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>; Ard
> > Biesheuvel <ardb@kernel.org>
> > Subject: [edk2-devel] [PATCH] MdeModulePkg/DxeCore: Use correct type for alignment mask
> >
> > The page allocator code in CoreFindFreePagesI() uses a mask derived from
> > its UINTN Alignment argument to align the descriptor end address of a
> > MEMORY_MAP entry to the requested alignment, in order to check whether
> > the descriptor covers enough sufficiently aligned area to satisfy the
> > request.
> >
> > However, on 32-bit architectures, 'Alignment' is a 32-bit type, whereas
> > DescEnd is a 64-bit type, and so the resulting operation performed on
> > the end address comes down to masking with 0xfffff000 instead of the
> > intended 0xffffffff_fffff000. Given the -1 at the end of the expression,
> > the resulting address is 0xffffffff_fffffffff for any descriptor that
> > ends on a 4G aligned boundary, and this is certainly not what was
> > intended.
> >
> > So cast Alignment to UINT64 to ensure that the mask has the right size.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  MdeModulePkg/Core/Dxe/Mem/Page.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > index 160289c1f9ec..5903ce7ab525 100644
> > --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> > +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> > @@ -1097,7 +1097,7 @@ CoreFindFreePagesI (
> >        DescEnd = MaxAddress;
> >
> >      }
> >
> >
> >
> > -    DescEnd = ((DescEnd + 1) & (~(Alignment - 1))) - 1;
> >
> > +    DescEnd = ((DescEnd + 1) & (~((UINT64)Alignment - 1))) - 1;
> >
> >
> >
> >      // Skip if DescEnd is less than DescStart after alignment clipping
> >
> >      if (DescEnd < DescStart) {
> >
> > --
> > 2.35.1
> >
> >
> >
> > -=-=-=-=-=-=
> > Groups.io Links: You receive all messages sent to this group.
> > View/Reply Online (#97101): https://edk2.groups.io/g/devel/message/97101
> > Mute This Topic: https://groups.io/mt/95520976/1643496
> > Group Owner: devel+owner@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> > -=-=-=-=-=-=
> >
>