[PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[Bryan O'Donoghue]

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@nexus-software.ie>
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.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

Re: [PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[Ingo Molnar]

* Bryan O'Donoghue <pure.logic@nexus-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@nexus-software.ie>
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.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);

Re: [PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[Bryan O'Donoghue]

On Thu, 2016-02-18 at 08:58 +0100, Ingo Molnar wrote:
> 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. 


Hi Ingo.

I agree - to flip the lock bit you need to be in ring-0 anyway.

> 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

That was the V1 of this patch

https://groups.google.com/forum/#!topic/linux.kernel/6ZuVOF3TJow

Andriy asked for the boot parameter to control the state of the IMR
lock bit, I'm just as happy to go back to that version TBH

Re: [PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[Ingo Molnar]

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

> On Thu, 2016-02-18 at 08:58 +0100, Ingo Molnar wrote:
> > 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. 
> 
> 
> Hi Ingo.
> 
> I agree - to flip the lock bit you need to be in ring-0 anyway.
> 
> > 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
> 
> That was the V1 of this patch
> 
> https://groups.google.com/forum/#!topic/linux.kernel/6ZuVOF3TJow

heh ;-)

> Andriy asked for the boot parameter to control the state of the IMR
> lock bit, I'm just as happy to go back to that version TBH

I really think it's over-engineered - especially considering that with the kernel 
lock-down removed there's no other IMR area that is really locked down - so we 
could get rid of the whole 'locked' logic that would simplify the code throughout.

Yeah, it's a nice looking hardware feature - but I don't think it's particularly 
useful in terms of extra protection.

Thanks,

	Ingo

Re: [PATCH] x86/intel/quark: Parameterize the kernel's IMR lock logic

[Bryan O'Donoghue]

On Thu, 2016-02-18 at 19:53 +0100, Ingo Molnar wrote:
> * Bryan O'Donoghue <pure.logic@nexus-software.ie> wrote:
> 
> > On Thu, 2016-02-18 at 08:58 +0100, Ingo Molnar wrote:
> > > 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. 
> > 
> > 
> > Hi Ingo.
> > 
> > I agree - to flip the lock bit you need to be in ring-0 anyway.
> > 
> > > 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
> > 
> > That was the V1 of this patch
> > 
> > https://groups.google.com/forum/#!topic/linux.kernel/6ZuVOF3TJow
> 
> heh ;-)

:)

> > Andriy asked for the boot parameter to control the state of the IMR
> > lock bit, I'm just as happy to go back to that version TBH
> 
> I really think it's over-engineered - especially considering that
> with the kernel 
> lock-down removed there's no other IMR area that is really locked
> down - so we 
> could get rid of the whole 'locked' logic that would simplify the
> code throughout.

I'm in favour of that too. Charitably I think locking a register like
this makes sense only when you talk about it in a meeting room
somewhere; as soon as you go to try to use it in a real situation you
find its far more trouble than its really worth.

So, I'm going to trim that out of this code unless I hear some pushback
from elsewhere in the 1/2 day or so.

> 
> Yeah, it's a nice looking hardware feature - but I don't think it's
> particularly 
> useful in terms of extra protection.
> 
> Thanks,
> 
> 	Ingo