All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matt Fleming <matt@codeblueprint.co.uk>
To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Baoquan He <bhe@redhat.com>, Kees Cook <keescook@chromium.org>,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@kernel.org>,
	izumi.taku@jp.fujitsu.com, Thomas Garnier <thgarnie@google.com>,
	fanc.fnst@cn.fujitsu.com,
	"Jun'ichi Nomura" <j-nomura@ce.jp.nec.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH v3 1/2] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
Date: Mon, 24 Jul 2017 14:17:07 +0100	[thread overview]
Message-ID: <20170724131707.GA11076@codeblueprint.co.uk> (raw)
In-Reply-To: <1499665896-23731-1-git-send-email-n-horiguchi@ah.jp.nec.com>

On Mon, 10 Jul, at 02:51:35PM, 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 <n-horiguchi@ah.jp.nec.com>
> ---
> v2 -> v3:
> - skip EFI_LOADER_CODE and EFI_LOADER_DATA in region scan
> 
> v1 -> v2:
> - switch efi_mirror_found to local variable
> - insert break when EFI_MEMORY_MORE_RELIABLE found
> ---
>  arch/x86/boot/compressed/kaslr.c | 44 +++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 12 deletions(-)

I think the patch content is fine, but please update the comments and
the changelog to say why EFI_CONVENTIONAL_MEMORY was chosen, i.e. that
it's the only memory type that we know to be free.

  reply	other threads:[~2017-07-24 13:17 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-06  8:31 [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Naoya Horiguchi
2017-07-06  9:13 ` Chao Fan
2017-07-06  9:22   ` Naoya Horiguchi
2017-07-06  9:36     ` Chao Fan
2017-07-06  9:18 ` Baoquan He
2017-07-06  9:36   ` Naoya Horiguchi
2017-07-06 10:04 ` Chao Fan
2017-07-06 10:20   ` Chao Fan
2017-07-06 14:57 ` Matt Fleming
2017-07-07  3:07   ` Baoquan He
2017-07-07  6:11     ` Naoya Horiguchi
2017-07-07 10:58       ` Matt Fleming
2017-07-10  5:47         ` Naoya Horiguchi
2017-07-10  5:51           ` [PATCH v3 1/2] " Naoya Horiguchi
2017-07-24 13:17             ` Matt Fleming [this message]
2017-07-25  6:17               ` Naoya Horiguchi
2017-07-10  5:51           ` [PATCH v3 2/2] x86/efi: clean up dead code around efi_reserve_boot_services() Naoya Horiguchi
2017-07-24 13:20             ` Matt Fleming
2017-07-26  0:12               ` Naoya Horiguchi
2017-07-26  1:13                 ` Baoquan He
2017-07-26  1:34                   ` Baoquan He
2017-07-28  6:48                     ` [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_* Naoya Horiguchi
2017-07-29 10:04                       ` kbuild test robot
2017-07-29 13:01                       ` kbuild test robot
2017-07-29 13:01                       ` [RFC PATCH] x86/boot: efi_kernel_boot_services_overlap can be static kbuild test robot
2017-08-23  8:24                       ` [PATCH] x86/boot: check overlap between kernel and EFI_BOOT_SERVICES_* Baoquan He
2017-07-07 10:56     ` [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice Matt Fleming
2017-07-09 10:44       ` Baoquan He
2017-07-09 14:27         ` Baoquan He
2017-07-07  7:22 ` [PATCH v2 1/2] " Naoya Horiguchi
2017-07-07  7:22 ` [PATCH v2 2/2] x86/efi: clean up dead code around efi_reserve_boot_services() Naoya Horiguchi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170724131707.GA11076@codeblueprint.co.uk \
    --to=matt@codeblueprint.co.uk \
    --cc=ard.biesheuvel@linaro.org \
    --cc=bhe@redhat.com \
    --cc=fanc.fnst@cn.fujitsu.com \
    --cc=hpa@zytor.com \
    --cc=izumi.taku@jp.fujitsu.com \
    --cc=j-nomura@ce.jp.nec.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=tglx@linutronix.de \
    --cc=thgarnie@google.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.