[PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Ard Biesheuvel]

This fixes two minor issues in the implementation of get_memory_map():
- Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
  which is usually true, but not mandated by the spec. (This was added
  intentionally to allow future additions to the definition of
  efi_memory_desc_t). The way the loop is implemented currently, the
  added slack space may be insufficient if desc_size is larger, which in
  some corner cases could result in the loop never terminating.
- It allocates 32 efi_memory_desc_t entries first (again, using the size
  of the struct instead of desc_size), and frees and reallocates if it
  turns out to be insufficient. Few implementations of UEFI have such small
  memory maps, which results in a unnecessary allocate/free pair on each
  invocation.

Fix this by calling the get_memory_map() boot service first with a '0'
input value for map size to retrieve the map size and desc size from the
firmware and only then perform the allocation, using desc_size rather
than sizeof(efi_memory_desc_t).

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index e766df60fbfb..caf91eab0bfc 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -75,25 +75,29 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 	unsigned long key;
 	u32 desc_version;
 
-	*map_size = sizeof(*m) * 32;
-again:
+	*map_size = 0;
+	*desc_size = 0;
+	key = 0;
+	status = efi_call_early(get_memory_map, map_size, NULL,
+				&key, desc_size, &desc_version);
+	if (status != EFI_BUFFER_TOO_SMALL)
+		return EFI_LOAD_ERROR;
+
 	/*
 	 * Add an additional efi_memory_desc_t because we're doing an
 	 * allocation which may be in a new descriptor region.
 	 */
-	*map_size += sizeof(*m);
+	*map_size += *desc_size;
 	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
 				*map_size, (void **)&m);
 	if (status != EFI_SUCCESS)
 		goto fail;
 
-	*desc_size = 0;
-	key = 0;
 	status = efi_call_early(get_memory_map, map_size, m,
 				&key, desc_size, &desc_version);
 	if (status == EFI_BUFFER_TOO_SMALL) {
 		efi_call_early(free_pool, m);
-		goto again;
+		return EFI_LOAD_ERROR;
 	}
 
 	if (status != EFI_SUCCESS)
-- 
1.8.3.2

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Mark Rutland]

Hi Ard,

On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
> This fixes two minor issues in the implementation of get_memory_map():
> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
>   which is usually true, but not mandated by the spec. (This was added
>   intentionally to allow future additions to the definition of
>   efi_memory_desc_t). The way the loop is implemented currently, the
>   added slack space may be insufficient if desc_size is larger, which in
>   some corner cases could result in the loop never terminating.
> - It allocates 32 efi_memory_desc_t entries first (again, using the size
>   of the struct instead of desc_size), and frees and reallocates if it
>   turns out to be insufficient. Few implementations of UEFI have such small
>   memory maps, which results in a unnecessary allocate/free pair on each
>   invocation.
> 
> Fix this by calling the get_memory_map() boot service first with a '0'
> input value for map size to retrieve the map size and desc size from the
> firmware and only then perform the allocation, using desc_size rather
> than sizeof(efi_memory_desc_t).

Is the desc_size guaranteed to be set up correctly if the size is too
small?

As far as I can see, for that case the spec only mandates that
MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.

It's not clear to me whether DescriptorSize or DescriptorVersion are
initialised in cases other than success.

Thanks,
Mark.

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index e766df60fbfb..caf91eab0bfc 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -75,25 +75,29 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
>  	unsigned long key;
>  	u32 desc_version;
>  
> -	*map_size = sizeof(*m) * 32;
> -again:
> +	*map_size = 0;
> +	*desc_size = 0;
> +	key = 0;
> +	status = efi_call_early(get_memory_map, map_size, NULL,
> +				&key, desc_size, &desc_version);
> +	if (status != EFI_BUFFER_TOO_SMALL)
> +		return EFI_LOAD_ERROR;
> +
>  	/*
>  	 * Add an additional efi_memory_desc_t because we're doing an
>  	 * allocation which may be in a new descriptor region.
>  	 */
> -	*map_size += sizeof(*m);
> +	*map_size += *desc_size;
>  	status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>  				*map_size, (void **)&m);
>  	if (status != EFI_SUCCESS)
>  		goto fail;
>  
> -	*desc_size = 0;
> -	key = 0;
>  	status = efi_call_early(get_memory_map, map_size, m,
>  				&key, desc_size, &desc_version);
>  	if (status == EFI_BUFFER_TOO_SMALL) {
>  		efi_call_early(free_pool, m);
> -		goto again;
> +		return EFI_LOAD_ERROR;
>  	}
>  
>  	if (status != EFI_SUCCESS)
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Ard Biesheuvel]

