[PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Ard Biesheuvel]

kaslr_early_init() is called with the kernel mapped at its
link time offset, and if it returns with a non-zero offset,
the kernel is unmapped and remapped again at the randomized
offset.

During its execution, kaslr_early_init() also randomizes the
base of the module region and of the linear mapping of DRAM,
and sets two variables accordingly. However, since these
variables are assigned with the caches on, they may get lost
during the cache maintenance that occurs when unmapping and
remapping the kernel, so ensure that these values are cleaned
to the PoC.

Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
Cc: <stable@vger.kernel.org> # v4.6+
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/kaslr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c
index f0e6ab8abe9c..ba6b41790fcd 100644
--- a/arch/arm64/kernel/kaslr.c
+++ b/arch/arm64/kernel/kaslr.c
@@ -14,6 +14,7 @@
 #include <linux/sched.h>
 #include <linux/types.h>
 
+#include <asm/cacheflush.h>
 #include <asm/fixmap.h>
 #include <asm/kernel-pgtable.h>
 #include <asm/memory.h>
@@ -43,7 +44,7 @@ static __init u64 get_kaslr_seed(void *fdt)
 	return ret;
 }
 
-static __init const u8 *get_cmdline(void *fdt)
+static __init const u8 *kaslr_get_cmdline(void *fdt)
 {
 	static __initconst const u8 default_cmdline[] = CONFIG_CMDLINE;
 
@@ -109,7 +110,7 @@ u64 __init kaslr_early_init(u64 dt_phys)
 	 * Check if 'nokaslr' appears on the command line, and
 	 * return 0 if that is the case.
 	 */
-	cmdline = get_cmdline(fdt);
+	cmdline = kaslr_get_cmdline(fdt);
 	str = strstr(cmdline, "nokaslr");
 	if (str == cmdline || (str > cmdline && *(str - 1) == ' '))
 		return 0;
@@ -169,5 +170,8 @@ u64 __init kaslr_early_init(u64 dt_phys)
 	module_alloc_base += (module_range * (seed & ((1 << 21) - 1))) >> 21;
 	module_alloc_base &= PAGE_MASK;
 
+	__flush_dcache_area(&module_alloc_base, sizeof(module_alloc_base));
+	__flush_dcache_area(&memstart_offset_seed, sizeof(memstart_offset_seed));
+
 	return offset;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Ard Biesheuvel]

On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> kaslr_early_init() is called with the kernel mapped at its
> link time offset, and if it returns with a non-zero offset,
> the kernel is unmapped and remapped again at the randomized
> offset.
>
> During its execution, kaslr_early_init() also randomizes the
> base of the module region and of the linear mapping of DRAM,
> and sets two variables accordingly. However, since these
> variables are assigned with the caches on, they may get lost
> during the cache maintenance that occurs when unmapping and
> remapping the kernel, so ensure that these values are cleaned
> to the PoC.
>
> Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> Cc: <stable@vger.kernel.org> # v4.6+
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

On an affected kernel build, I see the following variables sharing the
same 64-byte cacheline

ffff00001100bd18 R module_alloc_base
ffff00001100bd20 R memstart_addr
ffff00001100bd28 R arm64_dma_phys_limit
ffff00001100bd30 R kimage_voffset
ffff00001100bd38 R vabits_user

and we now have the following code in head.S (line 330)

adr_l x6, vabits_user
str x5, [x6]
dmb sy
dc ivac, x6 // Invalidate potentially stale cache line

which is invoked after kaslr_early_init() returns.


> ---
>  arch/arm64/kernel/kaslr.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/kaslr.c b/arch/arm64/kernel/kaslr.c
> index f0e6ab8abe9c..ba6b41790fcd 100644
> --- a/arch/arm64/kernel/kaslr.c
> +++ b/arch/arm64/kernel/kaslr.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched.h>
>  #include <linux/types.h>
>
> +#include <asm/cacheflush.h>
>  #include <asm/fixmap.h>
>  #include <asm/kernel-pgtable.h>
>  #include <asm/memory.h>
> @@ -43,7 +44,7 @@ static __init u64 get_kaslr_seed(void *fdt)
>         return ret;
>  }
>
> -static __init const u8 *get_cmdline(void *fdt)
> +static __init const u8 *kaslr_get_cmdline(void *fdt)
>  {
>         static __initconst const u8 default_cmdline[] = CONFIG_CMDLINE;
>
> @@ -109,7 +110,7 @@ u64 __init kaslr_early_init(u64 dt_phys)
>          * Check if 'nokaslr' appears on the command line, and
>          * return 0 if that is the case.
>          */
> -       cmdline = get_cmdline(fdt);
> +       cmdline = kaslr_get_cmdline(fdt);
>         str = strstr(cmdline, "nokaslr");
>         if (str == cmdline || (str > cmdline && *(str - 1) == ' '))
>                 return 0;
> @@ -169,5 +170,8 @@ u64 __init kaslr_early_init(u64 dt_phys)
>         module_alloc_base += (module_range * (seed & ((1 << 21) - 1))) >> 21;
>         module_alloc_base &= PAGE_MASK;
>
> +       __flush_dcache_area(&module_alloc_base, sizeof(module_alloc_base));
> +       __flush_dcache_area(&memstart_offset_seed, sizeof(memstart_offset_seed));
> +
>         return offset;
>  }
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Will Deacon]

