Re: [PATCH 12/24] powerpc/mm: Fix reporting of kernel execute faults
Le 30/11/2018 à 06:50, Aneesh Kumar K.V a écrit : Christophe LEROY writes: Hi Ben, I have an issue on the 8xx with this change Le 19/07/2017 à 06:49, Benjamin Herrenschmidt a écrit : We currently test for is_exec and DSISR_PROTFAULT but that doesn't make sense as this is the wrong error bit to test for an execute permission failure. On the 8xx, on an exec permission failure, this is the correct BIT, see below extract from reference manual: Note that only one of bits 1, 3, and 4 will be set. 1 1 if the translation of an attempted access is not in the translation tables. Otherwise 0 3 1 if the fetch access was to guarded memory when MSR[IR] = 1. Otherwise 0 4 1 if the access is not permitted by the protection mechanism; otherwise 0. So on the 8xx, bit 3 is not DSISR_NOEXEC_OR_G but only DSISR_G. When the PPP bits are set to No-Execute, we really get bit 4 that is DSISR_PROTFAULT. Do you have an url for the document? I am wondering whether we can get Documentation/powerpc/cpu_families.txt updated with these urls? https://www.nxp.com/docs/en/reference-manual/MPC885RM.pdf Christophe In fact, we had code that would return early if we had an exec fault in kernel mode so I think that was just dead code anyway. Finally the location of that test is awkward and prevents further simplifications. So instead move that test into a helper along with the existing early test for kernel exec faults and out of range accesses, and put it all in a "bad_kernel_fault()" helper. While at it test the correct error bits. Signed-off-by: Benjamin Herrenschmidt --- arch/powerpc/mm/fault.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index e8d6acc888c5..aead07cf8a5b 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault) return MM_FAULT_CONTINUE; } +/* Is this a bad kernel fault ? */ +static bool bad_kernel_fault(bool is_exec, unsigned long error_code, +unsigned long address) +{ + if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) { Do you mind if we had DSISR_PROTFAULT here as well ? Christophe + printk_ratelimited(KERN_CRIT "kernel tried to execute" + " exec-protected page (%lx) -" + "exploit attempt? (uid: %d)\n", + address, from_kuid(_user_ns, + current_uid())); + } + return is_exec || (address >= TASK_SIZE); +} + /* * Define the correct "is_write" bit in error_code based * on the processor family @@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * The kernel should never take an execute fault nor should it * take a page fault to a kernel address. */ - if (!is_user && (is_exec || (address >= TASK_SIZE))) + if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address))) return SIGSEGV; /* We restore the interrupt state now */ @@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, return 0; } - if (is_exec && (error_code & DSISR_PROTFAULT)) - printk_ratelimited(KERN_CRIT "kernel tried to execute NX-protected" - " page (%lx) - exploit attempt? (uid: %d)\n", - address, from_kuid(_user_ns, current_uid())); - return SIGSEGV; } NOKPROBE_SYMBOL(__do_page_fault);
Re: [PATCH 12/24] powerpc/mm: Fix reporting of kernel execute faults
On Wed, 2018-11-07 at 09:35 +0100, Christophe LEROY wrote: > Hi Ben, > > I have an issue on the 8xx with this change Ah ouch... .../... > > +/* Is this a bad kernel fault ? */ > > +static bool bad_kernel_fault(bool is_exec, unsigned long error_code, > > +unsigned long address) > > +{ > > + if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) { > > Do you mind if we had DSISR_PROTFAULT here as well ? Off the top of my mind, I don't see a problem with that... but it would definitely require an explanation comment. > > + printk_ratelimited(KERN_CRIT "kernel tried to execute" > > + " exec-protected page (%lx) -" > > + "exploit attempt? (uid: %d)\n", > > + address, from_kuid(_user_ns, > > + current_uid())); > > + } > > + return is_exec || (address >= TASK_SIZE); > > +} > > + > > /* > >* Define the correct "is_write" bit in error_code based > >* on the processor family > > @@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, > > unsigned long address, > > * The kernel should never take an execute fault nor should it > > * take a page fault to a kernel address. > > */ > > - if (!is_user && (is_exec || (address >= TASK_SIZE))) > > + if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, > > address))) > > return SIGSEGV; > > > > /* We restore the interrupt state now */ > > @@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, > > unsigned long address, > > return 0; > > } > > > > - if (is_exec && (error_code & DSISR_PROTFAULT)) > > - printk_ratelimited(KERN_CRIT "kernel tried to execute > > NX-protected" > > - " page (%lx) - exploit attempt? (uid: %d)\n", > > - address, from_kuid(_user_ns, > > current_uid())); > > - > > return SIGSEGV; > > } > > NOKPROBE_SYMBOL(__do_page_fault); > >
Re: [PATCH 12/24] powerpc/mm: Fix reporting of kernel execute faults
Hi Ben, I have an issue on the 8xx with this change Le 19/07/2017 à 06:49, Benjamin Herrenschmidt a écrit : We currently test for is_exec and DSISR_PROTFAULT but that doesn't make sense as this is the wrong error bit to test for an execute permission failure. On the 8xx, on an exec permission failure, this is the correct BIT, see below extract from reference manual: Note that only one of bits 1, 3, and 4 will be set. 1 1 if the translation of an attempted access is not in the translation tables. Otherwise 0 3 1 if the fetch access was to guarded memory when MSR[IR] = 1. Otherwise 0 4 1 if the access is not permitted by the protection mechanism; otherwise 0. So on the 8xx, bit 3 is not DSISR_NOEXEC_OR_G but only DSISR_G. When the PPP bits are set to No-Execute, we really get bit 4 that is DSISR_PROTFAULT. In fact, we had code that would return early if we had an exec fault in kernel mode so I think that was just dead code anyway. Finally the location of that test is awkward and prevents further simplifications. So instead move that test into a helper along with the existing early test for kernel exec faults and out of range accesses, and put it all in a "bad_kernel_fault()" helper. While at it test the correct error bits. Signed-off-by: Benjamin Herrenschmidt --- arch/powerpc/mm/fault.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index e8d6acc888c5..aead07cf8a5b 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault) return MM_FAULT_CONTINUE; } +/* Is this a bad kernel fault ? */ +static bool bad_kernel_fault(bool is_exec, unsigned long error_code, +unsigned long address) +{ + if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) { Do you mind if we had DSISR_PROTFAULT here as well ? Christophe + printk_ratelimited(KERN_CRIT "kernel tried to execute" + " exec-protected page (%lx) -" + "exploit attempt? (uid: %d)\n", + address, from_kuid(_user_ns, + current_uid())); + } + return is_exec || (address >= TASK_SIZE); +} + /* * Define the correct "is_write" bit in error_code based * on the processor family @@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * The kernel should never take an execute fault nor should it * take a page fault to a kernel address. */ - if (!is_user && (is_exec || (address >= TASK_SIZE))) + if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address))) return SIGSEGV; /* We restore the interrupt state now */ @@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, return 0; } - if (is_exec && (error_code & DSISR_PROTFAULT)) - printk_ratelimited(KERN_CRIT "kernel tried to execute NX-protected" - " page (%lx) - exploit attempt? (uid: %d)\n", - address, from_kuid(_user_ns, current_uid())); - return SIGSEGV; } NOKPROBE_SYMBOL(__do_page_fault);
[PATCH 12/24] powerpc/mm: Fix reporting of kernel execute faults
We currently test for is_exec and DSISR_PROTFAULT but that doesn't make sense as this is the wrong error bit to test for an execute permission failure. In fact, we had code that would return early if we had an exec fault in kernel mode so I think that was just dead code anyway. Finally the location of that test is awkward and prevents further simplifications. So instead move that test into a helper along with the existing early test for kernel exec faults and out of range accesses, and put it all in a "bad_kernel_fault()" helper. While at it test the correct error bits. Signed-off-by: Benjamin Herrenschmidt--- arch/powerpc/mm/fault.c | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index e8d6acc888c5..aead07cf8a5b 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault) return MM_FAULT_CONTINUE; } +/* Is this a bad kernel fault ? */ +static bool bad_kernel_fault(bool is_exec, unsigned long error_code, +unsigned long address) +{ + if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) { + printk_ratelimited(KERN_CRIT "kernel tried to execute" + " exec-protected page (%lx) -" + "exploit attempt? (uid: %d)\n", + address, from_kuid(_user_ns, + current_uid())); + } + return is_exec || (address >= TASK_SIZE); +} + /* * Define the correct "is_write" bit in error_code based * on the processor family @@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * The kernel should never take an execute fault nor should it * take a page fault to a kernel address. */ - if (!is_user && (is_exec || (address >= TASK_SIZE))) + if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address))) return SIGSEGV; /* We restore the interrupt state now */ @@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, return 0; } - if (is_exec && (error_code & DSISR_PROTFAULT)) - printk_ratelimited(KERN_CRIT "kernel tried to execute NX-protected" - " page (%lx) - exploit attempt? (uid: %d)\n", - address, from_kuid(_user_ns, current_uid())); - return SIGSEGV; } NOKPROBE_SYMBOL(__do_page_fault); -- 2.13.3