* Bryan O'Donoghue <pure.logic@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> Reported-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> 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);
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
