arm64: assembler: make adr_l work in modules under KASLR

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.

Hi Ard,

Quoted 5 lines · 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.

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.

Quoted 17 lines · Mark Rutland

Hi Ard,

Quoted 5 lines · 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.

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

Quoted 5 lines

... 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)

Quoted 3 lines

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.

Quoted 17 lines · Mark Rutland

Hi Ard,

Quoted 5 lines · 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.

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

Quoted 4 lines

... 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)

Quoted 2 lines

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.

Quoted 18 lines · Ard Biesheuvel

Quoted 17 lines · Mark Rutland

Hi Ard,

Quoted 5 lines · 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.

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

Quoted 8 lines

Quoted 4 lines

... 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.

Quoted 10 lines

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)

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 14 lines · Mark Rutland

Quoted 13 lines · Ard Biesheuvel

Quoted 1 line · Mark Rutland

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)

Quoted 2 lines

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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 3 lines

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.

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 <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 36 +++++++++++++++-----
 1 file changed, 27 insertions(+), 9 deletions(-)

Quoted 6 lines · 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.

Quoted 6 lines

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

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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 <[email protected]>