[PATCH]forbid ptrace changes psr.ri to 3

[PATCH]forbid ptrace changes psr.ri to 3

[Shaohua Li]

PSR.ri bits 3 are reserved, change to 3 will break kernel.

The patch ignore such change, if you thought access_urea should return
an error in the case, please let me know.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

--- a/arch/ia64/kernel/ptrace.c	2007-08-27 05:48:49.000000000 +0800
+++ b/arch/ia64/kernel/ptrace.c	2007-08-27 05:57:42.000000000 +0800
@@ -951,10 +951,14 @@ access_uarea (struct task_struct *child,
 			return 0;
 
 		      case PT_CR_IPSR:
-			if (write_access)
-				pt->cr_ipsr = ((*data & IPSR_MASK)
+			if (write_access) {
+				unsigned long tmp = *data;
+				/* psr.ri bits 11 are reserved, ingore the change */
+				if ((tmp & IA64_PSR_RI) = IA64_PSR_RI)
+					tmp &= ~IA64_PSR_RI;
+				pt->cr_ipsr = ((tmp & IPSR_MASK)
 					       | (pt->cr_ipsr & ~IPSR_MASK));
-			else
+			} else
 				*data = (pt->cr_ipsr & IPSR_MASK);
 			return 0;
 

Re: [PATCH]forbid ptrace changes psr.ri to 3

[Roland McGrath]

IA64_PSR_RI is in IPSR_MASK, apparently by intent.  
If you want to change the behavior, you should simply change that mask.


Thanks,
Roland

RE: [PATCH]forbid ptrace changes psr.ri to 3

[Luck, Tony]

> IA64_PSR_RI is in IPSR_MASK, apparently by intent.  
> If you want to change the behavior, you should simply change that mask.

psr.ri is more complex because it is two bits where not all of the
possible bit settings are defined. We definitely want to allow ptrace
to set psr.ri (how else can we single step through instructions in
a bundle?)  But we should only allow psr.ri to be set to 0, 1, or 2.

I don't see how changing IPSR_MASK can help solve this.

-Tony

Re: [PATCH]forbid ptrace changes psr.ri to 3

[Bjorn Helgaas]

On Friday 17 August 2007 11:21:14 am Luck, Tony wrote:
> > IA64_PSR_RI is in IPSR_MASK, apparently by intent.  
> > If you want to change the behavior, you should simply change that mask.
> 
> psr.ri is more complex because it is two bits where not all of the
> possible bit settings are defined. We definitely want to allow ptrace
> to set psr.ri (how else can we single step through instructions in
> a bundle?)  But we should only allow psr.ri to be set to 0, 1, or 2.

This confused me, too, because the changelog and comments say
"PSR.ri bits 3 are reserved" and "psr.ri bits 11 are reserved".

Something like "PSR.ri value 3 is reserved" would have made
the intent more clear.

Bjorn

RE: [PATCH]forbid ptrace changes psr.ri to 3

[Luck, Tony]

> This confused me, too, because the changelog and comments say
> "PSR.ri bits 3 are reserved" and "psr.ri bits 11 are reserved".
>
> Something like "PSR.ri value 3 is reserved" would have made
> the intent more clear.

Is this better?  I'd already re-worded it a bit when I was checking
into my GIT tree yesterday ... but I can make more changes if this
is still causing confusion.

commit f8655358b7a6f527a5b102cb7d36b4eb14414860
Author: Shaohua Li <shaohua.li@intel.com>
Date:   Thu Aug 16 10:47:05 2007 -0700

    [IA64] forbid ptrace changes psr.ri to 3
    
    The "ri" field in the processor status register only has defined
    values of 0, 1, 2.  Do not let ptrace set this to 3.  As with
    other reserved fields in registers we silently discard the value.
    
    Signed-off-by: Shaohua Li <shaohua.li@intel.com>
    Signed-off-by: Tony Luck <tony.luck@intel.com>

diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
index 00f8032..da031f8 100644
--- a/arch/ia64/kernel/ptrace.c
+++ b/arch/ia64/kernel/ptrace.c
@@ -951,10 +951,14 @@ access_uarea (struct task_struct *child, unsigned long addr,
 			return 0;
 
 		      case PT_CR_IPSR:
-			if (write_access)
-				pt->cr_ipsr = ((*data & IPSR_MASK)
+			if (write_access) {
+				unsigned long tmp = *data;
+				/* psr.ri bits 11 are reserved, ignore the change */
+				if ((tmp & IA64_PSR_RI) = IA64_PSR_RI)
+					tmp &= ~IA64_PSR_RI;
+				pt->cr_ipsr = ((tmp & IPSR_MASK)
 					       | (pt->cr_ipsr & ~IPSR_MASK));
-			else
+			} else
 				*data = (pt->cr_ipsr & IPSR_MASK);
 			return 0;
 

Re: [PATCH]forbid ptrace changes psr.ri to 3

[Bjorn Helgaas]

On Friday 17 August 2007 12:44:58 pm Luck, Tony wrote:
> > This confused me, too, because the changelog and comments say
> > "PSR.ri bits 3 are reserved" and "psr.ri bits 11 are reserved".
> >
> > Something like "PSR.ri value 3 is reserved" would have made
> > the intent more clear.
> 
> Is this better?  I'd already re-worded it a bit when I was checking
> into my GIT tree yesterday ... but I can make more changes if this
> is still causing confusion.

I like the changelog a lot, but you might touch up the comment in
the code as well.
> 
> commit f8655358b7a6f527a5b102cb7d36b4eb14414860
> Author: Shaohua Li <shaohua.li@intel.com>
> Date:   Thu Aug 16 10:47:05 2007 -0700
> 
>     [IA64] forbid ptrace changes psr.ri to 3
>     
>     The "ri" field in the processor status register only has defined
>     values of 0, 1, 2.  Do not let ptrace set this to 3.  As with
>     other reserved fields in registers we silently discard the value.
>     
>     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>     Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
> index 00f8032..da031f8 100644
> --- a/arch/ia64/kernel/ptrace.c
> +++ b/arch/ia64/kernel/ptrace.c
> @@ -951,10 +951,14 @@ access_uarea (struct task_struct *child, unsigned long addr,
>  			return 0;
>  
>  		      case PT_CR_IPSR:
> -			if (write_access)
> -				pt->cr_ipsr = ((*data & IPSR_MASK)
> +			if (write_access) {
> +				unsigned long tmp = *data;
> +				/* psr.ri bits 11 are reserved, ignore the change */
> +				if ((tmp & IA64_PSR_RI) = IA64_PSR_RI)
> +					tmp &= ~IA64_PSR_RI;
> +				pt->cr_ipsr = ((tmp & IPSR_MASK)
>  					       | (pt->cr_ipsr & ~IPSR_MASK));
> -			else
> +			} else
>  				*data = (pt->cr_ipsr & IPSR_MASK);
>  			return 0;
>  
>