arm64: fault: Don't populate ESR context for user fault on kernel VA

User faults on kernel addresses are a good sign that the faulting task is either up to no good or is in deep trouble. In such situations, exposing the optional ESR context on the sigframe as part of the delivered signal is only useful to attackers who are using information about underlying hardware fault (e.g. translation vs permission) as a mechanism to defeat KASLR.

Remove the ESR context from the sigframe for user faults on kernel addresses.

Cc Ard Biesheuvel <[email protected]>

Cc Dave Martin <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

Here's another one that doesn't make a huge amount of difference when kpti is enabled, but I think is a change worth making all the same.

  arch/arm64/mm/fault.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

arch/arm64/mm/fault.c +0 -0

index 49dfb08a6c4d..b9800395788e 100644

arch/arm64/mm/fault.c +0 -0

  	arm64_force_sig_info(info, esr_to_fault_info(esr)->name, current);
  }
 

Quoted 26 lines · Will Deacon

User faults on kernel addresses are a good sign that the faulting task is either up to no good or is in deep trouble. In such situations, exposing the optional ESR context on the sigframe as part of the delivered signal is only useful to attackers who are using information about underlying hardware fault (e.g. translation vs permission) as a mechanism to defeat KASLR.

Remove the ESR context from the sigframe for user faults on kernel addresses.

Cc Ard Biesheuvel <[email protected]>

Cc Dave Martin <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

Here's another one that doesn't make a huge amount of difference when kpti is enabled, but I think is a change worth making all the same.

 arch/arm64/mm/fault.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/arm64/mm/fault.c +0 -0

index 49dfb08a6c4d..b9800395788e 100644

arch/arm64/mm/fault.c static void __do_kernel_fault(unsigned long addr, unsigned int esr, · line 297

Quoted 8 lines

arch/arm64/mm/fault.c +4 -2

@@ -292,8 +292,10 @@ 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;
-	current->thread.fault_code = esr;
+	unsigned long addr = (unsigned long)info->si_addr;
+
+	current->thread.fault_address = addr;
+	current->thread.fault_code = addr < TASK_SIZE ? esr : 0;

Nit: there are still non-kernel addresses above TASK_SIZE which would only imply a wild pointer rather than nefarious misdeeds, but I guess if you can already see that the faulting address is in the hole you don't really need a level 0 translation fault spelled out. More generally though, if there's a chance that someone might still try to interpret fault_code as an ESR value regardless of what happened, should we be setting it to ESR_ELx_IL rather than 0, to be consistent with the implied "Unknown reason" EC value?

User faults on kernel addresses are a good sign that the faulting task is either up to no good or is in deep trouble. In such situations, exposing the optional ESR context on the sigframe as part of the delivered signal is only useful to attackers who are using information about underlying hardware fault (e.g. translation vs permission) as a mechanism to defeat KASLR.

Remove the ESR context from the sigframe for user faults on kernel addresses.

Cc Ard Biesheuvel <[email protected]>

Cc Dave Martin <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---

Here's another one that doesn't make a huge amount of difference when kpti is enabled, but I think is a change worth making all the same.

 arch/arm64/mm/fault.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/arm64/mm/fault.c +4 -2

index 49dfb08a6c4d..b9800395788e 100644
@@ -292,8 +292,10 @@ 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;
-	current->thread.fault_code = esr;
+	unsigned long addr = (unsigned long)info->si_addr;
+
+	current->thread.fault_address = addr;
+	current->thread.fault_code = addr < TASK_SIZE ? esr : 0;
 	arm64_force_sig_info(info, esr_to_fault_info(esr)->name, current);
 }
 

Signature · 7 lines

2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 7 lines · Will Deacon

User faults on kernel addresses are a good sign that the faulting task is either up to no good or is in deep trouble. In such situations, exposing the optional ESR context on the sigframe as part of the delivered signal is only useful to attackers who are using information about underlying hardware fault (e.g. translation vs permission) as a mechanism to defeat KASLR.

Quoted 2 lines

Remove the ESR context from the sigframe for user faults on kernel addresses.

As this wording suggests, this change causes esr_context to disappear entirely from the signal frame. Previously, I think user code could have relied on its being present for certain signals.

Does Debian's codesearch throw up any nontrivial users of esr_context?

Quoted 1 line · Dave Martin

Does Debian's codesearch throw up any nontrivial users of esr_context?

The main one seems to be ASAN, which uses the RnW bit to report "READ", "WRITE" or "UNKNOWN". So with this change, the access will be treated as UNKNOWN for kernel addresses.

Whilst I can see how that might cause a testsuite regression, I'm struggling to see how it could sensible impact ASAN given that userspace never has permission to access these addresses and so the fault should be treated as fatal regardless of whether or not it's a read or a write.

Quoted 11 lines · Will Deacon

Quoted 1 line · Dave Martin

Does Debian's codesearch throw up any nontrivial users of esr_context?

The main one seems to be ASAN, which uses the RnW bit to report "READ", "WRITE" or "UNKNOWN". So with this change, the access will be treated as UNKNOWN for kernel addresses.

Whilst I can see how that might cause a testsuite regression, I'm struggling to see how it could sensible impact ASAN given that userspace never has permission to access these addresses and so the fault should be treated as fatal regardless of whether or not it's a read or a write.

Right, but the read/write/unknown classification also affects the severity of that warning level ('scariness' in the asan code), and it's not immediately clear how much might then in turn be relying on that.

I think that if you have widely deployed code that is using this ESR value, then it's kernel ABI that people are relying on, and the safest thing to do is to make the minimal change that will fix the problem you have, not to yank the whole thing entirely and hope that the users will cope.

