Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1551961 > unrolled thread

[PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init() Nicolai Stange <nicstange@gmail.com> - 2017-01-05 14:00 +0100
    Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-01-06 09:40 +0100
      Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init() Nicolai Stange <nicstange@gmail.com> - 2017-01-06 14:10 +0100
        Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-01-06 17:50 +0100
          Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init() Nicolai Stange <nicstange@gmail.com> - 2017-01-06 18:50 +0100
            Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-01-06 20:30 +0100
              Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init() Nicolai Stange <nicstange@gmail.com> - 2017-01-08 01:30 +0100
                Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-09 14:20 +0100
              Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through  memblock after mm_init() Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-09 14:10 +0100

#1551961 — [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

Before invoking the arch specific handler, efi_mem_reserve() reserves
the given memory region through memblock.

efi_mem_reserve() can get called after mm_init() though -- through
efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
not be used anymore.

Let efi_mem_reserve() check whether memblock is dead and not do the
reservation if so. Emit a warning from the generic efi_arch mem_reserve()
in this case: if the architecture doesn't provide any other means of
registering the region as reserved, the operation would be a nop.

Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
Applicable to next-20170105.
No changes to v2.
Boot-tested on x86_64.

 drivers/firmware/efi/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 92914801e388..158a8df2f4af 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
 	return end;
 }
 
-void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
+void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
+{
+	WARN(slab_is_available(), "efi_mem_reserve() has no effect");
+}
 
 /**
  * efi_mem_reserve - Reserve an EFI memory region
@@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
  */
 void __init efi_mem_reserve(phys_addr_t addr, u64 size)
 {
-	if (!memblock_is_region_reserved(addr, size))
+	if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
 		memblock_reserve(addr, size);
 
 	/*
-- 
2.11.0

[toc] | [next] | [standalone]

#1552597 — Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
> Before invoking the arch specific handler, efi_mem_reserve() reserves
> the given memory region through memblock.
>
> efi_mem_reserve() can get called after mm_init() though -- through
> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> not be used anymore.
>
> Let efi_mem_reserve() check whether memblock is dead and not do the
> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
> in this case: if the architecture doesn't provide any other means of
> registering the region as reserved, the operation would be a nop.
>
> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> Applicable to next-20170105.
> No changes to v2.
> Boot-tested on x86_64.
>
>  drivers/firmware/efi/efi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 92914801e388..158a8df2f4af 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>         return end;
>  }
>
> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
> +{
> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
> +}
>
>  /**
>   * efi_mem_reserve - Reserve an EFI memory region
> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>   */
>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>  {
> -       if (!memblock_is_region_reserved(addr, size))
> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>                 memblock_reserve(addr, size);
>

I share Dave's concern: on x86, this will silently ignore the
reservation if slab_is_available() returns true, so we should at least
warn here. I don't think this patch solves any known issues, so I'd
rather defer this for now, and pick up the discussion when Matt is
back,

[toc] | [prev] | [next] | [standalone]

#1552769

Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:

> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>> the given memory region through memblock.
>>
>> efi_mem_reserve() can get called after mm_init() though -- through
>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>> not be used anymore.
>>
>> Let efi_mem_reserve() check whether memblock is dead and not do the
>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>> in this case: if the architecture doesn't provide any other means of
>> registering the region as reserved, the operation would be a nop.
>>
>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>> Applicable to next-20170105.
>> No changes to v2.
>> Boot-tested on x86_64.
>>
>>  drivers/firmware/efi/efi.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index 92914801e388..158a8df2f4af 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>         return end;
>>  }
>>
>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>> +{
>> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>> +}
>>
>>  /**
>>   * efi_mem_reserve - Reserve an EFI memory region
>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>   */
>>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>  {
>> -       if (!memblock_is_region_reserved(addr, size))
>> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>                 memblock_reserve(addr, size);
>>

More context:

            /*
             * Some architectures (x86) reserve all boot services ranges
             * until efi_free_boot_services() because of buggy firmware
             * implementations. This means the above memblock_reserve() is
             * superfluous on x86 and instead what it needs to do is
             * ensure the @start, @size is not freed.
             */
            efi_arch_mem_reserve(addr, size);
    }


> I share Dave's concern: on x86, this will silently ignore the
> reservation if slab_is_available() returns true,

AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
reservation at any stage.

The default implementation of efi_arch_mem_reserve() used on ARM is
empty though ... 

> so we should at least warn here.

... and this patch adds a WARN() to the empty stub.


> I don't think this patch solves any known issues, so I'd
> rather defer this for now, and pick up the discussion when Matt is
> back,

I'm fine with either way and yes, no splat has been observed in the
wild.

Just to make it explicit: the issue addressed here is a potential
use-after-free (both, read and write) on memblock.reserved.regions in
case of CONFIG_ARCH_DISCARD_MEMBLOCK=y. It would certainly make sense to
clarify the commit description in the next iteration...

Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1552951 — Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On 6 January 2017 at 13:02, Nicolai Stange <nicstange@gmail.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>
>> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>>> the given memory region through memblock.
>>>
>>> efi_mem_reserve() can get called after mm_init() though -- through
>>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>>> not be used anymore.
>>>
>>> Let efi_mem_reserve() check whether memblock is dead and not do the
>>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>>> in this case: if the architecture doesn't provide any other means of
>>> registering the region as reserved, the operation would be a nop.
>>>
>>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>> ---
>>> Applicable to next-20170105.
>>> No changes to v2.
>>> Boot-tested on x86_64.
>>>
>>>  drivers/firmware/efi/efi.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index 92914801e388..158a8df2f4af 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>>         return end;
>>>  }
>>>
>>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>> +{
>>> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>>> +}
>>>
>>>  /**
>>>   * efi_mem_reserve - Reserve an EFI memory region
>>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>   */
>>>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>>  {
>>> -       if (!memblock_is_region_reserved(addr, size))
>>> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>>                 memblock_reserve(addr, size);
>>>
>
> More context:
>
>             /*
>              * Some architectures (x86) reserve all boot services ranges
>              * until efi_free_boot_services() because of buggy firmware
>              * implementations. This means the above memblock_reserve() is
>              * superfluous on x86 and instead what it needs to do is
>              * ensure the @start, @size is not freed.
>              */
>             efi_arch_mem_reserve(addr, size);
>     }
>
>
>> I share Dave's concern: on x86, this will silently ignore the
>> reservation if slab_is_available() returns true,
>
> AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
> reservation at any stage.
>

