Message-ID: <20160218075810.GA16041@gmail.com>
Date:	Thu, 18 Feb 2016 08:58:10 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Bryan O'Donoghue <pure.logic@...us-software.ie>
Cc:	linux-kernel@...r.kernel.org, corbet@....net, tglx@...utronix.de,
	mingo@...hat.com, hpa@...or.com, x86@...nel.org,
	andriy.shevchenko@...ux.intel.com, boon.leong.ong@...el.com,
	fengguang.wu@...el.com, linux-doc@...r.kernel.org
Subject: Re: [PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic


* Bryan O'Donoghue <pure.logic@...us-software.ie> wrote:

> Currently when setting up an IMR around the kernel's .text area we lock
> that IMR, preventing further modification. While superficially this appears
> to be the right thing to do, in fact this doesn't account for a legitimate
> change in the memory map such as when executing a new kernel via kexec.
> 
> In such a scenario a second kernel can have a different size and location
> to it's predecessor and can view some of the memory occupied by it's
> predecessor as legitimately usable DMA RAM. If this RAM were then
> subsequently allocated to DMA agents within the system it could conceivably
> trigger an IMR violation.
> 
> This patch fixes the this potential situation by keeping the kernel's .text
> section IMR lock bit false by default, thus ensuring that a user of the
> system will have kexec just work without having to pass special parameters
> on the kernel command line to influence the state of the kernel's IMR. If a
> user so desires then it is possible to explicitly set the lock bit to true.
> 
> The new parameter is kernel_imr and this may be set to kernel_imr=locked or
> kernel_imr=unlocked.
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@...us-software.ie>
> Reported-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  Documentation/kernel-parameters.txt |  7 +++++++
>  arch/x86/platform/intel-quark/imr.c | 39 +++++++++++++++++++++++++++++++------
>  2 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 9a53c92..1aad1d2 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1359,6 +1359,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			        hardware thread id mappings.
>  				Format: <cpu>:<hwthread>
>  
> +	kernel_imr=	[X86, INTEL_IMR] Control the lock bit for the Isolated
> +			Memory Region protecting the kernel's .text section on
> +			X86 architectures that support IMRs such as Quark X1000.
> +			When an IMR lock bit is set it is not possible to unset
> +			it without a CPU reset.
> +			Format : {locked | unlocked (default) }
> +
>  	keep_bootcon	[KNL]
>  			Do not unregister boot console at start. This is only
>  			useful for debugging when something happens in the window
> diff --git a/arch/x86/platform/intel-quark/imr.c b/arch/x86/platform/intel-quark/imr.c
> index c61b6c3..8249f65 100644
> --- a/arch/x86/platform/intel-quark/imr.c
> +++ b/arch/x86/platform/intel-quark/imr.c
> @@ -37,6 +37,7 @@
>  struct imr_device {
>  	struct dentry	*file;
>  	bool		init;
> +	bool		kernel_imr_locked;
>  	struct mutex	lock;
>  	int		max_imr;
>  	int		reg_base;
> @@ -568,10 +569,15 @@ static inline int imr_clear(int reg)
>   * BIOS and Grub both setup IMRs around compressed kernel, initrd memory
>   * that need to be removed before the kernel hands out one of the IMR
>   * encased addresses to a downstream DMA agent such as the SD or Ethernet.
> + * Additionally if the current kernel is executing via kexec then we need to
> + * tear down the IMR the previous kernel had setup.
> + *
>   * IMRs on Galileo are setup to immediately reset the system on violation.
>   * As a result if you're running a root filesystem from SD - you'll need
>   * the boot-time IMRs torn down or you'll find seemingly random resets when
> - * using your filesystem.
> + * using your filesystem; similarly if you're doing a kexec boot of Linux then
> + * its required to sanitize the memory map with-respect to the previous IMR
> + * settings.
>   *
>   * @idev:	pointer to imr_device structure.
>   * @return:
> @@ -592,14 +598,20 @@ static void __init imr_fixup_memmap(struct imr_device *idev)
>  	end = (unsigned long)__end_rodata - 1;
>  
>  	/*
> -	 * Setup a locked IMR around the physical extent of the kernel
> -	 * from the beginning of the .text secton to the end of the
> -	 * .rodata section as one physically contiguous block.
> +	 * Setup an IMR around the physical extent of the kernel from the
> +	 * beginning of the .text section to the end of the .rodata section
> +	 * as one physically contiguous block.
>  	 *
>  	 * We don't round up @size since it is already PAGE_SIZE aligned.
>  	 * See vmlinux.lds.S for details.
> +	 *
> +	 * By default this IMR is unlocked to enable subsequent kernels booted
> +	 * by kexec to tear down the IMR we are setting up below. Its possible
> +	 * to set this IMR to the locked state by passing kernel_imr=locked on
> +	 * the kernel command line.
>  	 */
> -	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, true);
> +	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU,
> +			    imr_dev.kernel_imr_locked);
>  	if (ret < 0) {
>  		pr_err("unable to setup IMR for kernel: %zu KiB (%lx - %lx)\n",
>  			size / 1024, start, end);
> @@ -617,8 +629,23 @@ static const struct x86_cpu_id imr_ids[] __initconst = {
>  MODULE_DEVICE_TABLE(x86cpu, imr_ids);
>  
>  /**
> - * imr_init - entry point for IMR driver.
> + * imr_kernel_lock_setup - control the lock bit of the kernel's IMR
>   *
> + */
> +static int __init imr_kernel_lock_setup(char *str)
> +{
> +	if (!strcmp(str, "unlocked"))
> +		imr_dev.kernel_imr_locked = false;
> +	else if (!strcmp(str, "locked"))
> +		imr_dev.kernel_imr_locked = true;
> +	else
> +		return 0;
> +	return 1;
> +}
> +__setup("kernel_imr=", imr_kernel_lock_setup);
> +
> +/**
> + * imr_init - entry point for IMR driver.
>   * return: -ENODEV for no IMR support 0 if good to go.
>   */
>  static int __init imr_init(void)
> -- 
> 2.5.0

So why not simply do the patch below? Very few people use boot parameters, and the 
complexity does not seem to be worth it.

Furthermore I think an IMR range in itself is safe enough - it's not like such 
register state is going to be randomly corrupted, even with the 'lock' bit unset. 
So it's a perfectly fine protective measure against accidental memory corruption 
from the DMA space. It should not try to be more than that.

And once we do this, I suggest we get rid of the 'lock' parameter altogether - 
that will further simplify the code.

Thanks,

	Ingo

===============>

 arch/x86/platform/intel-quark/imr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/intel-quark/imr.c b/arch/x86/platform/intel-quark/imr.c
index 0a3736f03edc..f7c4f523910f 100644
--- a/arch/x86/platform/intel-quark/imr.c
+++ b/arch/x86/platform/intel-quark/imr.c
@@ -587,7 +587,7 @@ static void __init imr_fixup_memmap(struct imr_device *idev)
 	 * We don't round up @size since it is already PAGE_SIZE aligned.
 	 * See vmlinux.lds.S for details.
 	 */
-	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, true);
+	ret = imr_add_range(base, size, IMR_CPU, IMR_CPU, false);
 	if (ret < 0) {
 		pr_err("unable to setup IMR for kernel: %zu KiB (%lx - %lx)\n",
 			size / 1024, start, end);

Powered by blists - more mailing lists