diff mbox

[2/2] Detect instruction fetch denied and report

Message ID 1471831017-18167-2-git-send-email-bsingharora@gmail.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Balbir Singh Aug. 22, 2016, 1:56 a.m. UTC
ISA 3 allows for prevention of instruction fetch and execution
of user mode pages. If such an error occurs, SRR1 bit 35
reports the error. We catch and report the error in do_page_fault()

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/fault.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Aneesh Kumar K.V Aug. 22, 2016, 6:05 a.m. UTC | #1
Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 allows for prevention of instruction fetch and execution
> of user mode pages. If such an error occurs, SRR1 bit 35
> reports the error. We catch and report the error in do_page_fault()
>

But what does the error mean ? A buggy application ? IIUC, it indicate a
buggy kernel isn't it ?. So should we kill the application or panic() ?


> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a4db22f..f162e77 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ good_area:
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +#ifdef CONFIG_PPC_RADIX_MMU
> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> +			goto bad_area;
> +#endif
>  #ifdef CONFIG_PPC_STD_MMU
>  		/*
>  		 * protfault should only happen due to us

-aneesh
Balbir Singh Aug. 22, 2016, 7:55 a.m. UTC | #2
On Mon, Aug 22, 2016 at 11:35:36AM +0530, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
> > ISA 3 allows for prevention of instruction fetch and execution
> > of user mode pages. If such an error occurs, SRR1 bit 35
> > reports the error. We catch and report the error in do_page_fault()
> >
> 
> But what does the error mean ? A buggy application ? IIUC, it indicate a
> buggy kernel isn't it ?. So should we kill the application or panic() ?
>

We oops.. basically we get a kernel fault. We handle it the same way
we handle other kernel faults.
 
> 
> > Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> > ---
> >  arch/powerpc/mm/fault.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index a4db22f..f162e77 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -404,6 +404,10 @@ good_area:
> >  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
> >  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
> >  			goto bad_area;
> > +#ifdef CONFIG_PPC_RADIX_MMU
> > +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> > +			goto bad_area;
> > +#endif
> >  #ifdef CONFIG_PPC_STD_MMU
> >  		/*
> >  		 * protfault should only happen due to us
> 
> -aneesh
>
Michael Ellerman Sept. 20, 2016, 6:35 a.m. UTC | #3
On Mon, 2016-22-08 at 01:56:57 UTC, Balbir Singh wrote:
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index a4db22f..f162e77 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ good_area:
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +#ifdef CONFIG_PPC_RADIX_MMU

We shouldn't need the #ifdef, radix_enabled() will be false.

> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
> +			goto bad_area;

Is it really architected as radix only?

Personally I dislike PPC_BIT(), I'd rather you just used 0x10000000. That way
when I'm staring at a register dump I have some chance of spotting that mask.

Also brackets around the bitwise & would make me feel more comfortable.

cheers
Balbir Singh Sept. 20, 2016, 7:44 a.m. UTC | #4
On 20/09/16 16:35, Michael Ellerman wrote:
> On Mon, 2016-22-08 at 01:56:57 UTC, Balbir Singh wrote:
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index a4db22f..f162e77 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -404,6 +404,10 @@ good_area:
>>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>>  			goto bad_area;
>> +#ifdef CONFIG_PPC_RADIX_MMU
> 
> We shouldn't need the #ifdef, radix_enabled() will be false.
> 
>> +		if (radix_enabled() && regs->msr & PPC_BIT(35))
>> +			goto bad_area;
> 
> Is it really architected as radix only?
> 
> Personally I dislike PPC_BIT(), I'd rather you just used 0x10000000. That way
> when I'm staring at a register dump I have some chance of spotting that mask.
> 
> Also brackets around the bitwise & would make me feel more comfortable.
> 
> cheers
> 


Will make the changes and submit

Balbir
diff mbox

Patch

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a4db22f..f162e77 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -404,6 +404,10 @@  good_area:
 		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
 		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
 			goto bad_area;
+#ifdef CONFIG_PPC_RADIX_MMU
+		if (radix_enabled() && regs->msr & PPC_BIT(35))
+			goto bad_area;
+#endif
 #ifdef CONFIG_PPC_STD_MMU
 		/*
 		 * protfault should only happen due to us