Thanks for the clarification. But my concern is whether changing the
EFI memory map is going to have any effect at this stage, i.e., after
slab_is_available() returns true: haven't we already communicated to
the kernel which RAM regions it may allocate from? How does it know
the memory map has changed, and how do we ensure that it has not
already allocated from the region we are reserving here?

> The default implementation of efi_arch_mem_reserve() used on ARM is
> empty though ...
>
>> so we should at least warn here.
>
> ... and this patch adds a WARN() to the empty stub.
>
>
>> I don't think this patch solves any known issues, so I'd
>> rather defer this for now, and pick up the discussion when Matt is
>> back,
>
> I'm fine with either way and yes, no splat has been observed in the
> wild.
>
> Just to make it explicit: the issue addressed here is a potential
> use-after-free (both, read and write) on memblock.reserved.regions in
> case of CONFIG_ARCH_DISCARD_MEMBLOCK=y. It would certainly make sense to
> clarify the commit description in the next iteration...
>

[toc] | [prev] | [next] | [standalone]

#1552991

Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:

> On 6 January 2017 at 13:02, Nicolai Stange <nicstange@gmail.com> wrote:
>> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>>
>>> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>>>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>>>> the given memory region through memblock.
>>>>
>>>> efi_mem_reserve() can get called after mm_init() though -- through
>>>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>>>> not be used anymore.
>>>>
>>>> Let efi_mem_reserve() check whether memblock is dead and not do the
>>>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>>>> in this case: if the architecture doesn't provide any other means of
>>>> registering the region as reserved, the operation would be a nop.
>>>>
>>>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>>> ---
>>>> Applicable to next-20170105.
>>>> No changes to v2.
>>>> Boot-tested on x86_64.
>>>>
>>>>  drivers/firmware/efi/efi.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>>> index 92914801e388..158a8df2f4af 100644
>>>> --- a/drivers/firmware/efi/efi.c
>>>> +++ b/drivers/firmware/efi/efi.c
>>>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>>>         return end;
>>>>  }
>>>>
>>>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>>> +{
>>>> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>>>> +}
>>>>
>>>>  /**
>>>>   * efi_mem_reserve - Reserve an EFI memory region
>>>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>>   */
>>>>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>>>  {
>>>> -       if (!memblock_is_region_reserved(addr, size))
>>>> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>>>                 memblock_reserve(addr, size);
>>>>
>>
>> More context:
>>
>>             /*
>>              * Some architectures (x86) reserve all boot services ranges
>>              * until efi_free_boot_services() because of buggy firmware
>>              * implementations. This means the above memblock_reserve() is
>>              * superfluous on x86 and instead what it needs to do is
>>              * ensure the @start, @size is not freed.
>>              */
>>             efi_arch_mem_reserve(addr, size);
>>     }
>>
>>
>>> I share Dave's concern: on x86, this will silently ignore the
>>> reservation if slab_is_available() returns true,
>>
>> AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
>> reservation at any stage.
>>
>
> Thanks for the clarification. But my concern is whether changing the
> EFI memory map is going to have any effect at this stage, i.e., after
> slab_is_available() returns true: haven't we already communicated to
> the kernel which RAM regions it may allocate from? How does it know
> the memory map has changed, and how do we ensure that it has not
> already allocated from the region we are reserving here?