Hi Ard,

On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > kaslr_early_init() is called with the kernel mapped at its
> > link time offset, and if it returns with a non-zero offset,
> > the kernel is unmapped and remapped again at the randomized
> > offset.
> >
> > During its execution, kaslr_early_init() also randomizes the
> > base of the module region and of the linear mapping of DRAM,
> > and sets two variables accordingly. However, since these
> > variables are assigned with the caches on, they may get lost
> > during the cache maintenance that occurs when unmapping and
> > remapping the kernel, so ensure that these values are cleaned
> > to the PoC.
> >
> > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > Cc: <stable@vger.kernel.org> # v4.6+
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> On an affected kernel build, I see the following variables sharing the
> same 64-byte cacheline
> 
> ffff00001100bd18 R module_alloc_base
> ffff00001100bd20 R memstart_addr
> ffff00001100bd28 R arm64_dma_phys_limit
> ffff00001100bd30 R kimage_voffset
> ffff00001100bd38 R vabits_user
> 
> and we now have the following code in head.S (line 330)
> 
> adr_l x6, vabits_user
> str x5, [x6]
> dmb sy
> dc ivac, x6 // Invalidate potentially stale cache line
> 
> which is invoked after kaslr_early_init() returns.

Urgh, this is hideous. Well done for debugging it! It would be nice to make
this less fragile in future, perhaps by annotating the variable declarations
and ensuring they're padded to the CWG etc so that we avoid any false
sharing and make it very obvious what's going to be invalidated. We already
have .mmuoff.data.write for data written with the MMU off.

I'm happy to take your patch to plug the regression, but I think we should
aim to remove the false sharing rather than handling it. Another alternative
might be to use clean+invalidate instead of invalidate in head.S, along the
lines of:

	DC CIVAC,	X0
	DMB		SY
	STR		X1, [X0]
	DMB		SY
	DC CIVAC,	X0

Can you foresee any issues with that?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Ard Biesheuvel]

On Wed, 16 Jan 2019 at 12:32, Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Ard,
>
> On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> > On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > kaslr_early_init() is called with the kernel mapped at its
> > > link time offset, and if it returns with a non-zero offset,
> > > the kernel is unmapped and remapped again at the randomized
> > > offset.
> > >
> > > During its execution, kaslr_early_init() also randomizes the
> > > base of the module region and of the linear mapping of DRAM,
> > > and sets two variables accordingly. However, since these
> > > variables are assigned with the caches on, they may get lost
> > > during the cache maintenance that occurs when unmapping and
> > > remapping the kernel, so ensure that these values are cleaned
> > > to the PoC.
> > >
> > > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > > Cc: <stable@vger.kernel.org> # v4.6+
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > On an affected kernel build, I see the following variables sharing the
> > same 64-byte cacheline
> >
> > ffff00001100bd18 R module_alloc_base
> > ffff00001100bd20 R memstart_addr
> > ffff00001100bd28 R arm64_dma_phys_limit
> > ffff00001100bd30 R kimage_voffset
> > ffff00001100bd38 R vabits_user
> >
> > and we now have the following code in head.S (line 330)
> >
> > adr_l x6, vabits_user
> > str x5, [x6]
> > dmb sy
> > dc ivac, x6 // Invalidate potentially stale cache line
> >
> > which is invoked after kaslr_early_init() returns.
>
> Urgh, this is hideous. Well done for debugging it! It would be nice to make
> this less fragile in future, perhaps by annotating the variable declarations
> and ensuring they're padded to the CWG etc so that we avoid any false
> sharing and make it very obvious what's going to be invalidated. We already
> have .mmuoff.data.write for data written with the MMU off.
>
> I'm happy to take your patch to plug the regression, but I think we should
> aim to remove the false sharing rather than handling it. Another alternative
> might be to use clean+invalidate instead of invalidate in head.S, along the
> lines of:
>
>         DC CIVAC,       X0
>         DMB             SY
>         STR             X1, [X0]
>         DMB             SY
>         DC CIVAC,       X0
>
> Can you foresee any issues with that?
>