On 8 January 2015 at 19:04, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> Hi Ard,
>
> On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
>> This fixes two minor issues in the implementation of get_memory_map():
>> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
>>   which is usually true, but not mandated by the spec. (This was added
>>   intentionally to allow future additions to the definition of
>>   efi_memory_desc_t). The way the loop is implemented currently, the
>>   added slack space may be insufficient if desc_size is larger, which in
>>   some corner cases could result in the loop never terminating.
>> - It allocates 32 efi_memory_desc_t entries first (again, using the size
>>   of the struct instead of desc_size), and frees and reallocates if it
>>   turns out to be insufficient. Few implementations of UEFI have such small
>>   memory maps, which results in a unnecessary allocate/free pair on each
>>   invocation.
>>
>> Fix this by calling the get_memory_map() boot service first with a '0'
>> input value for map size to retrieve the map size and desc size from the
>> firmware and only then perform the allocation, using desc_size rather
>> than sizeof(efi_memory_desc_t).
>
> Is the desc_size guaranteed to be set up correctly if the size is too
> small?
>
> As far as I can see, for that case the spec only mandates that
> MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.
>
> It's not clear to me whether DescriptorSize or DescriptorVersion are
> initialised in cases other than success.
>

The way I read it, descriptor size and descriptor version are always
returned, e.g., as opposed to MapKey, which is only returned on
success, and the spec mentions that specifically.

We could ask for clarification just to be sure.

-- 
Ard.

>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index e766df60fbfb..caf91eab0bfc 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -75,25 +75,29 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
>>       unsigned long key;
>>       u32 desc_version;
>>
>> -     *map_size = sizeof(*m) * 32;
>> -again:
>> +     *map_size = 0;
>> +     *desc_size = 0;
>> +     key = 0;
>> +     status = efi_call_early(get_memory_map, map_size, NULL,
>> +                             &key, desc_size, &desc_version);
>> +     if (status != EFI_BUFFER_TOO_SMALL)
>> +             return EFI_LOAD_ERROR;
>> +
>>       /*
>>        * Add an additional efi_memory_desc_t because we're doing an
>>        * allocation which may be in a new descriptor region.
>>        */
>> -     *map_size += sizeof(*m);
>> +     *map_size += *desc_size;
>>       status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>                               *map_size, (void **)&m);
>>       if (status != EFI_SUCCESS)
>>               goto fail;
>>
>> -     *desc_size = 0;
>> -     key = 0;
>>       status = efi_call_early(get_memory_map, map_size, m,
>>                               &key, desc_size, &desc_version);
>>       if (status == EFI_BUFFER_TOO_SMALL) {
>>               efi_call_early(free_pool, m);
>> -             goto again;
>> +             return EFI_LOAD_ERROR;
>>       }
>>
>>       if (status != EFI_SUCCESS)
>> --
>> 1.8.3.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Mark Rutland]

On Thu, Jan 08, 2015 at 07:09:22PM +0000, Ard Biesheuvel wrote:
> On 8 January 2015 at 19:04, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > Hi Ard,
> >
> > On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
> >> This fixes two minor issues in the implementation of get_memory_map():
> >> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
> >>   which is usually true, but not mandated by the spec. (This was added
> >>   intentionally to allow future additions to the definition of
> >>   efi_memory_desc_t). The way the loop is implemented currently, the
> >>   added slack space may be insufficient if desc_size is larger, which in
> >>   some corner cases could result in the loop never terminating.
> >> - It allocates 32 efi_memory_desc_t entries first (again, using the size
> >>   of the struct instead of desc_size), and frees and reallocates if it
> >>   turns out to be insufficient. Few implementations of UEFI have such small
> >>   memory maps, which results in a unnecessary allocate/free pair on each
> >>   invocation.
> >>
> >> Fix this by calling the get_memory_map() boot service first with a '0'
> >> input value for map size to retrieve the map size and desc size from the
> >> firmware and only then perform the allocation, using desc_size rather
> >> than sizeof(efi_memory_desc_t).
> >
> > Is the desc_size guaranteed to be set up correctly if the size is too
> > small?
> >
> > As far as I can see, for that case the spec only mandates that
> > MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.
> >
> > It's not clear to me whether DescriptorSize or DescriptorVersion are
> > initialised in cases other than success.
> >
> 
> The way I read it, descriptor size and descriptor version are always
> returned, e.g., as opposed to MapKey, which is only returned on
> success, and the spec mentions that specifically.