Ah, I see what you mean. I think it works like this on x86:

All EFI_BOOT_SERVICES_* regions as reported by the firmware are marked
as reserved at memblock unconditionally through the early setup_arch()
=> efi_reserve_boot_services(). This prevents these from getting handed
over to the "normal" kernel MM until efi_free_boot_services()
gets called later on. The latter frees these EFI_BOOT_SERVICES_* regions,
but only if their EFI_MEMORY_RUNTIME flag is not set.

Now, efi_arch_mem_reserve() basically just sets the EFI_MEMORY_RUNTIME
flag, allowing the given region to survive beyond efi_free_boot_services().

Corrolary 1: any efi_mem_reserve() after efi_free_boot_services wouldn't
have any effect.

Corollary 2: anything handed to efi_arch_mem_reserve() must live within
some memory region which had been reported by firmware already.

Indeed, at its very top, there is

  if (efi_mem_desc_lookup(addr, &md)) {
    pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
    return;
  }

  if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
  	pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
  	return;
  }

For further information, the comment at the x86's efi_arch_mem_reserve()
might be helpful.


I hope this is correct and helps.

Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1553053 — Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On 6 January 2017 at 17:46, Nicolai Stange <nicstange@gmail.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>
>> On 6 January 2017 at 13:02, Nicolai Stange <nicstange@gmail.com> wrote:
>>> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>>>
>>>> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>>>>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>>>>> the given memory region through memblock.
>>>>>
>>>>> efi_mem_reserve() can get called after mm_init() though -- through
>>>>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>>>>> not be used anymore.
>>>>>
>>>>> Let efi_mem_reserve() check whether memblock is dead and not do the
>>>>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>>>>> in this case: if the architecture doesn't provide any other means of
>>>>> registering the region as reserved, the operation would be a nop.
>>>>>
>>>>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>>>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>>>> ---
>>>>> Applicable to next-20170105.
>>>>> No changes to v2.
>>>>> Boot-tested on x86_64.
>>>>>
>>>>>  drivers/firmware/efi/efi.c | 7 +++++--
>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>>>> index 92914801e388..158a8df2f4af 100644
>>>>> --- a/drivers/firmware/efi/efi.c
>>>>> +++ b/drivers/firmware/efi/efi.c
>>>>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>>>>         return end;
>>>>>  }
>>>>>
>>>>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>>>> +{
>>>>> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>>>>> +}
>>>>>
>>>>>  /**
>>>>>   * efi_mem_reserve - Reserve an EFI memory region
>>>>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>>>   */
>>>>>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>>>>  {
>>>>> -       if (!memblock_is_region_reserved(addr, size))
>>>>> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>>>>                 memblock_reserve(addr, size);
>>>>>
>>>
>>> More context:
>>>
>>>             /*
>>>              * Some architectures (x86) reserve all boot services ranges
>>>              * until efi_free_boot_services() because of buggy firmware
>>>              * implementations. This means the above memblock_reserve() is
>>>              * superfluous on x86 and instead what it needs to do is
>>>              * ensure the @start, @size is not freed.
>>>              */
>>>             efi_arch_mem_reserve(addr, size);
>>>     }
>>>
>>>
>>>> I share Dave's concern: on x86, this will silently ignore the
>>>> reservation if slab_is_available() returns true,
>>>
>>> AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
>>> reservation at any stage.
>>>
>>
>> Thanks for the clarification. But my concern is whether changing the
>> EFI memory map is going to have any effect at this stage, i.e., after
>> slab_is_available() returns true: haven't we already communicated to
>> the kernel which RAM regions it may allocate from? How does it know
>> the memory map has changed, and how do we ensure that it has not
>> already allocated from the region we are reserving here?
>
> Ah, I see what you mean. I think it works like this on x86:
>
> All EFI_BOOT_SERVICES_* regions as reported by the firmware are marked
> as reserved at memblock unconditionally through the early setup_arch()
> => efi_reserve_boot_services(). This prevents these from getting handed
> over to the "normal" kernel MM until efi_free_boot_services()
> gets called later on. The latter frees these EFI_BOOT_SERVICES_* regions,
> but only if their EFI_MEMORY_RUNTIME flag is not set.
>
> Now, efi_arch_mem_reserve() basically just sets the EFI_MEMORY_RUNTIME
> flag, allowing the given region to survive beyond efi_free_boot_services().
>
> Corrolary 1: any efi_mem_reserve() after efi_free_boot_services wouldn't
> have any effect.
>