In general, I think we should simply never invalidate ordinary
variables like that.

In this particular case, I don't think there is a need to set
vabits_user twice, so we can move that code to after the call to
__create_page_tables in stext() instead.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Ard Biesheuvel]

On Wed, 16 Jan 2019 at 12:49, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 16 Jan 2019 at 12:32, Will Deacon <will.deacon@arm.com> wrote:
> >
> > Hi Ard,
> >
> > On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> > > On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > kaslr_early_init() is called with the kernel mapped at its
> > > > link time offset, and if it returns with a non-zero offset,
> > > > the kernel is unmapped and remapped again at the randomized
> > > > offset.
> > > >
> > > > During its execution, kaslr_early_init() also randomizes the
> > > > base of the module region and of the linear mapping of DRAM,
> > > > and sets two variables accordingly. However, since these
> > > > variables are assigned with the caches on, they may get lost
> > > > during the cache maintenance that occurs when unmapping and
> > > > remapping the kernel, so ensure that these values are cleaned
> > > > to the PoC.
> > > >
> > > > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > > > Cc: <stable@vger.kernel.org> # v4.6+
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >
> > > On an affected kernel build, I see the following variables sharing the
> > > same 64-byte cacheline
> > >
> > > ffff00001100bd18 R module_alloc_base
> > > ffff00001100bd20 R memstart_addr
> > > ffff00001100bd28 R arm64_dma_phys_limit
> > > ffff00001100bd30 R kimage_voffset
> > > ffff00001100bd38 R vabits_user
> > >
> > > and we now have the following code in head.S (line 330)
> > >
> > > adr_l x6, vabits_user
> > > str x5, [x6]
> > > dmb sy
> > > dc ivac, x6 // Invalidate potentially stale cache line
> > >
> > > which is invoked after kaslr_early_init() returns.
> >
> > Urgh, this is hideous. Well done for debugging it! It would be nice to make
> > this less fragile in future, perhaps by annotating the variable declarations
> > and ensuring they're padded to the CWG etc so that we avoid any false
> > sharing and make it very obvious what's going to be invalidated. We already
> > have .mmuoff.data.write for data written with the MMU off.
> >
> > I'm happy to take your patch to plug the regression, but I think we should
> > aim to remove the false sharing rather than handling it. Another alternative
> > might be to use clean+invalidate instead of invalidate in head.S, along the
> > lines of:
> >
> >         DC CIVAC,       X0
> >         DMB             SY
> >         STR             X1, [X0]
> >         DMB             SY
> >         DC CIVAC,       X0
> >
> > Can you foresee any issues with that?
> >
>
> In general, I think we should simply never invalidate ordinary
> variables like that.
>
> In this particular case, I don't think there is a need to set
> vabits_user twice, so we can move that code to after the call to
> __create_page_tables in stext() instead.

I guess the same applies to idmap_t0sz, but we never hit that code path anymore.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Catalin Marinas]

On Wed, Jan 16, 2019 at 11:32:27AM +0000, Will Deacon wrote:
> On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> > On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > kaslr_early_init() is called with the kernel mapped at its
> > > link time offset, and if it returns with a non-zero offset,
> > > the kernel is unmapped and remapped again at the randomized
> > > offset.
> > >
> > > During its execution, kaslr_early_init() also randomizes the
> > > base of the module region and of the linear mapping of DRAM,
> > > and sets two variables accordingly. However, since these
> > > variables are assigned with the caches on, they may get lost
> > > during the cache maintenance that occurs when unmapping and
> > > remapping the kernel, so ensure that these values are cleaned
> > > to the PoC.
> > >
> > > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > > Cc: <stable@vger.kernel.org> # v4.6+
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > 
> > On an affected kernel build, I see the following variables sharing the
> > same 64-byte cacheline
> > 
> > ffff00001100bd18 R module_alloc_base
> > ffff00001100bd20 R memstart_addr
> > ffff00001100bd28 R arm64_dma_phys_limit
> > ffff00001100bd30 R kimage_voffset
> > ffff00001100bd38 R vabits_user
> > 
> > and we now have the following code in head.S (line 330)
> > 
> > adr_l x6, vabits_user
> > str x5, [x6]
> > dmb sy
> > dc ivac, x6 // Invalidate potentially stale cache line
> > 
> > which is invoked after kaslr_early_init() returns.
> 
> Urgh, this is hideous. Well done for debugging it! It would be nice to make
> this less fragile in future, perhaps by annotating the variable declarations
> and ensuring they're padded to the CWG etc so that we avoid any false
> sharing and make it very obvious what's going to be invalidated. We already
> have .mmuoff.data.write for data written with the MMU off.
> 
> I'm happy to take your patch to plug the regression, but I think we should
> aim to remove the false sharing rather than handling it. Another alternative
> might be to use clean+invalidate instead of invalidate in head.S, along the
> lines of:
> 
> 	DC CIVAC,	X0
> 	DMB		SY
> 	STR		X1, [X0]
> 	DMB		SY
> 	DC CIVAC,	X0
> 
> Can you foresee any issues with that?