I agree that that would be the sensible reading of the spec. I just fear
that there's room for an implementor to read it slightly differently,
and cause pain for us.

> We could ask for clarification just to be sure.

I think we should.

Given it could take a while for any conclusion to be reached and
published, I'm happy to go with the patch below in the meantime, so long
as the issue gets raised.

Cheers,
Mark.

> 
> -- 
> Ard.
> 
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>  drivers/firmware/efi/libstub/efi-stub-helper.c | 16 ++++++++++------
> >>  1 file changed, 10 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >> index e766df60fbfb..caf91eab0bfc 100644
> >> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> >> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >> @@ -75,25 +75,29 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
> >>       unsigned long key;
> >>       u32 desc_version;
> >>
> >> -     *map_size = sizeof(*m) * 32;
> >> -again:
> >> +     *map_size = 0;
> >> +     *desc_size = 0;
> >> +     key = 0;
> >> +     status = efi_call_early(get_memory_map, map_size, NULL,
> >> +                             &key, desc_size, &desc_version);
> >> +     if (status != EFI_BUFFER_TOO_SMALL)
> >> +             return EFI_LOAD_ERROR;
> >> +
> >>       /*
> >>        * Add an additional efi_memory_desc_t because we're doing an
> >>        * allocation which may be in a new descriptor region.
> >>        */
> >> -     *map_size += sizeof(*m);
> >> +     *map_size += *desc_size;
> >>       status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> >>                               *map_size, (void **)&m);
> >>       if (status != EFI_SUCCESS)
> >>               goto fail;
> >>
> >> -     *desc_size = 0;
> >> -     key = 0;
> >>       status = efi_call_early(get_memory_map, map_size, m,
> >>                               &key, desc_size, &desc_version);
> >>       if (status == EFI_BUFFER_TOO_SMALL) {
> >>               efi_call_early(free_pool, m);
> >> -             goto again;
> >> +             return EFI_LOAD_ERROR;
> >>       }
> >>
> >>       if (status != EFI_SUCCESS)
> >> --
> >> 1.8.3.2
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Leif Lindholm]

On Fri, Jan 09, 2015 at 10:19:50AM +0000, Mark Rutland wrote:
> On Thu, Jan 08, 2015 at 07:09:22PM +0000, Ard Biesheuvel wrote:
> > On 8 January 2015 at 19:04, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > > Hi Ard,
> > >
> > > On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
> > >> This fixes two minor issues in the implementation of get_memory_map():
> > >> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
> > >>   which is usually true, but not mandated by the spec. (This was added
> > >>   intentionally to allow future additions to the definition of
> > >>   efi_memory_desc_t). The way the loop is implemented currently, the
> > >>   added slack space may be insufficient if desc_size is larger, which in
> > >>   some corner cases could result in the loop never terminating.
> > >> - It allocates 32 efi_memory_desc_t entries first (again, using the size
> > >>   of the struct instead of desc_size), and frees and reallocates if it
> > >>   turns out to be insufficient. Few implementations of UEFI have such small
> > >>   memory maps, which results in a unnecessary allocate/free pair on each
> > >>   invocation.
> > >>
> > >> Fix this by calling the get_memory_map() boot service first with a '0'
> > >> input value for map size to retrieve the map size and desc size from the
> > >> firmware and only then perform the allocation, using desc_size rather
> > >> than sizeof(efi_memory_desc_t).
> > >
> > > Is the desc_size guaranteed to be set up correctly if the size is too
> > > small?
> > >
> > > As far as I can see, for that case the spec only mandates that
> > > MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.
> > >
> > > It's not clear to me whether DescriptorSize or DescriptorVersion are
> > > initialised in cases other than success.
> > >
> > 
> > The way I read it, descriptor size and descriptor version are always
> > returned, e.g., as opposed to MapKey, which is only returned on
> > success, and the spec mentions that specifically.
> 
> I agree that that would be the sensible reading of the spec. I just fear
> that there's room for an implementor to read it slightly differently,
> and cause pain for us.