This is my point exactly. But it appears efi_free_boot_services()
occurs much later than I thought, and so there is a sizabe time window
where SLAB is up but reservations can still be made. But we don't
check whether efi_free_boot_services() has been called. Another
problem is that we never check that the reservation is covered by a
BootServicesData region, which are the only ones that are guaranteed
to be retained up to this point.

> Corollary 2: anything handed to efi_arch_mem_reserve() must live within
> some memory region which had been reported by firmware already.
>

Yes, but the EFI memory map describes all of RAM, so this is not an
issue by itself. The issue is that the region must have been covered
by a BootServicesCode or BootServicesData region

> Indeed, at its very top, there is
>
>   if (efi_mem_desc_lookup(addr, &md)) {
>     pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
>     return;
>   }
>
>   if (addr + size > md.phys_addr + (md.num_pages << EFI_PAGE_SHIFT)) {
>         pr_err("Region spans EFI memory descriptors, %pa\n", &addr);
>         return;
>   }
>
> For further information, the comment at the x86's efi_arch_mem_reserve()
> might be helpful.
>
>
> I hope this is correct and helps.
>
> Thanks,
>
> Nicolai

[toc] | [prev] | [next] | [standalone]

#1553772

Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:

> On 6 January 2017 at 17:46, Nicolai Stange <nicstange@gmail.com> wrote:
>> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>>
>>> On 6 January 2017 at 13:02, Nicolai Stange <nicstange@gmail.com> wrote:
>>>> Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
>>>>
>>>>> On 5 January 2017 at 12:51, Nicolai Stange <nicstange@gmail.com> wrote:
>>>>>> Before invoking the arch specific handler, efi_mem_reserve() reserves
>>>>>> the given memory region through memblock.
>>>>>>
>>>>>> efi_mem_reserve() can get called after mm_init() though -- through
>>>>>> efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
>>>>>> not be used anymore.
>>>>>>
>>>>>> Let efi_mem_reserve() check whether memblock is dead and not do the
>>>>>> reservation if so. Emit a warning from the generic efi_arch mem_reserve()
>>>>>> in this case: if the architecture doesn't provide any other means of
>>>>>> registering the region as reserved, the operation would be a nop.
>>>>>>
>>>>>> Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
>>>>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>>>>> ---
>>>>>> Applicable to next-20170105.
>>>>>> No changes to v2.
>>>>>> Boot-tested on x86_64.
>>>>>>
>>>>>>  drivers/firmware/efi/efi.c | 7 +++++--
>>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>>>>> index 92914801e388..158a8df2f4af 100644
>>>>>> --- a/drivers/firmware/efi/efi.c
>>>>>> +++ b/drivers/firmware/efi/efi.c
>>>>>> @@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
>>>>>>         return end;
>>>>>>  }
>>>>>>
>>>>>> -void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>>>> +void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
>>>>>> +{
>>>>>> +       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
>>>>>> +}
>>>>>>
>>>>>>  /**
>>>>>>   * efi_mem_reserve - Reserve an EFI memory region
>>>>>> @@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
>>>>>>   */
>>>>>>  void __init efi_mem_reserve(phys_addr_t addr, u64 size)
>>>>>>  {
>>>>>> -       if (!memblock_is_region_reserved(addr, size))
>>>>>> +       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
>>>>>>                 memblock_reserve(addr, size);
>>>>>>
>>>>
>>>> More context:
>>>>
>>>>             /*
>>>>              * Some architectures (x86) reserve all boot services ranges
>>>>              * until efi_free_boot_services() because of buggy firmware
>>>>              * implementations. This means the above memblock_reserve() is
>>>>              * superfluous on x86 and instead what it needs to do is
>>>>              * ensure the @start, @size is not freed.
>>>>              */
>>>>             efi_arch_mem_reserve(addr, size);
>>>>     }
>>>>
>>>>
>>>>> I share Dave's concern: on x86, this will silently ignore the
>>>>> reservation if slab_is_available() returns true,
>>>>
>>>> AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
>>>> reservation at any stage.
>>>>
>>>
>>> Thanks for the clarification. But my concern is whether changing the
>>> EFI memory map is going to have any effect at this stage, i.e., after
>>> slab_is_available() returns true: haven't we already communicated to
>>> the kernel which RAM regions it may allocate from? How does it know
>>> the memory map has changed, and how do we ensure that it has not
>>> already allocated from the region we are reserving here?
>>
>> Ah, I see what you mean. I think it works like this on x86:
>>
>> All EFI_BOOT_SERVICES_* regions as reported by the firmware are marked
>> as reserved at memblock unconditionally through the early setup_arch()
>> => efi_reserve_boot_services(). This prevents these from getting handed
>> over to the "normal" kernel MM until efi_free_boot_services()
>> gets called later on. The latter frees these EFI_BOOT_SERVICES_* regions,
>> but only if their EFI_MEMORY_RUNTIME flag is not set.
>>
>> Now, efi_arch_mem_reserve() basically just sets the EFI_MEMORY_RUNTIME
>> flag, allowing the given region to survive beyond efi_free_boot_services().
>>
>> Corrolary 1: any efi_mem_reserve() after efi_free_boot_services wouldn't
>> have any effect.
>>
>
> This is my point exactly. But it appears efi_free_boot_services()
> occurs much later than I thought, and so there is a sizabe time window
> where SLAB is up but reservations can still be made. But we don't
> check whether efi_free_boot_services() has been called.