QEMU is not currently using the ESR value, but it would be nice to in future, and it would certainly be irritating not to have the WnR information just because the faulting address happens to be in the top half of memory.

AFAIK the major thing that consumers actually are after here is the WnR information, so preserving that and sanitizing the rest of the ESR if necessary would be a less risky fix IMHO.

Quoted 29 lines · Peter Maydell

Quoted 11 lines · Will Deacon

Quoted 1 line · Dave Martin

Does Debian's codesearch throw up any nontrivial users of esr_context?

The main one seems to be ASAN, which uses the RnW bit to report "READ", "WRITE" or "UNKNOWN". So with this change, the access will be treated as UNKNOWN for kernel addresses.

Whilst I can see how that might cause a testsuite regression, I'm struggling to see how it could sensible impact ASAN given that userspace never has permission to access these addresses and so the fault should be treated as fatal regardless of whether or not it's a read or a write.

Right, but the read/write/unknown classification also affects the severity of that warning level ('scariness' in the asan code), and it's not immediately clear how much might then in turn be relying on that.

I think that if you have widely deployed code that is using this ESR value, then it's kernel ABI that people are relying on, and the safest thing to do is to make the minimal change that will fix the problem you have, not to yank the whole thing entirely and hope that the users will cope.

QEMU is not currently using the ESR value, but it would be nice to in future, and it would certainly be irritating not to have the WnR information just because the faulting address happens to be in the top half of memory.

Quoted 3 lines

AFAIK the major thing that consumers actually are after here is the WnR information, so preserving that and sanitizing the rest of the ESR if necessary would be a less risky fix IMHO.

If there is a way of squashing the syndrome information so that it reports a fixed syndrome except for information about what userspace attempted to do (i.e., WnR -- I dunno if there's anything else), that seems reasonable.

Quoted 30 lines · Dave Martin

Quoted 29 lines · Peter Maydell

Quoted 11 lines · Will Deacon

Quoted 1 line · Dave Martin

Does Debian's codesearch throw up any nontrivial users of esr_context?

The main one seems to be ASAN, which uses the RnW bit to report "READ", "WRITE" or "UNKNOWN". So with this change, the access will be treated as UNKNOWN for kernel addresses.

Whilst I can see how that might cause a testsuite regression, I'm struggling to see how it could sensible impact ASAN given that userspace never has permission to access these addresses and so the fault should be treated as fatal regardless of whether or not it's a read or a write.

Right, but the read/write/unknown classification also affects the severity of that warning level ('scariness' in the asan code), and it's not immediately clear how much might then in turn be relying on that.

I think that if you have widely deployed code that is using this ESR value, then it's kernel ABI that people are relying on, and the safest thing to do is to make the minimal change that will fix the problem you have, not to yank the whole thing entirely and hope that the users will cope.

QEMU is not currently using the ESR value, but it would be nice to in future, and it would certainly be irritating not to have the WnR information just because the faulting address happens to be in the top half of memory.

Quoted 8 lines

Quoted 3 lines

AFAIK the major thing that consumers actually are after here is the WnR information, so preserving that and sanitizing the rest of the ESR if necessary would be a less risky fix IMHO.

If there is a way of squashing the syndrome information so that it reports a fixed syndrome except for information about what userspace attempted to do (i.e., WnR -- I dunno if there's anything else), that seems reasonable.

I don't know how we can do that, and I'm deeply sceptical of claims that the WnR bit matters at all for kernel addresses. Any change we make here will be user visible but I don't think that means we shouldn't consider changes for cases that are highly unlikely to cause problems. We'll obviously revert anything that does causes issues, but that shouldn't be the goal.

I'll try to reach out to the ASAN people to get their feedback on this.

If we do want to use a sanitised ESR value, then we need to do this within the constraints of the architecture because Linux advertises this as the ESR when it is provided. What encoding would you suggest? Should we report all faults on kernel addresses as Translation fault level 0?

Quoted 3 lines · Peter Maydell

AFAIK the major thing that consumers actually are after here is the WnR information, so preserving that and sanitizing the rest of the ESR if necessary would be a less risky fix IMHO.

If there is a way of squashing the syndrome information so that it reports a fixed syndrome except for information about what userspace attempted to do (i.e., WnR -- I dunno if there's anything else), that seems reasonable.

Quoted 5 lines · Dave Martin

Quoted 3 lines · Peter Maydell

AFAIK the major thing that consumers actually are after here is the WnR information, so preserving that and sanitizing the rest of the ESR if necessary would be a less risky fix IMHO.

Quoted 4 lines

If there is a way of squashing the syndrome information so that it reports a fixed syndrome except for information about what userspace attempted to do (i.e., WnR -- I dunno if there's anything else), that seems reasonable.

Anything else? For the RAS stuff I planned to use this to indicate whether a SIBGUS due to hwpoison was due to an instruction or data abort, so a sanitised-EC field would be good.

Quoted 7 lines · Will Deacon

User faults on kernel addresses are a good sign that the faulting task is either up to no good or is in deep trouble. In such situations, exposing the optional ESR context on the sigframe as part of the delivered signal is only useful to attackers who are using information about underlying hardware fault (e.g. translation vs permission) as a mechanism to defeat KASLR.

Quoted 2 lines

Remove the ESR context from the sigframe for user faults on kernel addresses.

As this wording suggests, this change causes esr_context to disappear entirely from the signal frame. Previously, I think user code could have relied on its being present for certain signals.

Does Debian's codesearch throw up any nontrivial users of esr_context?