Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Sun, Oct 14, 2018 at 12:05:41AM +0200, Borislav Petkov wrote:
> On Sat, Oct 13, 2018 at 05:45:51PM -0400, Masayoshi Mizuma wrote:
> > Yes, but I think we need to arrange the Chao's SRAT parsing to be used
> > from kernel_randomize_memory() because Chao's approach needs the SRAT
> > parsing before extract kernel and the padding size calculation needs
> > the parsing in start_kernel(), so they are living in different life
> > cycle and space.
>
> Or you would like to pass some info from the compressed kernel to kernel
> proper?
>
> For example, something like the passing in add_e820ext() and the consumtion
> in parse_setup_data():
>
> switch (data_type) {
> case SETUP_E820_EXT:
> e820__memory_setup_extended(pa_data, data_len);
>
> So info you need for your padding gets collected in
> arch/x86/boot/compressed/acpitb.c and you consume it in
> kernel_randomize_memory()?
>
> Would that work?
It's nice idea! Thank you so much!
- Masa
Re: [RFC PATCH] x86/efi: Allocate e820 buffer before calling efi_exit_boot_service
On 14 October 2018 at 18:51, Eric Snowberg wrote:
>
>> On Oct 14, 2018, at 9:34 AM, Jeffrey Hugo wrote:
>>
>> On 10/12/2018 3:46 PM, Ard Biesheuvel wrote:
>>> On 9 August 2018 at 16:50, Eric Snowberg wrote:
Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
introduced a regression on systems with large memory maps
causing them to hang on boot. The first "goto get_map" that was removed
from exit_boot insured there was enough room for the memory map when
efi_call_early(exit_boot_services) was called. This happens when
(nr_desc > ARRAY_SIZE(params->e820_table).
Chain of events:
exit_boot()
efi_exit_boot_services()
efi_get_memory_map <- at this point the mm can't grow over 8 desc
priv_func()
exit_boot_func()
allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
efi_call_early(exit_boot_services) <- mm key doesn't match so retry
efi_call_early(get_memory_map) <- not enough room for new mm
system hangs
This patch allocates the e820 buffer before calling efi_exit_boot_services
and fixes the regression.
Signed-off-by: Eric Snowberg
---
arch/x86/boot/compressed/eboot.c | 63
+---
1 file changed, 40 insertions(+), 23 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c
b/arch/x86/boot/compressed/eboot.c
index cbf4b87..91a650e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -866,11 +866,43 @@ static efi_status_t alloc_e820ext(u32 nr_desc,
struct setup_data **e820ext,
return status;
}
+static efi_status_t allocate_e820(struct boot_params *params,
+ struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ unsigned long map_size, desc_size, buff_size;
+ struct efi_boot_memmap boot_map;
+ efi_memory_desc_t *map;
+ efi_status_t status;
+ __u32 nr_desc;
+
+ boot_map.map =
+ boot_map.map_size = _size;
+ boot_map.desc_size =_size;
+ boot_map.desc_ver = NULL;
+ boot_map.key_ptr = NULL;
+ boot_map.buff_size =_size;
+
+ status = efi_get_memory_map(sys_table, _map);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ nr_desc = buff_size / desc_size;
+
>>> I think we should add some slack here. This function is now called
>>> right before the first call to ExitBootServices(), which means events
>>> may fire that allocate or free memory between this call to
>>> efi_get_memory_map() and the final one which obtains the map key
>>> (which is the whole reason for this complicated dance)
>>
>> Slack in addition to what efi_get_memory_map() provides?
>
> Would additional slack be necessary here? This takes place before memory
> allocation is no longer possible. The efi_get_memory_map function will be
> called again afterwards insuring their is slack available for the memory map
> passed into EBS.
>
The point is that memory allocation is always possible due to
asynchronous events firing. Only after the first call to EBS() is it
guaranteed that event dispatch has terminated.
But to Jeffrey's point, efi_get_memory_map() already has 8 slots'
worth of slack, which means that we have 8 spare e820 table entries as
well. I guess this should be sufficient.
Re: [RFC PATCH] x86/efi: Allocate e820 buffer before calling efi_exit_boot_service
> On Oct 14, 2018, at 9:34 AM, Jeffrey Hugo wrote:
>
> On 10/12/2018 3:46 PM, Ard Biesheuvel wrote:
>> On 9 August 2018 at 16:50, Eric Snowberg wrote:
>>> Commit d64934019f6c ("x86/efi: Use efi_exit_boot_services()")
>>> introduced a regression on systems with large memory maps
>>> causing them to hang on boot. The first "goto get_map" that was removed
>>> from exit_boot insured there was enough room for the memory map when
>>> efi_call_early(exit_boot_services) was called. This happens when
>>> (nr_desc > ARRAY_SIZE(params->e820_table).
>>>
>>> Chain of events:
>>> exit_boot()
>>> efi_exit_boot_services()
>>> efi_get_memory_map <- at this point the mm can't grow over 8 desc
>>> priv_func()
>>> exit_boot_func()
>>> allocate_e820ext() <- new mm grows over 8 desc from e820 alloc
>>> efi_call_early(exit_boot_services) <- mm key doesn't match so retry
>>> efi_call_early(get_memory_map) <- not enough room for new mm
>>> system hangs
>>>
>>> This patch allocates the e820 buffer before calling efi_exit_boot_services
>>> and fixes the regression.
>>>
>>> Signed-off-by: Eric Snowberg
>>> ---
>>> arch/x86/boot/compressed/eboot.c | 63
>>> +---
>>> 1 file changed, 40 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c
>>> b/arch/x86/boot/compressed/eboot.c
>>> index cbf4b87..91a650e 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -866,11 +866,43 @@ static efi_status_t alloc_e820ext(u32 nr_desc, struct
>>> setup_data **e820ext,
>>> return status;
>>> }
>>>
>>> +static efi_status_t allocate_e820(struct boot_params *params,
>>> + struct setup_data **e820ext,
>>> + u32 *e820ext_size)
>>> +{
>>> + unsigned long map_size, desc_size, buff_size;
>>> + struct efi_boot_memmap boot_map;
>>> + efi_memory_desc_t *map;
>>> + efi_status_t status;
>>> + __u32 nr_desc;
>>> +
>>> + boot_map.map =
>>> + boot_map.map_size = _size;
>>> + boot_map.desc_size =_size;
>>> + boot_map.desc_ver = NULL;
>>> + boot_map.key_ptr = NULL;
>>> + boot_map.buff_size =_size;
>>> +
>>> + status = efi_get_memory_map(sys_table, _map);
>>> + if (status != EFI_SUCCESS)
>>> + return status;
>>> +
>>> + nr_desc = buff_size / desc_size;
>>> +
>> I think we should add some slack here. This function is now called
>> right before the first call to ExitBootServices(), which means events
>> may fire that allocate or free memory between this call to
>> efi_get_memory_map() and the final one which obtains the map key
>> (which is the whole reason for this complicated dance)
>
> Slack in addition to what efi_get_memory_map() provides?
Would additional slack be necessary here? This takes place before memory
allocation is no longer possible. The efi_get_memory_map function will be
called again afterwards insuring their is slack available for the memory map
passed into EBS.
[PATCH 3.16 299/366] efi: Avoid potential crashes, fix the 'struct efi_pci_io_protocol_32' definition for mixed mode
3.16.60-rc1 review patch. If anyone has any objections, please let me know.
--
From: Ard Biesheuvel
commit 0b3225ab9407f557a8e20f23f37aa7236c10a9b1 upstream.
Mixed mode allows a kernel built for x86_64 to interact with 32-bit
EFI firmware, but requires us to define all struct definitions carefully
when it comes to pointer sizes.
'struct efi_pci_io_protocol_32' currently uses a 'void *' for the
'romimage' field, which will be interpreted as a 64-bit field
on such kernels, potentially resulting in bogus memory references
and subsequent crashes.
Tested-by: Hans de Goede
Signed-off-by: Ard Biesheuvel
Cc: Linus Torvalds
Cc: Matt Fleming
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: linux-efi@vger.kernel.org
Link: http://lkml.kernel.org/r/20180504060003.19618-13-ard.biesheu...@linaro.org
Signed-off-by: Ingo Molnar
Signed-off-by: Ben Hutchings
---
arch/x86/boot/compressed/eboot.c | 6 --
include/linux/efi.h | 8
2 files changed, 8 insertions(+), 6 deletions(-)
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -358,7 +358,8 @@ __setup_efi_pci32(efi_pci_io_protocol_32
if (status != EFI_SUCCESS)
goto free_struct;
- memcpy(rom->romdata, pci->romimage, pci->romsize);
+ memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+ pci->romsize);
return status;
free_struct:
@@ -460,7 +461,8 @@ __setup_efi_pci64(efi_pci_io_protocol_64
if (status != EFI_SUCCESS)
goto free_struct;
- memcpy(rom->romdata, pci->romimage, pci->romsize);
+ memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+ pci->romsize);
return status;
free_struct:
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -368,8 +368,8 @@ typedef struct {
u32 attributes;
u32 get_bar_attributes;
u32 set_bar_attributes;
- uint64_t romsize;
- void *romimage;
+ u64 romsize;
+ u32 romimage;
} efi_pci_io_protocol_32;
typedef struct {
@@ -388,8 +388,8 @@ typedef struct {
u64 attributes;
u64 get_bar_attributes;
u64 set_bar_attributes;
- uint64_t romsize;
- void *romimage;
+ u64 romsize;
+ u64 romimage;
} efi_pci_io_protocol_64;
typedef struct {
