| Message ID | 20190308011619.22402-10-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,01/10] powerpc/powernv/idle: Restore IAMR after idle | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | fail | total: 1 errors, 0 warnings, 0 checks, 80 lines checked |
Le 08/03/2019 à 02:16, Michael Ellerman a écrit : > When KUAP is enabled we have logic to detect page faults that occur > outside of a valid user access region and are blocked by the AMR. > > What we don't have at the moment is logic to detect a fault *within* a > valid user access region, that has been incorrectly blocked by AMR. > This is not meant to ever happen, but it can if we incorrectly > save/restore the AMR, or if the AMR was overwritten for some other > reason. > > Currently if that happens we assume it's just a regular fault that > will be corrected by handling the fault normally, so we just return. > But there is nothing the fault handling code can do to fix it, so the > fault just happens again and we spin forever, leading to soft lockups. > > So add some logic to detect that case and WARN() if we ever see it. > Arguably it should be a BUG(), but it's more polite to fail the access > and let the kernel continue, rather than taking down the box. There > should be no data integrity issue with failing the fault rather than > BUG'ing, as we're just going to disallow an access that should have > been allowed. > > To make the code a little easier to follow, unroll the condition at > the end of bad_kernel_fault() and comment each case, before adding the > call to bad_kuap_fault(). > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > > v5: New. > > .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++ > arch/powerpc/include/asm/kup.h | 1 + > arch/powerpc/mm/fault.c | 25 ++++++++++++++++--- > 3 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h > index 3d60b04fc3f6..8d2ddc61e92e 100644 > --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h > +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h > @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from, > set_kuap(AMR_KUAP_BLOCKED); > } > > +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) > +{ > + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && > + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || > + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) > + { Should this { go on the previous line ? > + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read"); > + return true; Could just be return WARN(true, ....) Or even return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) && ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...); > + } > + > + return false; > +} > #endif /* CONFIG_PPC_KUAP */ > > #endif /* __ASSEMBLY__ */ > diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h > index f79d4d970852..ccbd2a249575 100644 > --- a/arch/powerpc/include/asm/kup.h > +++ b/arch/powerpc/include/asm/kup.h > @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from, > unsigned long size) { } > static inline void allow_read_from_user(const void __user *from, unsigned long size) {} > static inline void allow_write_to_user(void __user *to, unsigned long size) {} > +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; } > #endif /* CONFIG_PPC_KUAP */ > > static inline void prevent_read_from_user(const void __user *from, unsigned long size) > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index 463d1e9d026e..b5d3578d9f65 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -44,6 +44,7 @@ > #include <asm/mmu_context.h> > #include <asm/siginfo.h> > #include <asm/debug.h> > +#include <asm/kup.h> > > static inline bool notify_page_fault(struct pt_regs *regs) > { > @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, > > /* Is this a bad kernel fault ? */ > static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, > - unsigned long address) > + unsigned long address, bool is_write) We have regs, do we need is_write in addition ? Christophe > { > int is_exec = TRAP(regs) == 0x400; > > @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, > address >= TASK_SIZE ? "exec-protected" : "user", > address, > from_kuid(&init_user_ns, current_uid())); > + > + // Kernel exec fault is always bad > + return true; > } > > if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) && > @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, > from_kuid(&init_user_ns, current_uid())); > } > > - return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip); > + // Kernel fault on kernel address is bad > + if (address >= TASK_SIZE) > + return true; > + > + // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad > + if (!search_exception_tables(regs->nip)) > + return true; > + > + // Read/write fault in a valid region (the exception table search passed > + // above), but blocked by KUAP is bad, it can never succeed. > + if (bad_kuap_fault(regs, is_write)) > + return true; > + > + // What's left? Kernel fault on user in well defined regions (extable > + // matched), and allowed by KUAP in the faulting context. > + return false; > } > > static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > * take a page fault to a kernel address or a page fault to a user > * address outside of dedicated places > */ > - if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address))) > + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write))) > return SIGSEGV; > > /* >
Le 08/03/2019 à 09:53, Christophe Leroy a écrit : > > > Le 08/03/2019 à 02:16, Michael Ellerman a écrit : >> When KUAP is enabled we have logic to detect page faults that occur >> outside of a valid user access region and are blocked by the AMR. >> >> What we don't have at the moment is logic to detect a fault *within* a >> valid user access region, that has been incorrectly blocked by AMR. >> This is not meant to ever happen, but it can if we incorrectly >> save/restore the AMR, or if the AMR was overwritten for some other >> reason. >> >> Currently if that happens we assume it's just a regular fault that >> will be corrected by handling the fault normally, so we just return. >> But there is nothing the fault handling code can do to fix it, so the >> fault just happens again and we spin forever, leading to soft lockups. >> >> So add some logic to detect that case and WARN() if we ever see it. >> Arguably it should be a BUG(), but it's more polite to fail the access >> and let the kernel continue, rather than taking down the box. There >> should be no data integrity issue with failing the fault rather than >> BUG'ing, as we're just going to disallow an access that should have >> been allowed. >> >> To make the code a little easier to follow, unroll the condition at >> the end of bad_kernel_fault() and comment each case, before adding the >> call to bad_kuap_fault(). >> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> >> v5: New. >> >> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++ >> arch/powerpc/include/asm/kup.h | 1 + >> arch/powerpc/mm/fault.c | 25 ++++++++++++++++--- >> 3 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h >> b/arch/powerpc/include/asm/book3s/64/kup-radix.h >> index 3d60b04fc3f6..8d2ddc61e92e 100644 >> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h >> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h >> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void >> __user *to, const void __user *from, >> set_kuap(AMR_KUAP_BLOCKED); >> } >> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) >> +{ >> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && >> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || >> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) >> + { > > Should this { go on the previous line ? > >> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? >> "Write" : "Read"); >> + return true; > > Could just be > return WARN(true, ....) > > Or even > return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) && > ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || > (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...); Could also be simplified as follows since (is_write && ...) and (!is_write && ...) are mutually exclusive: mmu_has_feature(MMU_FTR_RADIX_KUAP) && (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)) Christophe > >> + } >> + >> + return false; >> +} >> #endif /* CONFIG_PPC_KUAP */ >> #endif /* __ASSEMBLY__ */ >> diff --git a/arch/powerpc/include/asm/kup.h >> b/arch/powerpc/include/asm/kup.h >> index f79d4d970852..ccbd2a249575 100644 >> --- a/arch/powerpc/include/asm/kup.h >> +++ b/arch/powerpc/include/asm/kup.h >> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user >> *to, const void __user *from, >> unsigned long size) { } >> static inline void allow_read_from_user(const void __user *from, >> unsigned long size) {} >> static inline void allow_write_to_user(void __user *to, unsigned >> long size) {} >> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool >> is_write) { return false; } >> #endif /* CONFIG_PPC_KUAP */ >> static inline void prevent_read_from_user(const void __user *from, >> unsigned long size) >> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c >> index 463d1e9d026e..b5d3578d9f65 100644 >> --- a/arch/powerpc/mm/fault.c >> +++ b/arch/powerpc/mm/fault.c >> @@ -44,6 +44,7 @@ >> #include <asm/mmu_context.h> >> #include <asm/siginfo.h> >> #include <asm/debug.h> >> +#include <asm/kup.h> >> static inline bool notify_page_fault(struct pt_regs *regs) >> { >> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, >> unsigned long addr, >> /* Is this a bad kernel fault ? */ >> static bool bad_kernel_fault(struct pt_regs *regs, unsigned long >> error_code, >> - unsigned long address) >> + unsigned long address, bool is_write) > > We have regs, do we need is_write in addition ? > > Christophe > >> { >> int is_exec = TRAP(regs) == 0x400; >> @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, >> unsigned long error_code, >> address >= TASK_SIZE ? "exec-protected" : "user", >> address, >> from_kuid(&init_user_ns, current_uid())); >> + >> + // Kernel exec fault is always bad >> + return true; >> } >> if (!is_exec && address < TASK_SIZE && (error_code & >> DSISR_PROTFAULT) && >> @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs >> *regs, unsigned long error_code, >> from_kuid(&init_user_ns, current_uid())); >> } >> - return is_exec || (address >= TASK_SIZE) || >> !search_exception_tables(regs->nip); >> + // Kernel fault on kernel address is bad >> + if (address >= TASK_SIZE) >> + return true; >> + >> + // Fault on user outside of certain regions (eg. >> copy_tofrom_user()) is bad >> + if (!search_exception_tables(regs->nip)) >> + return true; >> + >> + // Read/write fault in a valid region (the exception table search >> passed >> + // above), but blocked by KUAP is bad, it can never succeed. >> + if (bad_kuap_fault(regs, is_write)) >> + return true; >> + >> + // What's left? Kernel fault on user in well defined regions >> (extable >> + // matched), and allowed by KUAP in the faulting context. >> + return false; >> } >> static bool bad_stack_expansion(struct pt_regs *regs, unsigned long >> address, >> @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, >> unsigned long address, >> * take a page fault to a kernel address or a page fault to a user >> * address outside of dedicated places >> */ >> - if (unlikely(!is_user && bad_kernel_fault(regs, error_code, >> address))) >> + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, >> address, is_write))) >> return SIGSEGV; >> /* >> --- L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast. https://www.avast.com/antivirus
Christophe Leroy <christophe.leroy@c-s.fr> writes: > Le 08/03/2019 à 02:16, Michael Ellerman a écrit : >> When KUAP is enabled we have logic to detect page faults that occur >> outside of a valid user access region and are blocked by the AMR. >> >> What we don't have at the moment is logic to detect a fault *within* a >> valid user access region, that has been incorrectly blocked by AMR. >> This is not meant to ever happen, but it can if we incorrectly >> save/restore the AMR, or if the AMR was overwritten for some other >> reason. >> >> Currently if that happens we assume it's just a regular fault that >> will be corrected by handling the fault normally, so we just return. >> But there is nothing the fault handling code can do to fix it, so the >> fault just happens again and we spin forever, leading to soft lockups. >> >> So add some logic to detect that case and WARN() if we ever see it. >> Arguably it should be a BUG(), but it's more polite to fail the access >> and let the kernel continue, rather than taking down the box. There >> should be no data integrity issue with failing the fault rather than >> BUG'ing, as we're just going to disallow an access that should have >> been allowed. >> >> To make the code a little easier to follow, unroll the condition at >> the end of bad_kernel_fault() and comment each case, before adding the >> call to bad_kuap_fault(). >> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> >> --- >> >> v5: New. >> >> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++ >> arch/powerpc/include/asm/kup.h | 1 + >> arch/powerpc/mm/fault.c | 25 ++++++++++++++++--- >> 3 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h >> index 3d60b04fc3f6..8d2ddc61e92e 100644 >> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h >> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h >> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from, >> set_kuap(AMR_KUAP_BLOCKED); >> } >> >> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) >> +{ >> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && >> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || >> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) { > > Should this { go on the previous line ? Yeah I guess. >> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read"); >> + return true; > > Could just be > return WARN(true, ....) > > Or even > return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) && > ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || > (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...); That's not any more readable IMO. >> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h >> index f79d4d970852..ccbd2a249575 100644 >> --- a/arch/powerpc/include/asm/kup.h >> +++ b/arch/powerpc/include/asm/kup.h >> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from, >> unsigned long size) { } >> static inline void allow_read_from_user(const void __user *from, unsigned long size) {} >> static inline void allow_write_to_user(void __user *to, unsigned long size) {} >> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; } >> #endif /* CONFIG_PPC_KUAP */ >> >> static inline void prevent_read_from_user(const void __user *from, unsigned long size) >> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c >> index 463d1e9d026e..b5d3578d9f65 100644 >> --- a/arch/powerpc/mm/fault.c >> +++ b/arch/powerpc/mm/fault.c >> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, >> >> /* Is this a bad kernel fault ? */ >> static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, >> - unsigned long address) >> + unsigned long address, bool is_write) > > We have regs, do we need is_write in addition ? It comes from error_code, which we also have. But I don't see any harm passing it as we already have it calculated and sitting in a GPR. cheers
christophe leroy <christophe.leroy@c-s.fr> writes: > Le 08/03/2019 à 09:53, Christophe Leroy a écrit : >> Le 08/03/2019 à 02:16, Michael Ellerman a écrit : ... >>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h >>> b/arch/powerpc/include/asm/book3s/64/kup-radix.h >>> index 3d60b04fc3f6..8d2ddc61e92e 100644 >>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h >>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h >>> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void >>> __user *to, const void __user *from, >>> set_kuap(AMR_KUAP_BLOCKED); >>> } >>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) >>> +{ >>> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && >>> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || >>> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) >>> + { >> >> Should this { go on the previous line ? >> >>> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? >>> "Write" : "Read"); >>> + return true; >> >> Could just be >> return WARN(true, ....) >> >> Or even >> return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) && >> ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || >> (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...); > > Could also be simplified as follows since (is_write && ...) and > (!is_write && ...) are mutually exclusive: > > mmu_has_feature(MMU_FTR_RADIX_KUAP) && > (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)) Yeah I guess that is better. I'll do: static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) && (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)), "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read"); } cheers
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h index 3d60b04fc3f6..8d2ddc61e92e 100644 --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from, set_kuap(AMR_KUAP_BLOCKED); } +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) +{ + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) && + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) || + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) + { + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read"); + return true; + } + + return false; +} #endif /* CONFIG_PPC_KUAP */ #endif /* __ASSEMBLY__ */ diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h index f79d4d970852..ccbd2a249575 100644 --- a/arch/powerpc/include/asm/kup.h +++ b/arch/powerpc/include/asm/kup.h @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from, unsigned long size) { } static inline void allow_read_from_user(const void __user *from, unsigned long size) {} static inline void allow_write_to_user(void __user *to, unsigned long size) {} +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; } #endif /* CONFIG_PPC_KUAP */ static inline void prevent_read_from_user(const void __user *from, unsigned long size) diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index 463d1e9d026e..b5d3578d9f65 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -44,6 +44,7 @@ #include <asm/mmu_context.h> #include <asm/siginfo.h> #include <asm/debug.h> +#include <asm/kup.h> static inline bool notify_page_fault(struct pt_regs *regs) { @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, /* Is this a bad kernel fault ? */ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, - unsigned long address) + unsigned long address, bool is_write) { int is_exec = TRAP(regs) == 0x400; @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, address >= TASK_SIZE ? "exec-protected" : "user", address, from_kuid(&init_user_ns, current_uid())); + + // Kernel exec fault is always bad + return true; } if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) && @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code, from_kuid(&init_user_ns, current_uid())); } - return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip); + // Kernel fault on kernel address is bad + if (address >= TASK_SIZE) + return true; + + // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad + if (!search_exception_tables(regs->nip)) + return true; + + // Read/write fault in a valid region (the exception table search passed + // above), but blocked by KUAP is bad, it can never succeed. + if (bad_kuap_fault(regs, is_write)) + return true; + + // What's left? Kernel fault on user in well defined regions (extable + // matched), and allowed by KUAP in the faulting context. + return false; } static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * take a page fault to a kernel address or a page fault to a user * address outside of dedicated places */ - if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address))) + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write))) return SIGSEGV; /*
When KUAP is enabled we have logic to detect page faults that occur outside of a valid user access region and are blocked by the AMR. What we don't have at the moment is logic to detect a fault *within* a valid user access region, that has been incorrectly blocked by AMR. This is not meant to ever happen, but it can if we incorrectly save/restore the AMR, or if the AMR was overwritten for some other reason. Currently if that happens we assume it's just a regular fault that will be corrected by handling the fault normally, so we just return. But there is nothing the fault handling code can do to fix it, so the fault just happens again and we spin forever, leading to soft lockups. So add some logic to detect that case and WARN() if we ever see it. Arguably it should be a BUG(), but it's more polite to fail the access and let the kernel continue, rather than taking down the box. There should be no data integrity issue with failing the fault rather than BUG'ing, as we're just going to disallow an access that should have been allowed. To make the code a little easier to follow, unroll the condition at the end of bad_kernel_fault() and comment each case, before adding the call to bad_kuap_fault(). Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- v5: New. .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++ arch/powerpc/include/asm/kup.h | 1 + arch/powerpc/mm/fault.c | 25 ++++++++++++++++--- 3 files changed, 35 insertions(+), 3 deletions(-)