Ok, but this patch is about slab_is_available()/resp. the
non-availability of memblock and I'd rather consider the implementation
of these kinds of safety checks as being a different story?



Out of curiosity, I had a deeper look at the BootServices*-md
requirement though:

> Another problem is that we never check that the reservation is covered
> by a BootServicesData region, which are the only ones that are
> guaranteed to be retained up to this point.

I think the "only ones that are guaranteed to be retained" part might
not be completely correct: at least my firmware seems to report only the
EFI_CONVENTIONAL_MEMORY, EFI_LOADER_DATA, EFI_LOADER_CODE,
EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA as E820_RAM
(I think that these mappings are dictated by table 15-330 of ACPI 6.1:
"UEFI Memory Types and mapping to ACPI address range types").

This would mean, that memblock_x86_fill() adds only these regions to
memblock.memory.

free_all_bootmem() only operates on the (non-highmem) regions given by
memblock.memory and thus, any region of a type different from the ones
listed above would never get freed to the buddy allocator anyway, AFAICS.

Thus, the only md type where ranges efi_mem_reserve()'d therein aren't
retained are EFI_CONVENTIONAL_MEMORY, EFI_LOADER_DATA and
EFI_LOADER_CODE (and possibly highmem). Hopefully, nobody would ever
call efi_mem_reserve() on such a range but that assumption might be
wrong.


