arm64: fault: Don't leak data in ESR context for user fault on kernel VA
RFC PATCH v2 1/1 arm64: fault: Don't leak data in ESR context for user fault on kernel VA
If userspace faults on a kernel address, handing them the raw ESR value on the sigframe as part of the delivered signal can leak data useful to attackers who are using information about the underlying hardware fault type (e.g. translation vs permission) as a mechanism to defeat KASLR.
However there are also legitimate uses for the information provided in the ESR -- notably the GCC and LLVM sanitizers use this to report whether wild pointer accesses by the application are reads or writes (since a wild write is a more serious bug than a wild read), so we don't want to drop the ESR information entirely.
For faulting addresses in the kernel, sanitize the ESR. We choose to present userspace with the illusion that there is nothing mapped in the kernel's part of the address space at all, by reporting all faults as level 0 translation faults.
These fields are safe to pass through to userspace as they depend only on the instruction that userspace used to provoke the fault:
EC IL (always)
ISV CM WNR (for all data aborts)
SAS SSE SRT SF AR (for data aborts when ISV is 1)
All the other fields in ESR except DFSC are architecturally RES0 for an L0 translation fault, so can be zeroed out without confusing userspace.
The illusion is not entirely perfect, as there is a tiny wrinkle where we will report an alignment fault that was not due to the memory type (for instance a LDREX to an unaligned address) as a translation fault, whereas if you do this on real unmapped memory the alignment fault takes precedence. This is not likely to trip anybody up in practice, as the only users we know of for the ESR information who care about the behaviour for kernel addresses only really want to know about the WnR bit.
---
This RFC patch is an alternative proposal to Will's patch
https://patchwork.kernel.org/patch/10258781/
which simply removed the ESR record entirely for kernel addresses.
Changes v1->v2:
* rebased on master
* commit message tweak
* DABT_CUR and IABT_CUR moved to "can't happen" default case
* explicitly clear the bits which are RES0 if ISV == 0
* comment text tweaks
--- arch/arm64/mm/fault.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
arch/arm64/mm/fault.c +55 -0
@@ -293,6 +293,61 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,static void __do_user_fault (struct siginfo *info,unsigned int esr) { current->thread.fault_address = (unsigned long )info->si_addr; + +/* +* If the faulting address is in the kernel, we must sanitize the ESR. +* From userspace's point of view, kernel-only mappings don't exist +* at all, so we report them as level 0 translation faults. +* (This is not quite the way that "no mapping there at all" behaves: +* an alignment fault not caused by the memory type would take +* precedence over translation fault for a real access to empty +* space. Unfortunately we can't easily distinguish "alignment fault +* not caused by memory type" from "alignment fault caused by memory +* type", so we ignore this wrinkle and just return the translation +* fault.) +*/ +if (current->thread.fault_address >=TASK_SIZE ) { +switch (ESR_ELx_EC (esr)) { +case ESR_ELx_EC_DABT_LOW: +/* +* These bits provide only information about the +* faulting instruction, which userspace knows already. +* We explicitly clear bits which are architecturally +* RES0 in case they are given meanings in future. +*/ +if (esr & ESR_ELx_ISV) + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_ISV | ESR_ELx_SAS | + ESR_ELx_SSE | ESR_ELx_SRT_MASK | + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM | + ESR_ELx_WNR; +else + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_CM | ESR_ELx_WNR; + esr |= ESR_ELx_FSC_FAULT; +break ; +case ESR_ELx_EC_IABT_LOW: +/* +* Claim a level 0 translation fault. +* All other bits are architecturally RES0 for faults +* reported with that DFSC value, so we clear them. +*/ + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL; + esr |= ESR_ELx_FSC_FAULT; +break ; +default : +/* +* This should never happen (entry.S only brings us +* into this code for insn and data aborts from a lower +* exception level). Fail safe by not providing an ESR +* context record at all. +*/ +WARN (1 ,"ESR 0x%x is not DABT or IABT from EL0\n" , esr); + esr =0 ; +break ; + } + } + current->thread.fault_code = esr;arm64_force_sig_info (info,esr_to_fault_info (esr)->name, current); }
Signature · 1 line
2.17.0
RFC PATCH v2 1/1 arm64: fault: Don't leak data in ESR context for user fault on kernel VA
Hi Peter,
Sorry for the delay in getting to this! Comments inline.
Quoted 76 lines · Peter Maydell
If userspace faults on a kernel address, handing them the raw ESR value on the sigframe as part of the delivered signal can leak data useful to attackers who are using information about the underlying hardware fault type (e.g. translation vs permission) as a mechanism to defeat KASLR.
However there are also legitimate uses for the information provided in the ESR -- notably the GCC and LLVM sanitizers use this to report whether wild pointer accesses by the application are reads or writes (since a wild write is a more serious bug than a wild read), so we don't want to drop the ESR information entirely.
For faulting addresses in the kernel, sanitize the ESR. We choose to present userspace with the illusion that there is nothing mapped in the kernel's part of the address space at all, by reporting all faults as level 0 translation faults.
These fields are safe to pass through to userspace as they depend only on the instruction that userspace used to provoke the fault:
EC IL (always)
ISV CM WNR (for all data aborts)
SAS SSE SRT SF AR (for data aborts when ISV is 1)
All the other fields in ESR except DFSC are architecturally RES0 for an L0 translation fault, so can be zeroed out without confusing userspace.The illusion is not entirely perfect, as there is a tiny wrinkle where we will report an alignment fault that was not due to the memory type (for instance a LDREX to an unaligned address) as a translation fault, whereas if you do this on real unmapped memory the alignment fault takes precedence. This is not likely to trip anybody up in practice, as the only users we know of for the ESR information who care about the behaviour for kernel addresses only really want to know about the WnR bit.
Signed-off-by Peter Maydell <[email protected]>---
This RFC patch is an alternative proposal to Will's patch
https://patchwork.kernel.org/patch/10258781/
which simply removed the ESR record entirely for kernel addresses.Changes
v1->v2:
* rebased on master
* commit message tweak
*DABT_CURandIABT_CURmoved to "can't happen" default case
* explicitly clear the bits which are RES0 if ISV == 0
* comment text tweaks--- arch/arm64/mm/fault.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)arch/arm64/mm/fault.c +18
-0@@ -293,6 +293,61 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,static void __do_user_fault (struct siginfo *info,unsigned int esr) { current->thread.fault_address = (unsigned long )info->si_addr; + +/* +* If the faulting address is in the kernel, we must sanitize the ESR. +* From userspace's point of view, kernel-only mappings don't exist +* at all, so we report them as level 0 translation faults. +* (This is not quite the way that "no mapping there at all" behaves: +* an alignment fault not caused by the memory type would take +* precedence over translation fault for a real access to empty +* space. Unfortunately we can't easily distinguish "alignment fault +* not caused by memory type" from "alignment fault caused by memory +* type", so we ignore this wrinkle and just return the translation +* fault.) +*/ +if (current->thread.fault_address >=TASK_SIZE ) { +switch (ESR_ELx_EC (esr)) { +case ESR_ELx_EC_DABT_LOW: +/* +* These bits provide only information about the
arch/arm64/mm/fault.c static void __do_kernel_fault(unsigned long addr, unsigned int esr, · line 323
Quoted 10 lines
arch/arm64/mm/fault.c +10
-0+* faulting instruction, which userspace knows already. +* We explicitly clear bits which are architecturally +* RES0 in case they are given meanings in future. +*/ +if (esr & ESR_ELx_ISV) + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_ISV | ESR_ELx_SAS | + ESR_ELx_SSE | ESR_ELx_SRT_MASK | + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM | + ESR_ELx_WNR;
Reading through the ARM ARM, it seems to say that ISV is always 0 for faults reported in ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT, SF and AR from this list and actually drop the conditional altogether.
Will
RFC PATCH v2 1/1 arm64: fault: Don't leak data in ESR context for user fault on kernel VA
Quoted 19 lines · Will Deacon
Hi Peter,
Sorry for the delay in getting to this! Comments inline.
arch/arm64/mm/fault.c static void __do_kernel_fault(unsigned long addr, unsigned int esr, · line 323
Quoted 12 lines · Peter Maydell
arch/arm64/mm/fault.c +12
-0+/* +* These bits provide only information about the +* faulting instruction, which userspace knows already. +* We explicitly clear bits which are architecturally +* RES0 in case they are given meanings in future. +*/ +if (esr & ESR_ELx_ISV) + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_ISV | ESR_ELx_SAS | + ESR_ELx_SSE | ESR_ELx_SRT_MASK | + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM | + ESR_ELx_WNR;
Quoted 3 lines
Reading through the ARM ARM, it seems to say that ISV is always 0 for faults reported in
ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT, SF and AR from this list and actually drop the conditional altogether.
Mmm, I guess so, if we're guaranteed to only be working with ESRs taken to EL1 (or we want to present userspace with an ESR that looks like that regardless of what EL we took it to). I'll respin without the conditional.
thanks
-- PMM
RFC PATCH v2 1/1 arm64: fault: Don't leak data in ESR context for user fault on kernel VA
Quoted 20 lines · Peter Maydell
Quoted 19 lines · Will Deacon
Hi Peter,
Sorry for the delay in getting to this! Comments inline.
Quoted 12 lines · Peter Maydell
arch/arm64/mm/fault.c +12
-0+/* +* These bits provide only information about the +* faulting instruction, which userspace knows already. +* We explicitly clear bits which are architecturally +* RES0 in case they are given meanings in future. +*/ +if (esr & ESR_ELx_ISV) + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_ISV | ESR_ELx_SAS | + ESR_ELx_SSE | ESR_ELx_SRT_MASK | + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM | + ESR_ELx_WNR;
Quoted 6 lines
Quoted 3 lines
Reading through the ARM ARM, it seems to say that ISV is always 0 for faults reported in
ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT, SF and AR from this list and actually drop the conditional altogether.Mmm, I guess so, if we're guaranteed to only be working with ESRs taken to EL1 (or we want to present userspace with an ESR that
There is no direct interface between EL0 and EL2 with the stage2 translation enabled, so even if this data is available for a fault at EL2, we won't be signalling the fault via delivering a signal to EL0.
Quoted 2 lines
looks like that regardless of what EL we took it to). I'll respin without the conditional.
Sounds fair. It might have been me that suggested this list of fields in the first place: I'd not completely understood the ISV behaviour previously, it seems.
Cheers
---Dave
RFC PATCH v2 1/1 arm64: fault: Don't leak data in ESR context for user fault on kernel VA
Quoted 25 lines · Dave Martin
Quoted 24 lines · Peter Maydell
Quoted 22 lines · Will Deacon
Hi Peter,
Sorry for the delay in getting to this! Comments inline.
arch/arm64/mm/fault.c static void __do_kernel_fault(unsigned long addr, unsigned int esr, · line 323
Quoted 12 lines · Peter Maydell
arch/arm64/mm/fault.c +12
-0+/* +* These bits provide only information about the +* faulting instruction, which userspace knows already. +* We explicitly clear bits which are architecturally +* RES0 in case they are given meanings in future. +*/ +if (esr & ESR_ELx_ISV) + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL | + ESR_ELx_ISV | ESR_ELx_SAS | + ESR_ELx_SSE | ESR_ELx_SRT_MASK | + ESR_ELx_SF | ESR_ELx_AR | ESR_ELx_CM | + ESR_ELx_WNR;Reading through the ARM ARM, it seems to say that ISV is always 0 for faults reported in
ESR_EL1, which implies we can drop ISV, SAS, SSE, SRT, SF and AR from this list and actually drop the conditional altogether.
Quoted 6 lines
Quoted 2 lines
Mmm, I guess so, if we're guaranteed to only be working with ESRs taken to EL1 (or we want to present userspace with an ESR that
There is no direct interface between EL0 and EL2 with the stage2 translation enabled, so even if this data is available for a fault at EL2, we won't be signalling the fault via delivering a signal to EL0.
Right -- and even if we did somehow end up with that, we probably wouldn't want to leak to userspace that their access was trapped to EL2 rather than EL1, so we should present the illusion that it was trapped at EL1 regardless.
thanks
-- PMM