Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Fri, Jul 07, 2017 at 11:58:14AM +0100, Matt Fleming wrote:
> On Fri, 07 Jul, at 06:11:24AM, Naoya Horiguchi wrote:
> > On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> > > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > > + for (i = 0; i < nr_desc; i++) {
> > > > > + md = (efi_memory_desc_t *)(pmap + (i *
> > > > > e->efi_memdesc_size));
> > > > > +
> > > > > + /*
> > > > > + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because
> > > > > boot
> > > > > + * services regions could be accessed after
> > > > > ExitBootServices()
> > > > > + * due to the workaround for buggy firmware.
> > > > > + */
> > > > > + if (!(md->type == EFI_LOADER_CODE ||
> > > > > + md->type == EFI_LOADER_DATA ||
> > > > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > > > + continue;
> > > >
> > > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > >
> > > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > > early in boot that you've no idea if data the kernel will need is in
> > > > those EFI_LOADER_* regions.
> > > >
> > > > For example, we pass struct setup_data objects inside of
> > > > EFI_LOADER_DATA regions.
> > >
> > > It doesn't matter because we have tried to avoid those memory setup_data
> > > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > > discard the whole regions while setup_data could occupy small part of
> > > them.
> >
> > Hi Matt, Baoquan,
> >
> > I added these three checks to accept any regions corresponding to
> > E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
> > surprising. Baoquan gave a good justification on that, so I'll leave it
> > as-is in next version.
>
> I disagree. The least surprising option would be to use the region
> type that everyone (boot loader, kernel, firmware) agrees is free:
> EFI_CONVENTIONAL_MEMORY.
OK, I will do it.
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On 07/09/17 at 06:44pm, Baoquan He wrote:
> On 07/07/17 at 11:56am, Matt Fleming wrote:
> > On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> > > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > > + for (i = 0; i < nr_desc; i++) {
> > > > > + md = (efi_memory_desc_t *)(pmap + (i *
> > > > > e->efi_memdesc_size));
> > > > > +
> > > > > + /*
> > > > > + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because
> > > > > boot
> > > > > + * services regions could be accessed after
> > > > > ExitBootServices()
> > > > > + * due to the workaround for buggy firmware.
> > > > > + */
> > > > > + if (!(md->type == EFI_LOADER_CODE ||
> > > > > + md->type == EFI_LOADER_DATA ||
> > > > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > > > + continue;
> > > >
> > > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > > >
> > > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > > early in boot that you've no idea if data the kernel will need is in
> > > > those EFI_LOADER_* regions.
> > > >
> > > > For example, we pass struct setup_data objects inside of
> > > > EFI_LOADER_DATA regions.
> > >
> > > It doesn't matter because we have tried to avoid those memory setup_data
> > > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > > discard the whole regions while setup_data could occupy small part of
> > > them.
> >
> > What about the GDT that we allocate in the x86 EFI boot stub as
> > EFI_LOADER_DATA? Are there functions to avoid that too?
>
> This is a very good question. For the current e820 processing, we don't
> avoid GDT allocated in x86 EFI STUB because we have no information about
> GDT in EFI STUB. You can see setup_e820() in boot/compressed/eboot.c
I was wrong here, we can know gdt information by sgdt. Surely GDT is
just an example, as Matt mentioned, there could be other stuffs we need
avoid, or future change of uefi.
> grabs EFI_LOADER_* regions as E820_TYPE_RAM. Now the GDT which is built
> in EFI STUB code will live till kernel is ready to build its onw gdt(in
> kernel/head_64.S). After that it become useless and can be reclaimed for
> reusing. I believe Naoya must have read boot/compressed/eboot.c and
> found setup_e820() code, then added EFI_LOADER_* for kaslr usage.
>
> Previously I thought e820 should take the precedence to be processed
> on uefi system because continuous efi memory regions will be merged into
> e820 regions. Using e820 can avoid those small efi regions or the left
> part of efi regions being discarded directly when they are smaller than
> kernel image size. Now considering this GDT in efi stub issue, we have to
> try efi regions first on uefi system. Otherwise it may cause problem that
> kernel could be decompressed onto the GDT tables of efi stub.
>
> In fact, I am wondering if we can reuse the gdt table which is built
> before entering into long mode in boot/compressed/head_64.S, but not
> allocate memory for GDT in efi stub. The thing is 32bit system doesn't
> have this gdt table in boot compressing stage since it has one before
> entering into protection mode. Just personal thought.
> >
> > What about any future uses we add? Who's going to remember to patch
> > the kaslr code which now duplicates some of the EFI memory map logic?
> >
> > All of these problems can avoided if you just stick with
> > EFI_CONVENTIONAL_MEMORY.
>
> Below is the dmesg with 'efi=debug' adding on my ovmf uefi kvm guest.
> Since uefi could do a lot of thing when loading OS, E.g loading into any
> kind of storage driver application, firmware usually reserve a chunk of
> memory of hunderes of Mega Bytes. Like this one:
>
> **
> [ +0.00] efi: mem12: [Loader Data| ||WB|WT|WC|UC]
> range=[0x5c8eb000-0x7bfbdfff] (502MB)
> **
>
> We can't say it's a big deal, but 500MB is also not so trival that we
> can easily ignore it without any consideration. Just uefi spec doesn't
> define the limitation of Loader memory and Conventional memory and if
> Conventional memory has to be present. With my understanding, there won't
> be any problem if only Loader memory exists.
>
> Further more, kaslr is not a precise searching job, no specific address
> has to be positioned. Even it's OK that the physical address
> randomization failed to find a new address randomly. So it's fine to me
> that we don't take EFI_LOADER_* memory into consideration for kaslr. BUT
> we need make this clear that why not, and if we can do anyting to make
> it better.
>
> [ +0.00] efi: SMBIOS=0x7fed5000 ACPI=0x7ff03000 ACPI 2.0=0x7ff03014
> MEMATTR=0x7ea50218
> [ +0.00] efi: mem00: [Boot Code | | | | | | | |
> |WB|WT|WC|UC] range=[0x-0x0fff] (0MB)
> [ +0.00] efi:
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On 07/07/17 at 11:56am, Matt Fleming wrote:
> On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > + for (i = 0; i < nr_desc; i++) {
> > > > + md = (efi_memory_desc_t *)(pmap + (i *
> > > > e->efi_memdesc_size));
> > > > +
> > > > + /*
> > > > +* EFI_BOOT_SERVICES_{CODE|DATA} are avoided because
> > > > boot
> > > > +* services regions could be accessed after
> > > > ExitBootServices()
> > > > +* due to the workaround for buggy firmware.
> > > > +*/
> > > > + if (!(md->type == EFI_LOADER_CODE ||
> > > > + md->type == EFI_LOADER_DATA ||
> > > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > > + continue;
> > >
> > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > >
> > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > early in boot that you've no idea if data the kernel will need is in
> > > those EFI_LOADER_* regions.
> > >
> > > For example, we pass struct setup_data objects inside of
> > > EFI_LOADER_DATA regions.
> >
> > It doesn't matter because we have tried to avoid those memory setup_data
> > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > discard the whole regions while setup_data could occupy small part of
> > them.
>
> What about the GDT that we allocate in the x86 EFI boot stub as
> EFI_LOADER_DATA? Are there functions to avoid that too?
This is a very good question. For the current e820 processing, we don't
avoid GDT allocated in x86 EFI STUB because we have no information about
GDT in EFI STUB. You can see setup_e820() in boot/compressed/eboot.c
grabs EFI_LOADER_* regions as E820_TYPE_RAM. Now the GDT which is built
in EFI STUB code will live till kernel is ready to build its onw gdt(in
kernel/head_64.S). After that it become useless and can be reclaimed for
reusing. I believe Naoya must have read boot/compressed/eboot.c and
found setup_e820() code, then added EFI_LOADER_* for kaslr usage.
Previously I thought e820 should take the precedence to be processed
on uefi system because continuous efi memory regions will be merged into
e820 regions. Using e820 can avoid those small efi regions or the left
part of efi regions being discarded directly when they are smaller than
kernel image size. Now considering this GDT in efi stub issue, we have to
try efi regions first on uefi system. Otherwise it may cause problem that
kernel could be decompressed onto the GDT tables of efi stub.
In fact, I am wondering if we can reuse the gdt table which is built
before entering into long mode in boot/compressed/head_64.S, but not
allocate memory for GDT in efi stub. The thing is 32bit system doesn't
have this gdt table in boot compressing stage since it has one before
entering into protection mode. Just personal thought.
>
> What about any future uses we add? Who's going to remember to patch
> the kaslr code which now duplicates some of the EFI memory map logic?
>
> All of these problems can avoided if you just stick with
> EFI_CONVENTIONAL_MEMORY.
Below is the dmesg with 'efi=debug' adding on my ovmf uefi kvm guest.
Since uefi could do a lot of thing when loading OS, E.g loading into any
kind of storage driver application, firmware usually reserve a chunk of
memory of hunderes of Mega Bytes. Like this one:
**
[ +0.00] efi: mem12: [Loader Data| ||WB|WT|WC|UC]
range=[0x5c8eb000-0x7bfbdfff] (502MB)
**
We can't say it's a big deal, but 500MB is also not so trival that we
can easily ignore it without any consideration. Just uefi spec doesn't
define the limitation of Loader memory and Conventional memory and if
Conventional memory has to be present. With my understanding, there won't
be any problem if only Loader memory exists.
Further more, kaslr is not a precise searching job, no specific address
has to be positioned. Even it's OK that the physical address
randomization failed to find a new address randomly. So it's fine to me
that we don't take EFI_LOADER_* memory into consideration for kaslr. BUT
we need make this clear that why not, and if we can do anyting to make
it better.
[ +0.00] efi: SMBIOS=0x7fed5000 ACPI=0x7ff03000 ACPI 2.0=0x7ff03014
MEMATTR=0x7ea50218
[ +0.00] efi: mem00: [Boot Code | | | | | | | |
|WB|WT|WC|UC] range=[0x-0x0fff] (0MB)
[ +0.00] efi: mem01: [Loader Data| | | | | | | |
|WB|WT|WC|UC] range=[0x1000-0x1fff] (0MB)
[ +0.00] efi: mem02: [Conventional Memory| | | | | | | |
|WB|WT|WC|UC] range=[0x2000-0x0009] (0MB)
[ +0.00] efi: mem03: [Conventional Memory| | | | | | | |
|WB|WT|WC|UC] range=[0x0010-0x00805
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Fri, 07 Jul, at 06:11:24AM, Naoya Horiguchi wrote:
> On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> > On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > > + for (i = 0; i < nr_desc; i++) {
> > > > + md = (efi_memory_desc_t *)(pmap + (i *
> > > > e->efi_memdesc_size));
> > > > +
> > > > + /*
> > > > +* EFI_BOOT_SERVICES_{CODE|DATA} are avoided because
> > > > boot
> > > > +* services regions could be accessed after
> > > > ExitBootServices()
> > > > +* due to the workaround for buggy firmware.
> > > > +*/
> > > > + if (!(md->type == EFI_LOADER_CODE ||
> > > > + md->type == EFI_LOADER_DATA ||
> > > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > > + continue;
> > >
> > > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> > >
> > > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > > early in boot that you've no idea if data the kernel will need is in
> > > those EFI_LOADER_* regions.
> > >
> > > For example, we pass struct setup_data objects inside of
> > > EFI_LOADER_DATA regions.
> >
> > It doesn't matter because we have tried to avoid those memory setup_data
> > resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> > discard the whole regions while setup_data could occupy small part of
> > them.
>
> Hi Matt, Baoquan,
>
> I added these three checks to accept any regions corresponding to
> E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
> surprising. Baoquan gave a good justification on that, so I'll leave it
> as-is in next version.
I disagree. The least surprising option would be to use the region
type that everyone (boot loader, kernel, firmware) agrees is free:
EFI_CONVENTIONAL_MEMORY.
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Fri, 07 Jul, at 11:07:59AM, Baoquan He wrote:
> On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > + for (i = 0; i < nr_desc; i++) {
> > > + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> > > +
> > > + /*
> > > + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
> > > + * services regions could be accessed after ExitBootServices()
> > > + * due to the workaround for buggy firmware.
> > > + */
> > > + if (!(md->type == EFI_LOADER_CODE ||
> > > + md->type == EFI_LOADER_DATA ||
> > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > + continue;
> >
> > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> >
> > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > early in boot that you've no idea if data the kernel will need is in
> > those EFI_LOADER_* regions.
> >
> > For example, we pass struct setup_data objects inside of
> > EFI_LOADER_DATA regions.
>
> It doesn't matter because we have tried to avoid those memory setup_data
> resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> discard the whole regions while setup_data could occupy small part of
> them.
What about the GDT that we allocate in the x86 EFI boot stub as
EFI_LOADER_DATA? Are there functions to avoid that too?
What about any future uses we add? Who's going to remember to patch
the kaslr code which now duplicates some of the EFI memory map logic?
All of these problems can avoided if you just stick with
EFI_CONVENTIONAL_MEMORY.
Honestly, how much memory do we expect to waste if we ignore
EFI_LOADER_* regions?
Also, the fact that you're referencing EFI-specific boot quirks in the
kaslr code should be a massive red flag that you're playing with the
innards of the EFI subsystem.
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Fri, Jul 07, 2017 at 11:07:59AM +0800, Baoquan He wrote:
> On 07/06/17 at 03:57pm, Matt Fleming wrote:
> > On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > > + for (i = 0; i < nr_desc; i++) {
> > > + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> > > +
> > > + /*
> > > + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
> > > + * services regions could be accessed after ExitBootServices()
> > > + * due to the workaround for buggy firmware.
> > > + */
> > > + if (!(md->type == EFI_LOADER_CODE ||
> > > + md->type == EFI_LOADER_DATA ||
> > > + md->type == EFI_CONVENTIONAL_MEMORY))
> > > + continue;
> >
> > Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
> >
> > You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> > early in boot that you've no idea if data the kernel will need is in
> > those EFI_LOADER_* regions.
> >
> > For example, we pass struct setup_data objects inside of
> > EFI_LOADER_DATA regions.
>
> It doesn't matter because we have tried to avoid those memory setup_data
> resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
> discard the whole regions while setup_data could occupy small part of
> them.
Hi Matt, Baoquan,
I added these three checks to accept any regions corresponding to
E820_TYPE_RAM except EFI_BOOT_SERVICES_*, just thinking of that it's minimum
surprising. Baoquan gave a good justification on that, so I'll leave it
as-is in next version.
Thanks,
Naoya Horiguchi
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On 07/06/17 at 03:57pm, Matt Fleming wrote:
> On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
> > + for (i = 0; i < nr_desc; i++) {
> > + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> > +
> > + /*
> > +* EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
> > +* services regions could be accessed after ExitBootServices()
> > +* due to the workaround for buggy firmware.
> > +*/
> > + if (!(md->type == EFI_LOADER_CODE ||
> > + md->type == EFI_LOADER_DATA ||
> > + md->type == EFI_CONVENTIONAL_MEMORY))
> > + continue;
>
> Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
>
> You can't re-use EFI_LOADER_* regions because the kaslr code is run so
> early in boot that you've no idea if data the kernel will need is in
> those EFI_LOADER_* regions.
>
> For example, we pass struct setup_data objects inside of
> EFI_LOADER_DATA regions.
It doesn't matter because we have tried to avoid those memory setup_data
resides in in mem_avoid_overlap(). Here discarding EFI_LOADER_* could
discard the whole regions while setup_data could occupy small part of
them.
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, 06 Jul, at 08:31:07AM, Naoya Horiguchi wrote:
>
> KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
> e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
> EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
> UEFI spec, all memory regions marked as EfiBootServicesCode and
> EfiBootServicesData are available for free memory after the first call
> of ExitBootServices(). So such regions should be usable for kernel on
> spec basis.
>
> In x86, however, we have some workaround for broken firmware, where we
> keep such regions reserved until SetVirtualAddressMap() is done.
> See the following code in should_map_region():
>
> static bool should_map_region(efi_memory_desc_t *md)
> {
> ...
> /*
>* Map boot services regions as a workaround for buggy
>* firmware that accesses them even when they shouldn't.
>*
>* See efi_{reserve,free}_boot_services().
>*/
> if (md->type == EFI_BOOT_SERVICES_CODE ||
> md->type == EFI_BOOT_SERVICES_DATA)
> return false;
>
> This workaround suppressed a boot crash, but potential issues still
> remain because no one prevents the regions from overlapping with kernel
> image by KASLR.
>
> So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
> chosen as kernel memory for the workaround to work fine.
>
> Signed-off-by: Naoya Horiguchi
> ---
> arch/x86/boot/compressed/kaslr.c | 41
> +++-
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c
> b/arch/x86/boot/compressed/kaslr.c
> index 94f08fd375ae..f43fed0441a6 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
> -static void process_efi_entry(unsigned long minimum, unsigned long
> image_size)
> +/* Returns true if we really enter efi memmap walk, otherwise returns false.
> */
> +static bool process_efi_entry(unsigned long minimum, unsigned long
> image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
> @@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
> unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> - return;
> + return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
> - return;
> + return false;
> }
> pmap = e->efi_memmap;
> #else
> @@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
> unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> - if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> - region.start = md->phys_addr;
> - region.size = md->num_pages << EFI_PAGE_SHIFT;
> - process_mem_region(®ion, minimum, image_size);
> + if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;
> + }
> +
> + for (i = 0; i < nr_desc; i++) {
> + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> +
> + /*
> + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
> + * services regions could be accessed after ExitBootServices()
> + * due to the workaround for buggy firmware.
> + */
> + if (!(md->type == EFI_LOADER_CODE ||
> + md->type == EFI_LOADER_DATA ||
> + md->type == EFI_CONVENTIONAL_MEMORY))
> + continue;
Wouldn't it make more sense to *only* use EFI_CONVENTIONAL_MEMORY?
You can't re-use EFI_LOADER_* regions because the kaslr code is run so
early in boot that you've no idea if data the kernel will need is in
those EFI_LOADER_* regions.
For example, we pass struct setup_data objects inside of
EFI_LOADER_DATA regions.
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 06:04:46PM +0800, Chao Fan wrote:
>On Thu, Jul 06, 2017 at 08:31:07AM +, Naoya Horiguchi wrote:
>>Hi Baoquan, everyone,
>>
>>I'm also interested in KASLR/EFI related issue (but not the same issue
>>with yours, so I separated the thread.)
>>
>>This patch is based on Baoquan's recent patches[1], adding more code
>>on the new function process_efi_entry().
>>If it's OK, could you queue this onto your tree/series?
>>
>>[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
>>https://lkml.org/lkml/2017/7/5/98
>>
>>Thanks,
>>Naoya Horiguchi
>>---
>>From: Naoya Horiguchi
>>Date: Thu, 6 Jul 2017 16:40:52 +0900
>>Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
>> KASLR's choice
>>
>>KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>>e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>>EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>>UEFI spec, all memory regions marked as EfiBootServicesCode and
>>EfiBootServicesData are available for free memory after the first call
>>of ExitBootServices(). So such regions should be usable for kernel on
>>spec basis.
>>
>>In x86, however, we have some workaround for broken firmware, where we
>>keep such regions reserved until SetVirtualAddressMap() is done.
>>See the following code in should_map_region():
>>
>> static bool should_map_region(efi_memory_desc_t *md)
>> {
>> ...
>> /*
>> * Map boot services regions as a workaround for buggy
>> * firmware that accesses them even when they shouldn't.
>> *
>> * See efi_{reserve,free}_boot_services().
>> */
>> if (md->type == EFI_BOOT_SERVICES_CODE ||
>> md->type == EFI_BOOT_SERVICES_DATA)
>> return false;
>>
>>This workaround suppressed a boot crash, but potential issues still
>>remain because no one prevents the regions from overlapping with kernel
>>image by KASLR.
>>
>>So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>>chosen as kernel memory for the workaround to work fine.
>>
>>Signed-off-by: Naoya Horiguchi
>>---
>> arch/x86/boot/compressed/kaslr.c | 41
>> +++-
>> 1 file changed, 32 insertions(+), 9 deletions(-)
>>
>>diff --git a/arch/x86/boot/compressed/kaslr.c
>>b/arch/x86/boot/compressed/kaslr.c
>>index 94f08fd375ae..f43fed0441a6 100644
>>--- a/arch/x86/boot/compressed/kaslr.c
>>+++ b/arch/x86/boot/compressed/kaslr.c
>>@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
>> /* Marks if efi mirror regions have been found and handled. */
>> static bool efi_mirror_found;
>>
>>-static void process_efi_entry(unsigned long minimum, unsigned long
>>image_size)
>>+/* Returns true if we really enter efi memmap walk, otherwise returns false.
>>*/
>>+static bool process_efi_entry(unsigned long minimum, unsigned long
>>image_size)
>> {
>> struct efi_info *e = &boot_params->efi_info;
>> struct mem_vector region;
>>@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
>>unsigned long image_size)
>> signature = (char *)&boot_params->efi_info.efi_loader_signature;
>> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>>- return;
>>+ return false;
>>
>> #ifdef CONFIG_X86_32
>> /* Can't handle data above 4GB at this time */
>> if (e->efi_memmap_hi) {
>> warn("Memory map is above 4GB, EFI should be disabled.\n");
>>- return;
>>+ return false;
>> }
>> pmap = e->efi_memmap;
>> #else
>>@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
>>unsigned long image_size)
>> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>> for (i = 0; i < nr_desc; i++) {
>> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>>- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>>- region.start = md->phys_addr;
>>- region.size = md->num_pages << EFI_PAGE_SHIFT;
>>- process_mem_region(®ion, minimum, image_size);
>>+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>> efi_mirror_found = true;
>>+ }
>
>Hi Horiguchi-san,
>
>Sorry for one more suggestion,
>How about add:
> if (!efi_mirror_found)
> return false;
>at this place, between the two cycles.
>
>Because if there are no mirror regions found, I think we can return
>directly and go to walk the e820 entries.
>I don't know whether my understanding is right.
Sorry for disturbing, it's my misunderstanding.
My suggestion is useless.
Since efi entries have been walked, we can use them directly,
no need to walk e820 entries again.
The logic of your codes is good.
Sorry
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 08:31:07AM +, Naoya Horiguchi wrote:
>Hi Baoquan, everyone,
>
>I'm also interested in KASLR/EFI related issue (but not the same issue
>with yours, so I separated the thread.)
>
>This patch is based on Baoquan's recent patches[1], adding more code
>on the new function process_efi_entry().
>If it's OK, could you queue this onto your tree/series?
>
>[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
>https://lkml.org/lkml/2017/7/5/98
>
>Thanks,
>Naoya Horiguchi
>---
>From: Naoya Horiguchi
>Date: Thu, 6 Jul 2017 16:40:52 +0900
>Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> KASLR's choice
>
>KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>UEFI spec, all memory regions marked as EfiBootServicesCode and
>EfiBootServicesData are available for free memory after the first call
>of ExitBootServices(). So such regions should be usable for kernel on
>spec basis.
>
>In x86, however, we have some workaround for broken firmware, where we
>keep such regions reserved until SetVirtualAddressMap() is done.
>See the following code in should_map_region():
>
> static bool should_map_region(efi_memory_desc_t *md)
> {
> ...
> /*
>* Map boot services regions as a workaround for buggy
>* firmware that accesses them even when they shouldn't.
>*
>* See efi_{reserve,free}_boot_services().
>*/
> if (md->type == EFI_BOOT_SERVICES_CODE ||
> md->type == EFI_BOOT_SERVICES_DATA)
> return false;
>
>This workaround suppressed a boot crash, but potential issues still
>remain because no one prevents the regions from overlapping with kernel
>image by KASLR.
>
>So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>chosen as kernel memory for the workaround to work fine.
>
>Signed-off-by: Naoya Horiguchi
>---
> arch/x86/boot/compressed/kaslr.c | 41 +++-
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c
>b/arch/x86/boot/compressed/kaslr.c
>index 94f08fd375ae..f43fed0441a6 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
>-static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>+/* Returns true if we really enter efi memmap walk, otherwise returns false.
>*/
>+static bool process_efi_entry(unsigned long minimum, unsigned long image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
>@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
>unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>- return;
>+ return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
>- return;
>+ return false;
> }
> pmap = e->efi_memmap;
> #else
>@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
>unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>- region.start = md->phys_addr;
>- region.size = md->num_pages << EFI_PAGE_SHIFT;
>- process_mem_region(®ion, minimum, image_size);
>+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;
>+ }
Hi Horiguchi-san,
Sorry for one more suggestion,
How about add:
if (!efi_mirror_found)
return false;
at this place, between the two cycles.
Because if there are no mirror regions found, I think we can return
directly and go to walk the e820 entries.
I don't know whether my understanding is right.
Thanks,
Chao Fan
>+
>+ for (i = 0; i < nr_desc; i++) {
>+ md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>+
>+ /*
>+ * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
>+ * services regions could be accessed after ExitBootServices()
>+ * due to the workaround for
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 09:22:38AM +, Naoya Horiguchi wrote:
>On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote:
>> On Thu, Jul 06, 2017 at 08:31:07AM +, Naoya Horiguchi wrote:
>> >Hi Baoquan, everyone,
>> >
>> >I'm also interested in KASLR/EFI related issue (but not the same issue
>> >with yours, so I separated the thread.)
>> >
>> >This patch is based on Baoquan's recent patches[1], adding more code
>> >on the new function process_efi_entry().
>> >If it's OK, could you queue this onto your tree/series?
>> >
>> >[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
>> >https://lkml.org/lkml/2017/7/5/98
>> >
>> >Thanks,
>> >Naoya Horiguchi
>> >---
>> >From: Naoya Horiguchi
>> >Date: Thu, 6 Jul 2017 16:40:52 +0900
>> >Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
>> > KASLR's choice
>> >
>> >KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>> >e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>> >EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>> >UEFI spec, all memory regions marked as EfiBootServicesCode and
>> >EfiBootServicesData are available for free memory after the first call
>> >of ExitBootServices(). So such regions should be usable for kernel on
>> >spec basis.
>> >
>> >In x86, however, we have some workaround for broken firmware, where we
>> >keep such regions reserved until SetVirtualAddressMap() is done.
>> >See the following code in should_map_region():
>> >
>> >static bool should_map_region(efi_memory_desc_t *md)
>> >{
>> >...
>> >/*
>> > * Map boot services regions as a workaround for buggy
>> > * firmware that accesses them even when they shouldn't.
>> > *
>> > * See efi_{reserve,free}_boot_services().
>> > */
>> >if (md->type == EFI_BOOT_SERVICES_CODE ||
>> >md->type == EFI_BOOT_SERVICES_DATA)
>> >return false;
>> >
>> >This workaround suppressed a boot crash, but potential issues still
>> >remain because no one prevents the regions from overlapping with kernel
>> >image by KASLR.
>> >
>> >So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>> >chosen as kernel memory for the workaround to work fine.
>> >
>> >Signed-off-by: Naoya Horiguchi
>> >---
>> > arch/x86/boot/compressed/kaslr.c | 41
>> > +++-
>> > 1 file changed, 32 insertions(+), 9 deletions(-)
>> >
>> >diff --git a/arch/x86/boot/compressed/kaslr.c
>> >b/arch/x86/boot/compressed/kaslr.c
>> >index 94f08fd375ae..f43fed0441a6 100644
>> >--- a/arch/x86/boot/compressed/kaslr.c
>> >+++ b/arch/x86/boot/compressed/kaslr.c
>> >@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
>> > /* Marks if efi mirror regions have been found and handled. */
>> > static bool efi_mirror_found;
>> >
>> >-static void process_efi_entry(unsigned long minimum, unsigned long
>> >image_size)
>> >+/* Returns true if we really enter efi memmap walk, otherwise returns
>> >false. */
>> >+static bool process_efi_entry(unsigned long minimum, unsigned long
>> >image_size)
>> > {
>> >struct efi_info *e = &boot_params->efi_info;
>> >struct mem_vector region;
>> >@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
>> >unsigned long image_size)
>> >signature = (char *)&boot_params->efi_info.efi_loader_signature;
>> >if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>> >strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>> >- return;
>> >+ return false;
>> >
>> > #ifdef CONFIG_X86_32
>> >/* Can't handle data above 4GB at this time */
>> >if (e->efi_memmap_hi) {
>> >warn("Memory map is above 4GB, EFI should be disabled.\n");
>> >- return;
>> >+ return false;
>> >}
>> >pmap = e->efi_memmap;
>> > #else
>> >@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
>> >unsigned long image_size)
>> >nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>> >for (i = 0; i < nr_desc; i++) {
>> >md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>> >- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> >- region.start = md->phys_addr;
>> >- region.size = md->num_pages << EFI_PAGE_SHIFT;
>> >- process_mem_region(®ion, minimum, image_size);
>> >+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>> >efi_mirror_found = true;
>>
>> Hi Horiguchi-san,
>>
>> If efi_mirror_found is changed to be true, we won't need to walk other
>> entries, so I think:
>> if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> efi_mirror_found = true;
>> break;
>> }
>> will be enough to show that mirror regions exist. And will wa
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 05:18:09PM +0800, Baoquan He wrote:
> Hi Naoya Horiguchi,
>
> Thanks for making this!
>
> On 07/06/17 at 08:31am, Naoya Horiguchi wrote:
> > Hi Baoquan, everyone,
> >
> > I'm also interested in KASLR/EFI related issue (but not the same issue
> > with yours, so I separated the thread.)
> >
> > This patch is based on Baoquan's recent patches[1], adding more code
> > on the new function process_efi_entry().
> > If it's OK, could you queue this onto your tree/series?
>
> This is interesting. So you are suggesting that we should try to avoid
> those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map
> regions are available, meanwhile try to locate kernel inside mirrored
> regions if existed. I do know the efi work around, so it seems reasonable
> to me, I can add it when repost. Or you can post after mine has been
> merged.
Thank you for the positive response, Baoquan.
>
> A little adjustment, please see the inline comment.
> >
> > [1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
> > https://lkml.org/lkml/2017/7/5/98
> >
> > Thanks,
> > Naoya Horiguchi
> > ---
> > From: Naoya Horiguchi
> > Date: Thu, 6 Jul 2017 16:40:52 +0900
> > Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> > KASLR's choice
> >
> > KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
> > e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
> > EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
> > UEFI spec, all memory regions marked as EfiBootServicesCode and
> > EfiBootServicesData are available for free memory after the first call
> > of ExitBootServices(). So such regions should be usable for kernel on
> > spec basis.
> >
> > In x86, however, we have some workaround for broken firmware, where we
> > keep such regions reserved until SetVirtualAddressMap() is done.
> > See the following code in should_map_region():
> >
> > static bool should_map_region(efi_memory_desc_t *md)
> > {
> > ...
> > /*
> > * Map boot services regions as a workaround for buggy
> > * firmware that accesses them even when they shouldn't.
> > *
> > * See efi_{reserve,free}_boot_services().
> > */
> > if (md->type == EFI_BOOT_SERVICES_CODE ||
> > md->type == EFI_BOOT_SERVICES_DATA)
> > return false;
> >
> > This workaround suppressed a boot crash, but potential issues still
> > remain because no one prevents the regions from overlapping with kernel
> > image by KASLR.
> >
> > So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
> > chosen as kernel memory for the workaround to work fine.
> >
> > Signed-off-by: Naoya Horiguchi
> > ---
> > arch/x86/boot/compressed/kaslr.c | 41
> > +++-
> > 1 file changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/kaslr.c
> > b/arch/x86/boot/compressed/kaslr.c
> > index 94f08fd375ae..f43fed0441a6 100644
> > --- a/arch/x86/boot/compressed/kaslr.c
> > +++ b/arch/x86/boot/compressed/kaslr.c
> > @@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> > /* Marks if efi mirror regions have been found and handled. */
> > static bool efi_mirror_found;
> >
> > -static void process_efi_entry(unsigned long minimum, unsigned long
> > image_size)
> > +/* Returns true if we really enter efi memmap walk, otherwise returns
> > false. */
> > +static bool process_efi_entry(unsigned long minimum, unsigned long
> > image_size)
> > {
> > struct efi_info *e = &boot_params->efi_info;
> > struct mem_vector region;
> > @@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
> > unsigned long image_size)
> > signature = (char *)&boot_params->efi_info.efi_loader_signature;
> > if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> > strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> > - return;
> > + return false;
> >
> > #ifdef CONFIG_X86_32
> > /* Can't handle data above 4GB at this time */
> > if (e->efi_memmap_hi) {
> > warn("Memory map is above 4GB, EFI should be disabled.\n");
> > - return;
> > + return false;
> > }
> > pmap = e->efi_memmap;
> > #else
> > @@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
> > unsigned long image_size)
> > nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> > for (i = 0; i < nr_desc; i++) {
> > md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> > - if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> > - region.start = md->phys_addr;
> > - region.size = md->num_pages << EFI_PAGE_SHIFT;
> > - process_mem_region(®ion, minimum, image_size);
> > +
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote:
> On Thu, Jul 06, 2017 at 08:31:07AM +, Naoya Horiguchi wrote:
> >Hi Baoquan, everyone,
> >
> >I'm also interested in KASLR/EFI related issue (but not the same issue
> >with yours, so I separated the thread.)
> >
> >This patch is based on Baoquan's recent patches[1], adding more code
> >on the new function process_efi_entry().
> >If it's OK, could you queue this onto your tree/series?
> >
> >[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
> >https://lkml.org/lkml/2017/7/5/98
> >
> >Thanks,
> >Naoya Horiguchi
> >---
> >From: Naoya Horiguchi
> >Date: Thu, 6 Jul 2017 16:40:52 +0900
> >Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> > KASLR's choice
> >
> >KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
> >e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
> >EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
> >UEFI spec, all memory regions marked as EfiBootServicesCode and
> >EfiBootServicesData are available for free memory after the first call
> >of ExitBootServices(). So such regions should be usable for kernel on
> >spec basis.
> >
> >In x86, however, we have some workaround for broken firmware, where we
> >keep such regions reserved until SetVirtualAddressMap() is done.
> >See the following code in should_map_region():
> >
> > static bool should_map_region(efi_memory_desc_t *md)
> > {
> > ...
> > /*
> > * Map boot services regions as a workaround for buggy
> > * firmware that accesses them even when they shouldn't.
> > *
> > * See efi_{reserve,free}_boot_services().
> > */
> > if (md->type == EFI_BOOT_SERVICES_CODE ||
> > md->type == EFI_BOOT_SERVICES_DATA)
> > return false;
> >
> >This workaround suppressed a boot crash, but potential issues still
> >remain because no one prevents the regions from overlapping with kernel
> >image by KASLR.
> >
> >So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
> >chosen as kernel memory for the workaround to work fine.
> >
> >Signed-off-by: Naoya Horiguchi
> >---
> > arch/x86/boot/compressed/kaslr.c | 41
> > +++-
> > 1 file changed, 32 insertions(+), 9 deletions(-)
> >
> >diff --git a/arch/x86/boot/compressed/kaslr.c
> >b/arch/x86/boot/compressed/kaslr.c
> >index 94f08fd375ae..f43fed0441a6 100644
> >--- a/arch/x86/boot/compressed/kaslr.c
> >+++ b/arch/x86/boot/compressed/kaslr.c
> >@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> > /* Marks if efi mirror regions have been found and handled. */
> > static bool efi_mirror_found;
> >
> >-static void process_efi_entry(unsigned long minimum, unsigned long
> >image_size)
> >+/* Returns true if we really enter efi memmap walk, otherwise returns
> >false. */
> >+static bool process_efi_entry(unsigned long minimum, unsigned long
> >image_size)
> > {
> > struct efi_info *e = &boot_params->efi_info;
> > struct mem_vector region;
> >@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
> >unsigned long image_size)
> > signature = (char *)&boot_params->efi_info.efi_loader_signature;
> > if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> > strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> >-return;
> >+return false;
> >
> > #ifdef CONFIG_X86_32
> > /* Can't handle data above 4GB at this time */
> > if (e->efi_memmap_hi) {
> > warn("Memory map is above 4GB, EFI should be disabled.\n");
> >-return;
> >+return false;
> > }
> > pmap = e->efi_memmap;
> > #else
> >@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
> >unsigned long image_size)
> > nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> > for (i = 0; i < nr_desc; i++) {
> > md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> >-if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> >-region.start = md->phys_addr;
> >-region.size = md->num_pages << EFI_PAGE_SHIFT;
> >-process_mem_region(®ion, minimum, image_size);
> >+if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> > efi_mirror_found = true;
>
> Hi Horiguchi-san,
>
> If efi_mirror_found is changed to be true, we won't need to walk other
> entries, so I think:
> if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> efi_mirror_found = true;
> break;
> }
> will be enough to show that mirror regions exist. And will walk
> less entries. How do you think about this?
Thank you for the review, Chao.
And you're right, I'll add break here.
# I'll post revised o
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
Hi Naoya Horiguchi,
Thanks for making this!
On 07/06/17 at 08:31am, Naoya Horiguchi wrote:
> Hi Baoquan, everyone,
>
> I'm also interested in KASLR/EFI related issue (but not the same issue
> with yours, so I separated the thread.)
>
> This patch is based on Baoquan's recent patches[1], adding more code
> on the new function process_efi_entry().
> If it's OK, could you queue this onto your tree/series?
This is interesting. So you are suggesting that we should try to avoid
those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map
regions are available, meanwhile try to locate kernel inside mirrored
regions if existed. I do know the efi work around, so it seems reasonable
to me, I can add it when repost. Or you can post after mine has been
merged.
A little adjustment, please see the inline comment.
>
> [1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
> https://lkml.org/lkml/2017/7/5/98
>
> Thanks,
> Naoya Horiguchi
> ---
> From: Naoya Horiguchi
> Date: Thu, 6 Jul 2017 16:40:52 +0900
> Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> KASLR's choice
>
> KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
> e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
> EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
> UEFI spec, all memory regions marked as EfiBootServicesCode and
> EfiBootServicesData are available for free memory after the first call
> of ExitBootServices(). So such regions should be usable for kernel on
> spec basis.
>
> In x86, however, we have some workaround for broken firmware, where we
> keep such regions reserved until SetVirtualAddressMap() is done.
> See the following code in should_map_region():
>
> static bool should_map_region(efi_memory_desc_t *md)
> {
> ...
> /*
>* Map boot services regions as a workaround for buggy
>* firmware that accesses them even when they shouldn't.
>*
>* See efi_{reserve,free}_boot_services().
>*/
> if (md->type == EFI_BOOT_SERVICES_CODE ||
> md->type == EFI_BOOT_SERVICES_DATA)
> return false;
>
> This workaround suppressed a boot crash, but potential issues still
> remain because no one prevents the regions from overlapping with kernel
> image by KASLR.
>
> So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
> chosen as kernel memory for the workaround to work fine.
>
> Signed-off-by: Naoya Horiguchi
> ---
> arch/x86/boot/compressed/kaslr.c | 41
> +++-
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c
> b/arch/x86/boot/compressed/kaslr.c
> index 94f08fd375ae..f43fed0441a6 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
> -static void process_efi_entry(unsigned long minimum, unsigned long
> image_size)
> +/* Returns true if we really enter efi memmap walk, otherwise returns false.
> */
> +static bool process_efi_entry(unsigned long minimum, unsigned long
> image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
> @@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
> unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> - return;
> + return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
> - return;
> + return false;
> }
> pmap = e->efi_memmap;
> #else
> @@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
> unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
> - if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
> - region.start = md->phys_addr;
> - region.size = md->num_pages << EFI_PAGE_SHIFT;
> - process_mem_region(®ion, minimum, image_size);
> + if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;
Here, we should define a local variable of bool type to mark if mirrored
region is found.
> + }
> +
> + for (i = 0; i < nr_desc; i++) {
> + md = (efi_memory
Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
On Thu, Jul 06, 2017 at 08:31:07AM +, Naoya Horiguchi wrote:
>Hi Baoquan, everyone,
>
>I'm also interested in KASLR/EFI related issue (but not the same issue
>with yours, so I separated the thread.)
>
>This patch is based on Baoquan's recent patches[1], adding more code
>on the new function process_efi_entry().
>If it's OK, could you queue this onto your tree/series?
>
>[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
>https://lkml.org/lkml/2017/7/5/98
>
>Thanks,
>Naoya Horiguchi
>---
>From: Naoya Horiguchi
>Date: Thu, 6 Jul 2017 16:40:52 +0900
>Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
> KASLR's choice
>
>KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>UEFI spec, all memory regions marked as EfiBootServicesCode and
>EfiBootServicesData are available for free memory after the first call
>of ExitBootServices(). So such regions should be usable for kernel on
>spec basis.
>
>In x86, however, we have some workaround for broken firmware, where we
>keep such regions reserved until SetVirtualAddressMap() is done.
>See the following code in should_map_region():
>
> static bool should_map_region(efi_memory_desc_t *md)
> {
> ...
> /*
>* Map boot services regions as a workaround for buggy
>* firmware that accesses them even when they shouldn't.
>*
>* See efi_{reserve,free}_boot_services().
>*/
> if (md->type == EFI_BOOT_SERVICES_CODE ||
> md->type == EFI_BOOT_SERVICES_DATA)
> return false;
>
>This workaround suppressed a boot crash, but potential issues still
>remain because no one prevents the regions from overlapping with kernel
>image by KASLR.
>
>So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>chosen as kernel memory for the workaround to work fine.
>
>Signed-off-by: Naoya Horiguchi
>---
> arch/x86/boot/compressed/kaslr.c | 41 +++-
> 1 file changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/arch/x86/boot/compressed/kaslr.c
>b/arch/x86/boot/compressed/kaslr.c
>index 94f08fd375ae..f43fed0441a6 100644
>--- a/arch/x86/boot/compressed/kaslr.c
>+++ b/arch/x86/boot/compressed/kaslr.c
>@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
> /* Marks if efi mirror regions have been found and handled. */
> static bool efi_mirror_found;
>
>-static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>+/* Returns true if we really enter efi memmap walk, otherwise returns false.
>*/
>+static bool process_efi_entry(unsigned long minimum, unsigned long image_size)
> {
> struct efi_info *e = &boot_params->efi_info;
> struct mem_vector region;
>@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum,
>unsigned long image_size)
> signature = (char *)&boot_params->efi_info.efi_loader_signature;
> if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>- return;
>+ return false;
>
> #ifdef CONFIG_X86_32
> /* Can't handle data above 4GB at this time */
> if (e->efi_memmap_hi) {
> warn("Memory map is above 4GB, EFI should be disabled.\n");
>- return;
>+ return false;
> }
> pmap = e->efi_memmap;
> #else
>@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum,
>unsigned long image_size)
> nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> for (i = 0; i < nr_desc; i++) {
> md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>- region.start = md->phys_addr;
>- region.size = md->num_pages << EFI_PAGE_SHIFT;
>- process_mem_region(®ion, minimum, image_size);
>+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
> efi_mirror_found = true;
Hi Horiguchi-san,
If efi_mirror_found is changed to be true, we won't need to walk other
entries, so I think:
if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
efi_mirror_found = true;
break;
}
will be enough to show that mirror regions exist. And will walk
less entries. How do you think about this?
Another question: what's the benifit of putting this part of
"efi_mirror_found = true" to a independent cycle.
Thanks,
Chao Fan
>+ }
>+
>+ for (i = 0; i < nr_desc; i++) {
>+ md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>+
>+ /*
>+ * EFI_BOOT_S