I disagree.
The intent is clear, and you can not follow the current text and
provide a version that does not do the right thing. Only the MapKey
return is conditional on success.
 
> > We could ask for clarification just to be sure.
> 
> I think we should.
> 
> Given it could take a while for any conclusion to be reached and
> published, I'm happy to go with the patch below in the meantime, so long
> as the issue gets raised.

We could ask for clarification of the spec, but I see no need to ask
for guidance on the current text.

/
    Leif

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Mark Rutland]

On Fri, Jan 09, 2015 at 10:55:29AM +0000, Leif Lindholm wrote:
> On Fri, Jan 09, 2015 at 10:19:50AM +0000, Mark Rutland wrote:
> > On Thu, Jan 08, 2015 at 07:09:22PM +0000, Ard Biesheuvel wrote:
> > > On 8 January 2015 at 19:04, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > > > Hi Ard,
> > > >
> > > > On Thu, Jan 08, 2015 at 05:51:47PM +0000, Ard Biesheuvel wrote:
> > > >> This fixes two minor issues in the implementation of get_memory_map():
> > > >> - Currently, it assumes that sizeof(efi_memory_desc_t) == desc_size,
> > > >>   which is usually true, but not mandated by the spec. (This was added
> > > >>   intentionally to allow future additions to the definition of
> > > >>   efi_memory_desc_t). The way the loop is implemented currently, the
> > > >>   added slack space may be insufficient if desc_size is larger, which in
> > > >>   some corner cases could result in the loop never terminating.
> > > >> - It allocates 32 efi_memory_desc_t entries first (again, using the size
> > > >>   of the struct instead of desc_size), and frees and reallocates if it
> > > >>   turns out to be insufficient. Few implementations of UEFI have such small
> > > >>   memory maps, which results in a unnecessary allocate/free pair on each
> > > >>   invocation.
> > > >>
> > > >> Fix this by calling the get_memory_map() boot service first with a '0'
> > > >> input value for map size to retrieve the map size and desc size from the
> > > >> firmware and only then perform the allocation, using desc_size rather
> > > >> than sizeof(efi_memory_desc_t).
> > > >
> > > > Is the desc_size guaranteed to be set up correctly if the size is too
> > > > small?
> > > >
> > > > As far as I can see, for that case the spec only mandates that
> > > > MemoryMapSize is updated nd EFI_BUFFER_TOO_SMALL is returned.
> > > >
> > > > It's not clear to me whether DescriptorSize or DescriptorVersion are
> > > > initialised in cases other than success.
> > > >
> > > 
> > > The way I read it, descriptor size and descriptor version are always
> > > returned, e.g., as opposed to MapKey, which is only returned on
> > > success, and the spec mentions that specifically.
> > 
> > I agree that that would be the sensible reading of the spec. I just fear
> > that there's room for an implementor to read it slightly differently,
> > and cause pain for us.
> 
> I disagree.
> The intent is clear, and you can not follow the current text and
> provide a version that does not do the right thing. Only the MapKey
> return is conditional on success.
>  
> > > We could ask for clarification just to be sure.
> > 
> > I think we should.
> > 
> > Given it could take a while for any conclusion to be reached and
> > published, I'm happy to go with the patch below in the meantime, so long
> > as the issue gets raised.
> 
> We could ask for clarification of the spec, but I see no need to ask
> for guidance on the current text.

I'm fine with that.

Mark.

Re: [PATCH] efi: stub: call get_memory_map() to obtain map and desc sizes

[Matt Fleming]

On Fri, 09 Jan, at 10:55:29AM, Leif Lindholm wrote:
> 
> I disagree.
> The intent is clear, and you can not follow the current text and
> provide a version that does not do the right thing. Only the MapKey
> return is conditional on success.

Tianocore does in fact appear to set the descriptor version and size
even with a zero sized memory map buffer, so that's encouraging.

The real test will be to see how this patch performs in the wild, so
let's get it in linux-next?

-- 
Matt Fleming, Intel Open Source Technology Center