The assumptions we've made in head.S were that any meaningful data has
already been written back by whoever wrote it (we don't care about clean
entries in the D-cache). Ard's patch fixes the code for this assumption.

I don't think changing head.S to CIVAC breaks anything since dirty lines
can be randomly evicted anyway. I'm more worried about no longer
detecting potential issues in things running prior to the kernel
(whether it's boot-loader, qemu).

FWIW, on Ard's patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: kaslr: ensure randomized quantities are clean to the PoC

[Will Deacon]

On Wed, Jan 16, 2019 at 11:58:05AM +0000, Catalin Marinas wrote:
> On Wed, Jan 16, 2019 at 11:32:27AM +0000, Will Deacon wrote:
> > On Tue, Jan 15, 2019 at 09:12:41PM +0100, Ard Biesheuvel wrote:
> > > On Tue, 15 Jan 2019 at 20:47, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > > > kaslr_early_init() is called with the kernel mapped at its
> > > > link time offset, and if it returns with a non-zero offset,
> > > > the kernel is unmapped and remapped again at the randomized
> > > > offset.
> > > >
> > > > During its execution, kaslr_early_init() also randomizes the
> > > > base of the module region and of the linear mapping of DRAM,
> > > > and sets two variables accordingly. However, since these
> > > > variables are assigned with the caches on, they may get lost
> > > > during the cache maintenance that occurs when unmapping and
> > > > remapping the kernel, so ensure that these values are cleaned
> > > > to the PoC.
> > > >
> > > > Fixes: f80fb3a3d508 ("arm64: add support for kernel ASLR")
> > > > Cc: <stable@vger.kernel.org> # v4.6+
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > 
> > > On an affected kernel build, I see the following variables sharing the
> > > same 64-byte cacheline
> > > 
> > > ffff00001100bd18 R module_alloc_base
> > > ffff00001100bd20 R memstart_addr
> > > ffff00001100bd28 R arm64_dma_phys_limit
> > > ffff00001100bd30 R kimage_voffset
> > > ffff00001100bd38 R vabits_user
> > > 
> > > and we now have the following code in head.S (line 330)
> > > 
> > > adr_l x6, vabits_user
> > > str x5, [x6]
> > > dmb sy
> > > dc ivac, x6 // Invalidate potentially stale cache line
> > > 
> > > which is invoked after kaslr_early_init() returns.
> > 
> > Urgh, this is hideous. Well done for debugging it! It would be nice to make
> > this less fragile in future, perhaps by annotating the variable declarations
> > and ensuring they're padded to the CWG etc so that we avoid any false
> > sharing and make it very obvious what's going to be invalidated. We already
> > have .mmuoff.data.write for data written with the MMU off.
> > 
> > I'm happy to take your patch to plug the regression, but I think we should
> > aim to remove the false sharing rather than handling it. Another alternative
> > might be to use clean+invalidate instead of invalidate in head.S, along the
> > lines of:
> > 
> > 	DC CIVAC,	X0
> > 	DMB		SY
> > 	STR		X1, [X0]
> > 	DMB		SY
> > 	DC CIVAC,	X0
> > 
> > Can you foresee any issues with that?
> 
> The assumptions we've made in head.S were that any meaningful data has
> already been written back by whoever wrote it (we don't care about clean
> entries in the D-cache). Ard's patch fixes the code for this assumption.
> 
> I don't think changing head.S to CIVAC breaks anything since dirty lines
> can be randomly evicted anyway. I'm more worried about no longer
> detecting potential issues in things running prior to the kernel
> (whether it's boot-loader, qemu).

Well, this code was never about detecting broken bootloaders :) The reason
I proposed using CIVAC explicitly is because it unifies the host and guest
behaviour. I failed to reproduce the module-loading crash which was
triggered by the issue because I was testing inside a guest.

> FWIW, on Ard's patch:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Cheers, I'll pick it up as a fix.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel