[PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[Roy Franz]

efi_arch_allocate_mmap_buffer() allocates a buffer for the EFI memory map, and
for ARM it allocates a larger buffer than requested. This is done to account
for the increase in the map size that may occur when the allocation is made.
The previous code allocated a larger buffer, but did not adjust the size to
match.  This caused the later call to GetMemoryMap() to fail with a
BUFFER_TOO_SMALL error, since the original, smaller size was used.  This patch
changes the argument to efi_arch_allocate_mmap_buffer() to be a pointer to
UINTN, and the ARM version updates the size on a successful allocation.
The x86 version uses a different allocation method, so only the function
argument type is changed.
Also add decode of the BUFFER_TOO_SMALL error code to PrintErrMesg().

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 xen/arch/arm/efi/efi-boot.h | 6 ++++--
 xen/arch/x86/efi/efi-boot.h | 4 ++--
 xen/common/efi/boot.c       | 5 ++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index d40d8b2..639942d 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -370,14 +370,16 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect
 {
 }
 
-static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
+static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
 {
     void *ptr;
     EFI_STATUS status;
+    UINTN map_size_alloc = *map_size + EFI_PAGE_SIZE;
 
-    status = efi_bs->AllocatePool(EfiLoaderData, map_size + EFI_PAGE_SIZE, &ptr);
+    status = efi_bs->AllocatePool(EfiLoaderData, map_size_alloc, &ptr);
     if ( status != EFI_SUCCESS )
         return NULL;
+    *map_size = map_size_alloc;
     return ptr;
 }
 
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 4348cfe..454ffb6 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -190,10 +190,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 
 }
 
-static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
+static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
 {
     place_string(&mbi.mem_upper, NULL);
-    mbi.mem_upper -= map_size;
+    mbi.mem_upper -= *map_size;
     mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
     if ( mbi.mem_upper < xen_phys_start )
         return NULL;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index f272171..4257341 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
     case EFI_COMPROMISED_DATA:
         mesg = L"Compromised data";
         break;
+    case EFI_BUFFER_TOO_SMALL:
+        mesg = L"Buffer too small";
+        break;
     default:
         PrintErr(L"ErrCode: ");
         DisplayUint(ErrCode, 0);
@@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 
     efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
                          &efi_mdesc_size, &mdesc_ver);
-    efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
+    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
     if ( !efi_memmap )
         blexit(L"Unable to allocate memory for EFI memory map");
 
-- 
1.9.1

Re: [PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[Ian Campbell]

On Tue, 2014-10-21 at 21:18 -0700, Roy Franz wrote:
> efi_arch_allocate_mmap_buffer() allocates a buffer for the EFI memory map, and
> for ARM it allocates a larger buffer than requested. This is done to account
> for the increase in the map size that may occur when the allocation is made.
> The previous code allocated a larger buffer, but did not adjust the size to
> match.  This caused the later call to GetMemoryMap() to fail with a
> BUFFER_TOO_SMALL error, since the original, smaller size was used.  This patch
> changes the argument to efi_arch_allocate_mmap_buffer() to be a pointer to
> UINTN, and the ARM version updates the size on a successful allocation.
> The x86 version uses a different allocation method, so only the function
> argument type is changed.
> Also add decode of the BUFFER_TOO_SMALL error code to PrintErrMesg().
> 
> Signed-off-by: Roy Franz <roy.franz@linaro.org>

For the arm side:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/efi/efi-boot.h | 6 ++++--
>  xen/arch/x86/efi/efi-boot.h | 4 ++--
>  xen/common/efi/boot.c       | 5 ++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index d40d8b2..639942d 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -370,14 +370,16 @@ static void __init efi_arch_cfg_file_late(EFI_FILE_HANDLE dir_handle, char *sect
>  {
>  }
>  
> -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
> +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
>  {
>      void *ptr;
>      EFI_STATUS status;
> +    UINTN map_size_alloc = *map_size + EFI_PAGE_SIZE;
>  
> -    status = efi_bs->AllocatePool(EfiLoaderData, map_size + EFI_PAGE_SIZE, &ptr);
> +    status = efi_bs->AllocatePool(EfiLoaderData, map_size_alloc, &ptr);
>      if ( status != EFI_SUCCESS )
>          return NULL;
> +    *map_size = map_size_alloc;
>      return ptr;
>  }
>  
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 4348cfe..454ffb6 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -190,10 +190,10 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  
>  }
>  
> -static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
> +static void *__init efi_arch_allocate_mmap_buffer(UINTN *map_size)
>  {
>      place_string(&mbi.mem_upper, NULL);
> -    mbi.mem_upper -= map_size;
> +    mbi.mem_upper -= *map_size;
>      mbi.mem_upper &= -__alignof__(EFI_MEMORY_DESCRIPTOR);
>      if ( mbi.mem_upper < xen_phys_start )
>          return NULL;
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index f272171..4257341 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>      case EFI_COMPROMISED_DATA:
>          mesg = L"Compromised data";
>          break;
> +    case EFI_BUFFER_TOO_SMALL:
> +        mesg = L"Buffer too small";
> +        break;
>      default:
>          PrintErr(L"ErrCode: ");
>          DisplayUint(ErrCode, 0);
> @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>  
>      efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>                           &efi_mdesc_size, &mdesc_ver);
> -    efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
> +    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
>      if ( !efi_memmap )
>          blexit(L"Unable to allocate memory for EFI memory map");
>  

Re: [PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[Jan Beulich]

>>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>      case EFI_COMPROMISED_DATA:
>          mesg = L"Compromised data";
>          break;
> +    case EFI_BUFFER_TOO_SMALL:
> +        mesg = L"Buffer too small";
> +        break;

I don't really mind this change, but ...

> @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  
>      efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>                           &efi_mdesc_size, &mdesc_ver);
> -    efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
> +    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
>      if ( !efi_memmap )
>          blexit(L"Unable to allocate memory for EFI memory map");

... it would not have helped here, as you don't even get to see the
error code here.

Jan

Re: [PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[Ian Campbell]

On Wed, 2014-10-22 at 11:07 +0100, Jan Beulich wrote:
> >>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
> >      case EFI_COMPROMISED_DATA:
> >          mesg = L"Compromised data";
> >          break;
> > +    case EFI_BUFFER_TOO_SMALL:
> > +        mesg = L"Buffer too small";
> > +        break;
> 
> I don't really mind this change, but ...
> 
> > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> > *SystemTable)
> >  
> >      efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
> >                           &efi_mdesc_size, &mdesc_ver);
> > -    efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
> > +    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
> >      if ( !efi_memmap )
> >          blexit(L"Unable to allocate memory for EFI memory map");
> 
> ... it would not have helped here, as you don't even get to see the
> error code here.

This patch came about because I was seeing:
        Cannot obtain memory map: ErrCode: 0x8000000000000005
which I suppose is a different path. I don't think the allocation above
would have ever returned EFI_BUFFER_TOO_SMALL, would it?

Ian.

Re: [PATCH for-4.5] EFI: Fix efi_arch_allocate_mmap_buffer() to return new size

[Roy Franz]

On Wed, Oct 22, 2014 at 3:42 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-22 at 11:07 +0100, Jan Beulich wrote:
>> >>> On 22.10.14 at 06:18, <roy.franz@linaro.org> wrote:
>> > --- a/xen/common/efi/boot.c
>> > +++ b/xen/common/efi/boot.c
>> > @@ -271,6 +271,9 @@ static void __init PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode)
>> >      case EFI_COMPROMISED_DATA:
>> >          mesg = L"Compromised data";
>> >          break;
>> > +    case EFI_BUFFER_TOO_SMALL:
>> > +        mesg = L"Buffer too small";
>> > +        break;
>>
>> I don't really mind this change, but ...
>>
>> > @@ -1038,7 +1041,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE
>> > *SystemTable)
>> >
>> >      efi_bs->GetMemoryMap(&efi_memmap_size, NULL, &map_key,
>> >                           &efi_mdesc_size, &mdesc_ver);
>> > -    efi_memmap = efi_arch_allocate_mmap_buffer(efi_memmap_size);
>> > +    efi_memmap = efi_arch_allocate_mmap_buffer(&efi_memmap_size);
>> >      if ( !efi_memmap )
>> >          blexit(L"Unable to allocate memory for EFI memory map");
>>
>> ... it would not have helped here, as you don't even get to see the
>> error code here.
>
> This patch came about because I was seeing:
>         Cannot obtain memory map: ErrCode: 0x8000000000000005
> which I suppose is a different path. I don't think the allocation above
> would have ever returned EFI_BUFFER_TOO_SMALL, would it?
>
> Ian.

The error there is generic since for ARM we do an AllocatePool(), and for x86 we
use the string buffer that is allocated before the _start symbol.  We
don't have an
EFI error code to decode at this point - the EFI error code only exists in the
ARM efi_arch_allocate_mmap_buffer() function.

Roy