>> Corollary 2: anything handed to efi_arch_mem_reserve() must live within
>> some memory region which had been reported by firmware already.
>>
>
> Yes, but the EFI memory map describes all of RAM, so this is not an
> issue by itself.

Ok. But I've just realized that after __efi_enter_virtual_mode(),
efi_map_regions() would have stripped that list down to only those MDs
for which should_map_region() returns true. With efi_is_native(), that's
everything having EFI_MEMORY_RUNTIME set and the BOOT_SERVICES_*
regions.


Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1554302 — Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On Sun, 08 Jan, at 01:24:49AM, Nicolai Stange wrote:
> 
> Out of curiosity, I had a deeper look at the BootServices*-md
> requirement though:
> 
> > Another problem is that we never check that the reservation is covered
> > by a BootServicesData region, which are the only ones that are
> > guaranteed to be retained up to this point.
> 
> I think the "only ones that are guaranteed to be retained" part might
> not be completely correct: at least my firmware seems to report only the
> EFI_CONVENTIONAL_MEMORY, EFI_LOADER_DATA, EFI_LOADER_CODE,
> EFI_BOOT_SERVICES_CODE and EFI_BOOT_SERVICES_DATA as E820_RAM
> (I think that these mappings are dictated by table 15-330 of ACPI 6.1:
> "UEFI Memory Types and mapping to ACPI address range types").
> 
> This would mean, that memblock_x86_fill() adds only these regions to
> memblock.memory.
 
Data required at runtime should only be in EFI_LOADER* regions if it's
part of some setup_data object (see things like SETUP_EFI), and
subsequently has been memblock_reserve()'d at some point.

Nothing valuable should be in EFI_CONVENTIONAL_MEMORY because, by
definition, it's free memory. 

> free_all_bootmem() only operates on the (non-highmem) regions given by
> memblock.memory and thus, any region of a type different from the ones
> listed above would never get freed to the buddy allocator anyway, AFAICS.
 
This is true.

> Thus, the only md type where ranges efi_mem_reserve()'d therein aren't
> retained are EFI_CONVENTIONAL_MEMORY, EFI_LOADER_DATA and
> EFI_LOADER_CODE (and possibly highmem). Hopefully, nobody would ever
> call efi_mem_reserve() on such a range but that assumption might be
> wrong.

I would happily welcome some diagnostic checks to ensure we never get
silently stung by this.

[toc] | [prev] | [next] | [standalone]

#1554292 — Re: [PATCH v3 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

On Fri, 06 Jan, at 07:28:40PM, Ard Biesheuvel wrote:
> 
> This is my point exactly. But it appears efi_free_boot_services()
> occurs much later than I thought, and so there is a sizabe time window
> where SLAB is up but reservations can still be made. But we don't
> check whether efi_free_boot_services() has been called.

True. This has only been correct thus far because all code has been
audited, but adding a check to catch future offenders is a good idea.

> Another problem is that we never check that the reservation is
> covered by a BootServicesData region, which are the only ones that
> are guaranteed to be retained up to this point.

The runtime regions are guaranteed to be retained too.

Again, this shouldn't actually be a problem today, but the potential
for breakage here warrants some kind of check and loud warning. 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web