| Message ID | 20190115194707.11614-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Commit | 1598ecda7b239e9232dda032bfddeed9d89fab6c |
| Headers | show |
| Series | arm64: kaslr: ensure randomized quantities are clean to the PoC | expand |
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 >
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
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.
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.
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>
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
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; }
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(-)