| Message ID | 20230307115727.2311154-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: entry: Simplify tramp_alias macro and tramp_exit routine | expand |
Hey Ard, On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > The tramp_alias macro constructs the virtual alias of a symbol in the > trampoline text mapping, based on its kernel text address, and does so > in a way that is more convoluted than necessary. So let's simplify it. > > Also, now that the address of the vector table is kept in a per-CPU > variable, there is no need to defer the load and the assignment of > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > register, this means we can restore X30 earlier as well, and only leave > X29 for tramp_exit() to restore. > > Finally, merge the native and compat versions of tramp_exit() - the only > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > it conditionally rather than having two distinct code paths. > > While at it, give some related symbols static linkage, considering that > they are only referenced from the object file that defines them. > > Cc: James Morse <james.morse@arm.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm64/kernel/entry.S | 58 ++++++++------------ > 1 file changed, 22 insertions(+), 36 deletions(-) [...] > @@ -768,7 +753,7 @@ alternative_else_nop_endif > */ > .pushsection ".entry.tramp.text", "ax" > .align 11 > -SYM_CODE_START_NOALIGN(tramp_vectors) > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > SYM_CODE_END(tramp_vectors) > > -SYM_CODE_START(tramp_exit_native) > - tramp_exit > -SYM_CODE_END(tramp_exit_native) > - > -SYM_CODE_START(tramp_exit_compat) > - tramp_exit 32 > -SYM_CODE_END(tramp_exit_compat) > +SYM_CODE_START_LOCAL(tramp_exit) > + // Entered with Z flag set if task is native > + tramp_unmap_kernel x29 > + mrs x29, far_el1 // restore x29 > + csel x29, x29, xzr, eq // clear x29 for compat Why do we care about zeroing x29 for a compat task? Will
On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > Hey Ard, > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > The tramp_alias macro constructs the virtual alias of a symbol in the > > trampoline text mapping, based on its kernel text address, and does so > > in a way that is more convoluted than necessary. So let's simplify it. > > > > Also, now that the address of the vector table is kept in a per-CPU > > variable, there is no need to defer the load and the assignment of > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > register, this means we can restore X30 earlier as well, and only leave > > X29 for tramp_exit() to restore. > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > it conditionally rather than having two distinct code paths. > > > > While at it, give some related symbols static linkage, considering that > > they are only referenced from the object file that defines them. > > > > Cc: James Morse <james.morse@arm.com> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > 1 file changed, 22 insertions(+), 36 deletions(-) > > [...] > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > */ > > .pushsection ".entry.tramp.text", "ax" > > .align 11 > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > SYM_CODE_END(tramp_vectors) > > > > -SYM_CODE_START(tramp_exit_native) > > - tramp_exit > > -SYM_CODE_END(tramp_exit_native) > > - > > -SYM_CODE_START(tramp_exit_compat) > > - tramp_exit 32 > > -SYM_CODE_END(tramp_exit_compat) > > +SYM_CODE_START_LOCAL(tramp_exit) > > + // Entered with Z flag set if task is native > > + tramp_unmap_kernel x29 > > + mrs x29, far_el1 // restore x29 > > + csel x29, x29, xzr, eq // clear x29 for compat > > Why do we care about zeroing x29 for a compat task? > The only reason I could think of is that it we don't want to leak anything, in case the value might be exposed in some way? (e.,g, ptrace from another, 64-bit process?)
On Thu, 6 Apr 2023 at 17:30, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > > > Hey Ard, > > > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > > The tramp_alias macro constructs the virtual alias of a symbol in the > > > trampoline text mapping, based on its kernel text address, and does so > > > in a way that is more convoluted than necessary. So let's simplify it. > > > > > > Also, now that the address of the vector table is kept in a per-CPU > > > variable, there is no need to defer the load and the assignment of > > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > > register, this means we can restore X30 earlier as well, and only leave > > > X29 for tramp_exit() to restore. > > > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > > it conditionally rather than having two distinct code paths. > > > > > > While at it, give some related symbols static linkage, considering that > > > they are only referenced from the object file that defines them. > > > > > > Cc: James Morse <james.morse@arm.com> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > > 1 file changed, 22 insertions(+), 36 deletions(-) > > > > [...] > > > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > > */ > > > .pushsection ".entry.tramp.text", "ax" > > > .align 11 > > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > > SYM_CODE_END(tramp_vectors) > > > > > > -SYM_CODE_START(tramp_exit_native) > > > - tramp_exit > > > -SYM_CODE_END(tramp_exit_native) > > > - > > > -SYM_CODE_START(tramp_exit_compat) > > > - tramp_exit 32 > > > -SYM_CODE_END(tramp_exit_compat) > > > +SYM_CODE_START_LOCAL(tramp_exit) > > > + // Entered with Z flag set if task is native > > > + tramp_unmap_kernel x29 > > > + mrs x29, far_el1 // restore x29 > > > + csel x29, x29, xzr, eq // clear x29 for compat > > > > Why do we care about zeroing x29 for a compat task? > > > > The only reason I could think of is that it we don't want to leak > anything, in case the value might be exposed in some way? (e.,g, > ptrace from another, 64-bit process?) BTW just realized that I failed to mention in the commit log that this removes a memory access from the the ret_to_user path: By moving this - tramp_data_read_var x30, this_cpu_vector - get_this_cpu_offset x29 - ldr x30, [x30, x29] out of the trampoline and into the ordinary kernel mapping, it can be changed to +4: ldr_this_cpu x30, this_cpu_vector, x29 which removes one of the two LDR instructions.
On Thu, Apr 06, 2023 at 05:30:58PM +0200, Ard Biesheuvel wrote: > On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > > > Hey Ard, > > > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > > The tramp_alias macro constructs the virtual alias of a symbol in the > > > trampoline text mapping, based on its kernel text address, and does so > > > in a way that is more convoluted than necessary. So let's simplify it. > > > > > > Also, now that the address of the vector table is kept in a per-CPU > > > variable, there is no need to defer the load and the assignment of > > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > > register, this means we can restore X30 earlier as well, and only leave > > > X29 for tramp_exit() to restore. > > > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > > it conditionally rather than having two distinct code paths. > > > > > > While at it, give some related symbols static linkage, considering that > > > they are only referenced from the object file that defines them. > > > > > > Cc: James Morse <james.morse@arm.com> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > > 1 file changed, 22 insertions(+), 36 deletions(-) > > > > [...] > > > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > > */ > > > .pushsection ".entry.tramp.text", "ax" > > > .align 11 > > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > > SYM_CODE_END(tramp_vectors) > > > > > > -SYM_CODE_START(tramp_exit_native) > > > - tramp_exit > > > -SYM_CODE_END(tramp_exit_native) > > > - > > > -SYM_CODE_START(tramp_exit_compat) > > > - tramp_exit 32 > > > -SYM_CODE_END(tramp_exit_compat) > > > +SYM_CODE_START_LOCAL(tramp_exit) > > > + // Entered with Z flag set if task is native > > > + tramp_unmap_kernel x29 > > > + mrs x29, far_el1 // restore x29 > > > + csel x29, x29, xzr, eq // clear x29 for compat > > > > Why do we care about zeroing x29 for a compat task? > > > > The only reason I could think of is that it we don't want to leak > anything, in case the value might be exposed in some way? (e.,g, > ptrace from another, 64-bit process?) I'm not sure this is possible (see 'user_aarch32_ptrace_view'), but in any case wouldn't it be a problem for the existing code if it was? i.e. the zeroing of x29 should be a separate fix if it's actually needed. Will
On Tue, 11 Apr 2023 at 19:33, Will Deacon <will@kernel.org> wrote: > > On Thu, Apr 06, 2023 at 05:30:58PM +0200, Ard Biesheuvel wrote: > > On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > > > > > Hey Ard, > > > > > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > > > The tramp_alias macro constructs the virtual alias of a symbol in the > > > > trampoline text mapping, based on its kernel text address, and does so > > > > in a way that is more convoluted than necessary. So let's simplify it. > > > > > > > > Also, now that the address of the vector table is kept in a per-CPU > > > > variable, there is no need to defer the load and the assignment of > > > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > > > register, this means we can restore X30 earlier as well, and only leave > > > > X29 for tramp_exit() to restore. > > > > > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > > > it conditionally rather than having two distinct code paths. > > > > > > > > While at it, give some related symbols static linkage, considering that > > > > they are only referenced from the object file that defines them. > > > > > > > > Cc: James Morse <james.morse@arm.com> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > --- > > > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > > > 1 file changed, 22 insertions(+), 36 deletions(-) > > > > > > [...] > > > > > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > > > */ > > > > .pushsection ".entry.tramp.text", "ax" > > > > .align 11 > > > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > > > SYM_CODE_END(tramp_vectors) > > > > > > > > -SYM_CODE_START(tramp_exit_native) > > > > - tramp_exit > > > > -SYM_CODE_END(tramp_exit_native) > > > > - > > > > -SYM_CODE_START(tramp_exit_compat) > > > > - tramp_exit 32 > > > > -SYM_CODE_END(tramp_exit_compat) > > > > +SYM_CODE_START_LOCAL(tramp_exit) > > > > + // Entered with Z flag set if task is native > > > > + tramp_unmap_kernel x29 > > > > + mrs x29, far_el1 // restore x29 > > > > + csel x29, x29, xzr, eq // clear x29 for compat > > > > > > Why do we care about zeroing x29 for a compat task? > > > > > > > The only reason I could think of is that it we don't want to leak > > anything, in case the value might be exposed in some way? (e.,g, > > ptrace from another, 64-bit process?) > > I'm not sure this is possible (see 'user_aarch32_ptrace_view'), but in > any case wouldn't it be a problem for the existing code if it was? i.e. > the zeroing of x29 should be a separate fix if it's actually needed. > The existing code has two different versions, and the 32-bit one never preserves or restores X29 to/from FAR_EL1. I've merged the two code paths on the restore side, and the merged version always restores X29, and subsequently zeroes it again for compat tasks. We could drop the CSEL as well as the conditional branch around the preserve entirely if we simply don't care about the contents of X29 for compat tasks.
On Tue, Apr 11, 2023 at 08:42:32PM +0200, Ard Biesheuvel wrote: > On Tue, 11 Apr 2023 at 19:33, Will Deacon <will@kernel.org> wrote: > > On Thu, Apr 06, 2023 at 05:30:58PM +0200, Ard Biesheuvel wrote: > > > On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > > > > The tramp_alias macro constructs the virtual alias of a symbol in the > > > > > trampoline text mapping, based on its kernel text address, and does so > > > > > in a way that is more convoluted than necessary. So let's simplify it. > > > > > > > > > > Also, now that the address of the vector table is kept in a per-CPU > > > > > variable, there is no need to defer the load and the assignment of > > > > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > > > > register, this means we can restore X30 earlier as well, and only leave > > > > > X29 for tramp_exit() to restore. > > > > > > > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > > > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > > > > it conditionally rather than having two distinct code paths. > > > > > > > > > > While at it, give some related symbols static linkage, considering that > > > > > they are only referenced from the object file that defines them. > > > > > > > > > > Cc: James Morse <james.morse@arm.com> > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > --- > > > > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > > > > 1 file changed, 22 insertions(+), 36 deletions(-) > > > > > > > > [...] > > > > > > > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > > > > */ > > > > > .pushsection ".entry.tramp.text", "ax" > > > > > .align 11 > > > > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > > > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > > > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > > > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > > > > SYM_CODE_END(tramp_vectors) > > > > > > > > > > -SYM_CODE_START(tramp_exit_native) > > > > > - tramp_exit > > > > > -SYM_CODE_END(tramp_exit_native) > > > > > - > > > > > -SYM_CODE_START(tramp_exit_compat) > > > > > - tramp_exit 32 > > > > > -SYM_CODE_END(tramp_exit_compat) > > > > > +SYM_CODE_START_LOCAL(tramp_exit) > > > > > + // Entered with Z flag set if task is native > > > > > + tramp_unmap_kernel x29 > > > > > + mrs x29, far_el1 // restore x29 > > > > > + csel x29, x29, xzr, eq // clear x29 for compat > > > > > > > > Why do we care about zeroing x29 for a compat task? > > > > > > > > > > The only reason I could think of is that it we don't want to leak > > > anything, in case the value might be exposed in some way? (e.,g, > > > ptrace from another, 64-bit process?) > > > > I'm not sure this is possible (see 'user_aarch32_ptrace_view'), but in > > any case wouldn't it be a problem for the existing code if it was? i.e. > > the zeroing of x29 should be a separate fix if it's actually needed. > > > > The existing code has two different versions, and the 32-bit one never > preserves or restores X29 to/from FAR_EL1. Sure, but doesn't 'tramp_unmap_kernel' leave whatever junk behind in x29 (the page-table base of the intermediate table root) when it's expanded in 'tramp_exit'? > I've merged the two code paths on the restore side, and the merged > version always restores X29, and subsequently zeroes it again for > compat tasks. > > We could drop the CSEL as well as the conditional branch around the > preserve entirely if we simply don't care about the contents of X29 > for compat tasks. I'm just trying to work out whether we care, because if we do, then it would be best to fix that _before_ reworking the rest of the code so that it's easy to backport. Will
On Tue, 11 Apr 2023 at 23:15, Will Deacon <will@kernel.org> wrote: > > On Tue, Apr 11, 2023 at 08:42:32PM +0200, Ard Biesheuvel wrote: > > On Tue, 11 Apr 2023 at 19:33, Will Deacon <will@kernel.org> wrote: > > > On Thu, Apr 06, 2023 at 05:30:58PM +0200, Ard Biesheuvel wrote: > > > > On Thu, 6 Apr 2023 at 16:39, Will Deacon <will@kernel.org> wrote: > > > > > On Tue, Mar 07, 2023 at 12:57:27PM +0100, Ard Biesheuvel wrote: > > > > > > The tramp_alias macro constructs the virtual alias of a symbol in the > > > > > > trampoline text mapping, based on its kernel text address, and does so > > > > > > in a way that is more convoluted than necessary. So let's simplify it. > > > > > > > > > > > > Also, now that the address of the vector table is kept in a per-CPU > > > > > > variable, there is no need to defer the load and the assignment of > > > > > > VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp > > > > > > register, this means we can restore X30 earlier as well, and only leave > > > > > > X29 for tramp_exit() to restore. > > > > > > > > > > > > Finally, merge the native and compat versions of tramp_exit() - the only > > > > > > difference is whether X29 is reloaded from FAR_EL1, and we can just zero > > > > > > it conditionally rather than having two distinct code paths. > > > > > > > > > > > > While at it, give some related symbols static linkage, considering that > > > > > > they are only referenced from the object file that defines them. > > > > > > > > > > > > Cc: James Morse <james.morse@arm.com> > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > > > --- > > > > > > arch/arm64/kernel/entry.S | 58 ++++++++------------ > > > > > > 1 file changed, 22 insertions(+), 36 deletions(-) > > > > > > > > > > [...] > > > > > > > > > > > @@ -768,7 +753,7 @@ alternative_else_nop_endif > > > > > > */ > > > > > > .pushsection ".entry.tramp.text", "ax" > > > > > > .align 11 > > > > > > -SYM_CODE_START_NOALIGN(tramp_vectors) > > > > > > +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) > > > > > > #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY > > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP > > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW > > > > > > @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) > > > > > > generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE > > > > > > SYM_CODE_END(tramp_vectors) > > > > > > > > > > > > -SYM_CODE_START(tramp_exit_native) > > > > > > - tramp_exit > > > > > > -SYM_CODE_END(tramp_exit_native) > > > > > > - > > > > > > -SYM_CODE_START(tramp_exit_compat) > > > > > > - tramp_exit 32 > > > > > > -SYM_CODE_END(tramp_exit_compat) > > > > > > +SYM_CODE_START_LOCAL(tramp_exit) > > > > > > + // Entered with Z flag set if task is native > > > > > > + tramp_unmap_kernel x29 > > > > > > + mrs x29, far_el1 // restore x29 > > > > > > + csel x29, x29, xzr, eq // clear x29 for compat > > > > > > > > > > Why do we care about zeroing x29 for a compat task? > > > > > > > > > > > > > The only reason I could think of is that it we don't want to leak > > > > anything, in case the value might be exposed in some way? (e.,g, > > > > ptrace from another, 64-bit process?) > > > > > > I'm not sure this is possible (see 'user_aarch32_ptrace_view'), but in > > > any case wouldn't it be a problem for the existing code if it was? i.e. > > > the zeroing of x29 should be a separate fix if it's actually needed. > > > > > > > The existing code has two different versions, and the 32-bit one never > > preserves or restores X29 to/from FAR_EL1. > > Sure, but doesn't 'tramp_unmap_kernel' leave whatever junk behind in x29 > (the page-table base of the intermediate table root) when it's expanded > in 'tramp_exit'? > Yes, so the only potential issue there would be that we might leak the physical placement of the kernel image, but if that value can never be observed from the context of the compat task, that shouldn't matter. So the current situation should be fine afaict. > > I've merged the two code paths on the restore side, and the merged > > version always restores X29, and subsequently zeroes it again for > > compat tasks. > > > > We could drop the CSEL as well as the conditional branch around the > > preserve entirely if we simply don't care about the contents of X29 > > for compat tasks. > > I'm just trying to work out whether we care, because if we do, then it > would be best to fix that _before_ reworking the rest of the code so > that it's easy to backport. > Yeah so the difference is not restoring X29 at all (as we do now) versus restoring whatever value FAR_EL1 held, as the compat code path does not store X29 in there (as its value is irrelevant) I think none of this matters tbh, but clearing it for compat is a reasonable choice given that it's just a single CSEL.
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index ab2a6e33c0528d82..953aac3620478673 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -101,12 +101,11 @@ .org .Lventry_start\@ + 128 // Did we overflow the ventry slot? .endm - .macro tramp_alias, dst, sym, tmp - mov_q \dst, TRAMP_VALIAS - adr_l \tmp, \sym - add \dst, \dst, \tmp - adr_l \tmp, .entry.tramp.text - sub \dst, \dst, \tmp + .macro tramp_alias, dst, sym + .set .Lalias\@, TRAMP_VALIAS + \sym - .entry.tramp.text + movz \dst, :abs_g2_s:.Lalias\@ + movk \dst, :abs_g1_nc:.Lalias\@ + movk \dst, :abs_g0_nc:.Lalias\@ .endm /* @@ -437,11 +436,13 @@ alternative_else_nop_endif #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 bne 4f msr far_el1, x29 - tramp_alias x30, tramp_exit_native, x29 - br x30 -4: - tramp_alias x30, tramp_exit_compat, x29 - br x30 + +4: ldr_this_cpu x30, this_cpu_vector, x29 + tramp_alias x29, tramp_exit + msr vbar_el1, x30 // install vector table + ldr lr, [sp, #S_LR] // restore x30 + add sp, sp, #PT_REGS_SIZE // restore sp + br x29 // Z flag set if native #endif .else ldr lr, [sp, #S_LR] @@ -732,22 +733,6 @@ alternative_else_nop_endif .org 1b + 128 // Did we overflow the ventry slot? .endm - .macro tramp_exit, regsize = 64 - tramp_data_read_var x30, this_cpu_vector - get_this_cpu_offset x29 - ldr x30, [x30, x29] - - msr vbar_el1, x30 - ldr lr, [sp, #S_LR] - tramp_unmap_kernel x29 - .if \regsize == 64 - mrs x29, far_el1 - .endif - add sp, sp, #PT_REGS_SIZE // restore sp - eret - sb - .endm - .macro generate_tramp_vector, kpti, bhb .Lvector_start\@: .space 0x400 @@ -768,7 +753,7 @@ alternative_else_nop_endif */ .pushsection ".entry.tramp.text", "ax" .align 11 -SYM_CODE_START_NOALIGN(tramp_vectors) +SYM_CODE_START_LOCAL_NOALIGN(tramp_vectors) #ifdef CONFIG_MITIGATE_SPECTRE_BRANCH_HISTORY generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_LOOP generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_FW @@ -777,13 +762,14 @@ SYM_CODE_START_NOALIGN(tramp_vectors) generate_tramp_vector kpti=1, bhb=BHB_MITIGATION_NONE SYM_CODE_END(tramp_vectors) -SYM_CODE_START(tramp_exit_native) - tramp_exit -SYM_CODE_END(tramp_exit_native) - -SYM_CODE_START(tramp_exit_compat) - tramp_exit 32 -SYM_CODE_END(tramp_exit_compat) +SYM_CODE_START_LOCAL(tramp_exit) + // Entered with Z flag set if task is native + tramp_unmap_kernel x29 + mrs x29, far_el1 // restore x29 + csel x29, x29, xzr, eq // clear x29 for compat + eret + sb +SYM_CODE_END(tramp_exit) .popsection // .entry.tramp.text #endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */ @@ -1077,7 +1063,7 @@ alternative_if_not ARM64_UNMAP_KERNEL_AT_EL0 alternative_else_nop_endif #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 - tramp_alias dst=x5, sym=__sdei_asm_exit_trampoline, tmp=x3 + tramp_alias dst=x5, sym=__sdei_asm_exit_trampoline br x5 #endif SYM_CODE_END(__sdei_asm_handler)
The tramp_alias macro constructs the virtual alias of a symbol in the trampoline text mapping, based on its kernel text address, and does so in a way that is more convoluted than necessary. So let's simplify it. Also, now that the address of the vector table is kept in a per-CPU variable, there is no need to defer the load and the assignment of VBAR_EL1 to tramp_exit(). Given that tramp_alias no longer needs a temp register, this means we can restore X30 earlier as well, and only leave X29 for tramp_exit() to restore. Finally, merge the native and compat versions of tramp_exit() - the only difference is whether X29 is reloaded from FAR_EL1, and we can just zero it conditionally rather than having two distinct code paths. While at it, give some related symbols static linkage, considering that they are only referenced from the object file that defines them. Cc: James Morse <james.morse@arm.com> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kernel/entry.S | 58 ++++++++------------ 1 file changed, 22 insertions(+), 36 deletions(-)