[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Ard Biesheuvel]

When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
modules and the core kernel may exceed 4 GB, putting symbols exported
by the core kernel out of the reach of the ordinary adrp/add instruction
pairs used to generate relative symbol references. So make the adr_l
macro emit a movz/movk sequence instead when executing in module context.

While at it, remove the pointless special case for the stack pointer.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/assembler.h | 36 +++++++++++++++-----
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 446f6c46d4b1..3a4301163e04 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -164,22 +164,25 @@ lr	.req	x30		// link register
 
 /*
  * Pseudo-ops for PC-relative adr/ldr/str <reg>, <symbol> where
- * <symbol> is within the range +/- 4 GB of the PC.
+ * <symbol> is within the range +/- 4 GB of the PC when running
+ * in core kernel context. In module context, a movz/movk sequence
+ * is used, since modules may be loaded far away from the kernel
+ * when KASLR is in effect.
  */
 	/*
 	 * @dst: destination register (64 bit wide)
 	 * @sym: name of the symbol
-	 * @tmp: optional scratch register to be used if <dst> == sp, which
-	 *       is not allowed in an adrp instruction
 	 */
-	.macro	adr_l, dst, sym, tmp=
-	.ifb	\tmp
+	.macro	adr_l, dst, sym
+#ifndef MODULE
 	adrp	\dst, \sym
 	add	\dst, \dst, :lo12:\sym
-	.else
-	adrp	\tmp, \sym
-	add	\dst, \tmp, :lo12:\sym
-	.endif
+#else
+	movz	\dst, #:abs_g3:\sym
+	movk	\dst, #:abs_g2_nc:\sym
+	movk	\dst, #:abs_g1_nc:\sym
+	movk	\dst, #:abs_g0_nc:\sym
+#endif
 	.endm
 
 	/*
@@ -190,6 +193,7 @@ lr	.req	x30		// link register
 	 *       the address
 	 */
 	.macro	ldr_l, dst, sym, tmp=
+#ifndef MODULE
 	.ifb	\tmp
 	adrp	\dst, \sym
 	ldr	\dst, [\dst, :lo12:\sym]
@@ -197,6 +201,15 @@ lr	.req	x30		// link register
 	adrp	\tmp, \sym
 	ldr	\dst, [\tmp, :lo12:\sym]
 	.endif
+#else
+	.ifb	\tmp
+	adr_l	\dst, \sym
+	ldr	\dst, [\dst]
+	.else
+	adr_l	\tmp, \sym
+	ldr	\dst, [\tmp]
+	.endif
+#endif
 	.endm
 
 	/*
@@ -206,8 +219,13 @@ lr	.req	x30		// link register
 	 *       while <src> needs to be preserved.
 	 */
 	.macro	str_l, src, sym, tmp
+#ifndef MODULE
 	adrp	\tmp, \sym
 	str	\src, [\tmp, :lo12:\sym]
+#else
+	adr_l	\tmp, \sym
+	str	\src, [\tmp]
+#endif
 	.endm
 
 	/*
-- 
2.7.4

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Mark Rutland]

Hi Ard,

On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> modules and the core kernel may exceed 4 GB, putting symbols exported
> by the core kernel out of the reach of the ordinary adrp/add instruction
> pairs used to generate relative symbol references. So make the adr_l
> macro emit a movz/movk sequence instead when executing in module context.

AFAICT, we only use adr_l in a few assembly files that shouldn't matter
to modules:

* arch/arm64/kernel/head.S
* arch/arm64/kernel/sleep.S
* arch/arm64/kvm/hyp-init.S
* arch/arm64/kvm/hyp/hyp-entry.S

... so I don't follow why we need this.

Have I missed something? Or do you intend to use this in module code in
future?

It seems somewhat surprising to me to have adr_l expand to something
that doesn't use adr/adrp, but that's not necessarily a problem.

Thanks,
Mark.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Ard Biesheuvel]

On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
>> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
>> modules and the core kernel may exceed 4 GB, putting symbols exported
>> by the core kernel out of the reach of the ordinary adrp/add instruction
>> pairs used to generate relative symbol references. So make the adr_l
>> macro emit a movz/movk sequence instead when executing in module context.
>
> AFAICT, we only use adr_l in a few assembly files that shouldn't matter
> to modules:
>
> * arch/arm64/kernel/head.S
> * arch/arm64/kernel/sleep.S
> * arch/arm64/kvm/hyp-init.S
> * arch/arm64/kvm/hyp/hyp-entry.S
>
> ... so I don't follow why we need this.
>
> Have I missed something? Or do you intend to use this in module code in
> future?
>

Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
the lookup tables from crypto/aes_generic.c, which may be built into
the core kernel, while the code itself may be built as a module.

But in general, if the macro is available to modules, I would like to
make sure that it does not result in code that builds fine but may
fail in some cases only at runtime, especially given the fact that
there is also a Cortex-A53 erratum regarding adrp instructions, for
which reason we build modules with -mcmodel=large (which amounts to
the same thing as the patch above)

> It seems somewhat surprising to me to have adr_l expand to something
> that doesn't use adr/adrp, but that's not necessarily a problem.
>

I did realise that, but I don't think it is a problem tbh.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Ard Biesheuvel]

On 11 January 2017 at 15:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
>> Hi Ard,
>>
>> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
>>> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
>>> modules and the core kernel may exceed 4 GB, putting symbols exported
>>> by the core kernel out of the reach of the ordinary adrp/add instruction
>>> pairs used to generate relative symbol references. So make the adr_l
>>> macro emit a movz/movk sequence instead when executing in module context.
>>
>> AFAICT, we only use adr_l in a few assembly files that shouldn't matter
>> to modules:
>>
>> * arch/arm64/kernel/head.S
>> * arch/arm64/kernel/sleep.S
>> * arch/arm64/kvm/hyp-init.S
>> * arch/arm64/kvm/hyp/hyp-entry.S
>>
>> ... so I don't follow why we need this.
>>
>> Have I missed something? Or do you intend to use this in module code in
>> future?
>>
>
> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
> the lookup tables from crypto/aes_generic.c, which may be built into
> the core kernel, while the code itself may be built as a module.
>
> But in general, if the macro is available to modules, I would like to
> make sure that it does not result in code that builds fine but may
> fail in some cases only at runtime, especially given the fact that
> there is also a Cortex-A53 erratum regarding adrp instructions, for
> which reason we build modules with -mcmodel=large (which amounts to
> the same thing as the patch above)
>

Actually, we could test for

defined(MODULE) && defined(CONFIG_ARM64_MODULE_CMODEL_LARGE)

instead, so we revert to adrp/add pairs for modules if the erratum and
KASLR are both disabled

>> It seems somewhat surprising to me to have adr_l expand to something
>> that doesn't use adr/adrp, but that's not necessarily a problem.
>>
>
> I did realise that, but I don't think it is a problem tbh.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Mark Rutland]

On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote:
> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
> > Hi Ard,
> >
> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> >> modules and the core kernel may exceed 4 GB, putting symbols exported
> >> by the core kernel out of the reach of the ordinary adrp/add instruction
> >> pairs used to generate relative symbol references. So make the adr_l
> >> macro emit a movz/movk sequence instead when executing in module context.
> >
> > AFAICT, we only use adr_l in a few assembly files that shouldn't matter
> > to modules:
> >
> > * arch/arm64/kernel/head.S
> > * arch/arm64/kernel/sleep.S
> > * arch/arm64/kvm/hyp-init.S
> > * arch/arm64/kvm/hyp/hyp-entry.S
> >
> > ... so I don't follow why we need this.
> >
> > Have I missed something? Or do you intend to use this in module code in
> > future?
> 
> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
> the lookup tables from crypto/aes_generic.c, which may be built into
> the core kernel, while the code itself may be built as a module.

Ah, ok.

> But in general, if the macro is available to modules, I would like to
> make sure that it does not result in code that builds fine but may
> fail in some cases only at runtime, especially given the fact that
> there is also a Cortex-A53 erratum regarding adrp instructions, for
> which reason we build modules with -mcmodel=large (which amounts to
> the same thing as the patch above)
> 
> > It seems somewhat surprising to me to have adr_l expand to something
> > that doesn't use adr/adrp, but that's not necessarily a problem.
> 
> I did realise that, but I don't think it is a problem tbh.

In this case it should be fine, certainly.

There are cases like the early boot code and hyp code where it's
critical that we use adr. It's also possible that we might build
(modular) drivers which want some idmapped code, where we want adr, so
it seems unfortunate that this depends on howthe code is built.

So, maybe it's better to have a mov_sym helper for this case, to be
explicit about what we want? That can use either adr* or mov*, or the
latter consistently.

Thanks,
Mark.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Ard Biesheuvel]

On 11 January 2017 at 15:34, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote:
>> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
>> > Hi Ard,
>> >
>> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
>> >> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
>> >> modules and the core kernel may exceed 4 GB, putting symbols exported
>> >> by the core kernel out of the reach of the ordinary adrp/add instruction
>> >> pairs used to generate relative symbol references. So make the adr_l
>> >> macro emit a movz/movk sequence instead when executing in module context.
>> >
>> > AFAICT, we only use adr_l in a few assembly files that shouldn't matter
>> > to modules:
>> >
>> > * arch/arm64/kernel/head.S
>> > * arch/arm64/kernel/sleep.S
>> > * arch/arm64/kvm/hyp-init.S
>> > * arch/arm64/kvm/hyp/hyp-entry.S
>> >
>> > ... so I don't follow why we need this.
>> >
>> > Have I missed something? Or do you intend to use this in module code in
>> > future?
>>
>> Yes. E.g., the scalar AES cipher that I am proposing for v4.11 reuses
>> the lookup tables from crypto/aes_generic.c, which may be built into
>> the core kernel, while the code itself may be built as a module.
>
> Ah, ok.
>
>> But in general, if the macro is available to modules, I would like to
>> make sure that it does not result in code that builds fine but may
>> fail in some cases only at runtime, especially given the fact that
>> there is also a Cortex-A53 erratum regarding adrp instructions, for
>> which reason we build modules with -mcmodel=large (which amounts to
>> the same thing as the patch above)
>>
>> > It seems somewhat surprising to me to have adr_l expand to something
>> > that doesn't use adr/adrp, but that's not necessarily a problem.
>>
>> I did realise that, but I don't think it is a problem tbh.
>
> In this case it should be fine, certainly.
>
> There are cases like the early boot code and hyp code where it's
> critical that we use adr. It's also possible that we might build
> (modular) drivers which want some idmapped code, where we want adr, so
> it seems unfortunate that this depends on howthe code is built.
>

How would /that/ work? Modules are vmalloc'ed, and not covered by the
ID map to begin with, so it is impossible to execute those adr
instructions in a way that would make them return anything other than
the virtual address of the symbol they refer to.

> So, maybe it's better to have a mov_sym helper for this case, to be
> explicit about what we want? That can use either adr* or mov*, or the
> latter consistently.
>

Well, the point is that adr_l should not be used for modules so adding
something that may be used is fine, but that still leaves the risk
that someone may end up using it in a module.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Mark Rutland]

On Wed, Jan 11, 2017 at 04:08:30PM +0000, Ard Biesheuvel wrote:
> On 11 January 2017 at 15:34, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Jan 11, 2017 at 03:25:09PM +0000, Ard Biesheuvel wrote:
> >> On 11 January 2017 at 15:18, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:

> >> But in general, if the macro is available to modules, I would like to
> >> make sure that it does not result in code that builds fine but may
> >> fail in some cases only at runtime, especially given the fact that
> >> there is also a Cortex-A53 erratum regarding adrp instructions, for
> >> which reason we build modules with -mcmodel=large (which amounts to
> >> the same thing as the patch above)
> >>
> >> > It seems somewhat surprising to me to have adr_l expand to something
> >> > that doesn't use adr/adrp, but that's not necessarily a problem.
> >>
> >> I did realise that, but I don't think it is a problem tbh.
> >
> > In this case it should be fine, certainly.
> >
> > There are cases like the early boot code and hyp code where it's
> > critical that we use adr. It's also possible that we might build
> > (modular) drivers which want some idmapped code, where we want adr, so
> > it seems unfortunate that this depends on howthe code is built.
> 
> How would /that/ work? Modules are vmalloc'ed, and not covered by the
> ID map to begin with, so it is impossible to execute those adr
> instructions in a way that would make them return anything other than
> the virtual address of the symbol they refer to.

That's a fair point.

> > So, maybe it's better to have a mov_sym helper for this case, to be
> > explicit about what we want? That can use either adr* or mov*, or the
> > latter consistently.
> 
> Well, the point is that adr_l should not be used for modules so adding
> something that may be used is fine, but that still leaves the risk
> that someone may end up using it in a module.

Sure.

Given you have a concrete use case, and all I have are some vague
concerns for code that doesn't exist at present, I have no real
objection to the patch as it stands. FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Will Deacon]

On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> modules and the core kernel may exceed 4 GB, putting symbols exported
> by the core kernel out of the reach of the ordinary adrp/add instruction
> pairs used to generate relative symbol references. So make the adr_l
> macro emit a movz/movk sequence instead when executing in module context.
> 
> While at it, remove the pointless special case for the stack pointer.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/assembler.h | 36 +++++++++++++++-----
>  1 file changed, 27 insertions(+), 9 deletions(-)

Given that you need this for 4.11, I suggest Catalin takes it as a fix
for 4.10 to avoid the crypto dependency.

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH] arm64: assembler: make adr_l work in modules under KASLR

[Catalin Marinas]

On Thu, Jan 12, 2017 at 03:44:20PM +0000, Will Deacon wrote:
> On Wed, Jan 11, 2017 at 02:54:53PM +0000, Ard Biesheuvel wrote:
> > When CONFIG_RANDOMIZE_MODULE_REGION_FULL=y, the offset between loaded
> > modules and the core kernel may exceed 4 GB, putting symbols exported
> > by the core kernel out of the reach of the ordinary adrp/add instruction
> > pairs used to generate relative symbol references. So make the adr_l
> > macro emit a movz/movk sequence instead when executing in module context.
> > 
> > While at it, remove the pointless special case for the stack pointer.
> > 
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  arch/arm64/include/asm/assembler.h | 36 +++++++++++++++-----
> >  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> Given that you need this for 4.11, I suggest Catalin takes it as a fix
> for 4.10 to avoid the crypto dependency.
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

That's fine by me. Thanks for the ack.

-- 
Catalin