[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Janboe Ye]

Using OABI, do_signal need to copy restart_syscall to user stack.
It is possible that put_user fail. This triggers flush_icache page
fault and crash kernel.

Signed-off-by: janboe <janboe.ye@gmail.com>
---
 arch/arm/kernel/signal.c |   21 +++++++++++++--------
 1 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index f6bc5d4..2122d43 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -626,6 +626,7 @@ static int do_signal(sigset_t *oldset, struct pt_regs *regs, int syscall)
 	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
+	int ret = 0;
 
 	/*
 	 * We want the common case to go fast, which
@@ -679,16 +680,20 @@ static int do_signal(sigset_t *oldset, struct pt_regs *regs, int syscall)
 				 */
 				swival = swival - __NR_SYSCALL_BASE + __NR_OABI_SYSCALL_BASE;
 
-				put_user(regs->ARM_pc, &usp[0]);
+				ret |= put_user(regs->ARM_pc, &usp[0]);
 				/* swi __NR_restart_syscall */
-				put_user(0xef000000 | swival, &usp[1]);
+				ret |= put_user(0xef000000 | swival, &usp[1]);
 				/* ldr	pc, [sp], #12 */
-				put_user(0xe49df00c, &usp[2]);
-
-				flush_icache_range((unsigned long)usp,
-						   (unsigned long)(usp + 3));
-
-				regs->ARM_pc = regs->ARM_sp + 4;
+				ret |= put_user(0xe49df00c, &usp[2]);
+				if (!ret) {
+					flush_icache_range((unsigned long)usp,
+						(unsigned long)(usp + 3));
+
+					regs->ARM_pc = regs->ARM_sp + 4;
+				} else {
+					regs->ARM_sp += 12;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 
1.6.3.3

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[ye janboe]

hi,
No one think that it is possible that put_user could fail in some
situation even if currently kernel subtract 12 from ARM_sp and cross
page bound?

Thanks

Janboe

2009/10/21 Janboe Ye <yuan-bo.ye@motorola.com>:
> Using OABI, do_signal need to copy restart_syscall to user stack.
> It is possible that put_user fail. This triggers flush_icache page
> fault and crash kernel.
>
> Signed-off-by: janboe <janboe.ye@gmail.com>
> ---
> ?arch/arm/kernel/signal.c | ? 21 +++++++++++++--------
> ?1 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index f6bc5d4..2122d43 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -626,6 +626,7 @@ static int do_signal(sigset_t *oldset, struct pt_regs *regs, int syscall)
> ? ? ? ?struct k_sigaction ka;
> ? ? ? ?siginfo_t info;
> ? ? ? ?int signr;
> + ? ? ? int ret = 0;
>
> ? ? ? ?/*
> ? ? ? ? * We want the common case to go fast, which
> @@ -679,16 +680,20 @@ static int do_signal(sigset_t *oldset, struct pt_regs *regs, int syscall)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?swival = swival - __NR_SYSCALL_BASE + __NR_OABI_SYSCALL_BASE;
>
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? put_user(regs->ARM_pc, &usp[0]);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret |= put_user(regs->ARM_pc, &usp[0]);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* swi __NR_restart_syscall */
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? put_user(0xef000000 | swival, &usp[1]);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret |= put_user(0xef000000 | swival, &usp[1]);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?/* ldr ?pc, [sp], #12 */
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? put_user(0xe49df00c, &usp[2]);
> -
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? flush_icache_range((unsigned long)usp,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(unsigned long)(usp + 3));
> -
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? regs->ARM_pc = regs->ARM_sp + 4;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret |= put_user(0xe49df00c, &usp[2]);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (!ret) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? flush_icache_range((unsigned long)usp,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (unsigned long)(usp + 3));
> +
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? regs->ARM_pc = regs->ARM_sp + 4;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? } else {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? regs->ARM_sp += 12;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? force_sigsegv(0, current);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
> ?#endif
> ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?}
> --
> 1.6.3.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Thu, Oct 22, 2009 at 11:35:22AM +0800, ye janboe wrote:
> hi,
> No one think that it is possible that put_user could fail in some
> situation even if currently kernel subtract 12 from ARM_sp and cross
> page bound?

It is possible, and it's something we should fix.  Note there's another
fix in this area (I've just posted) which will make your patch invalid.
Please wait until we've sorted out this other patch.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[ye janboe]

hi, Russell

Your patch looks good for real fixing this issue.


2009/10/24 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Thu, Oct 22, 2009 at 11:35:22AM +0800, ye janboe wrote:
>> hi,
>> No one think that it is possible that put_user could fail in some
>> situation even if currently kernel subtract 12 from ARM_sp and cross
>> page bound?
>
> It is possible, and it's something we should fix. ?Note there's another
> fix in this area (I've just posted) which will make your patch invalid.
> Please wait until we've sorted out this other patch.
>

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Sun, Oct 25, 2009 at 02:23:09PM +0800, ye janboe wrote:
> hi, Russell
> 
> Your patch looks good for real fixing this issue.

My patch doesn't address your issue, it only addresses a previous
problem.  We're still decrementing the stack pointer and writing to
a possible new page - which could fault.

Therefore, your patch is still required, but it needs to be modified
to apply on top of my patch.

A replacement patch would be welcome.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[ye janboe]

Sure. Russell

After you restart_syscall patch are merged, I will send out another patch.

Thanks

2009/10/25 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sun, Oct 25, 2009 at 02:23:09PM +0800, ye janboe wrote:
>> hi, Russell
>>
>> Your patch looks good for real fixing this issue.
>
> My patch doesn't address your issue, it only addresses a previous
> problem. ?We're still decrementing the stack pointer and writing to
> a possible new page - which could fault.
>
> Therefore, your patch is still required, but it needs to be modified
> to apply on top of my patch.
>
> A replacement patch would be welcome.
>

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

Russell, Janboe,

On Monday 26 October 2009 03:59:23 ye janboe wrote:
> Sure. Russell
>
> After you restart_syscall patch are merged, I will send out another patch.
Here is what I ended up with after merging.
It has been tested on OMAP3 boards although I could not trigger the failing 
put_user test case.

What do you think?

Regards,
Jean


>From dcaf0812f86948fe1c9dbb9fe53e9040f5a6deeb Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Tue, 27 Oct 2009 10:09:22 +0100
Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT

Using OABI, do_signal need to copy restart_syscall to user stack.
It is possible that put_user fail. This triggers flush_icache page
fault and crash kernel.

Signed-off-by: janboe <janboe.ye@gmail.com>

Merged from
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
on top of
http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2

Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
suspend/resume.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 arch/arm/kernel/signal.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 998fa8d..8ac56fe 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -643,6 +643,7 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
+	int ret = 0;
 
 	/*
 	 * We want the common case to go fast, which
@@ -685,8 +686,15 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 				regs->ARM_sp -= 4;
 				usp = (u32 __user *)regs->ARM_sp;
 
-				put_user(regs->ARM_pc, usp);
-				regs->ARM_pc = KERN_RESTART_CODE;
+				ret |= put_user(regs->ARM_pc, usp);
+				if (!ret) {
+					flush_icache_range((unsigned long)usp,
+						(unsigned long)(usp + 1));
+					regs->ARM_pc = KERN_RESTART_CODE;
+				} else {
+					regs->ARM_sp += 4;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 

>
> Thanks
>
> 2009/10/25 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Sun, Oct 25, 2009 at 02:23:09PM +0800, ye janboe wrote:
> >> hi, Russell
> >>
> >> Your patch looks good for real fixing this issue.
> >
> > My patch doesn't address your issue, it only addresses a previous
> > problem. ?We're still decrementing the stack pointer and writing to
> > a possible new page - which could fault.
> >
> > Therefore, your patch is still required, but it needs to be modified
> > to apply on top of my patch.
> >
> > A replacement patch would be welcome.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Tue, Oct 27, 2009 at 03:14:53PM +0100, Jean Pihet wrote:
> Russell, Janboe,
> 
> On Monday 26 October 2009 03:59:23 ye janboe wrote:
> > Sure. Russell
> >
> > After you restart_syscall patch are merged, I will send out another patch.
> Here is what I ended up with after merging.
> It has been tested on OMAP3 boards although I could not trigger the failing 
> put_user test case.
> 
> What do you think?

Looks fine, thanks.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[ye janboe]

looks fine too. Thanks

2009/10/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Tue, Oct 27, 2009 at 03:14:53PM +0100, Jean Pihet wrote:
>> Russell, Janboe,
>>
>> On Monday 26 October 2009 03:59:23 ye janboe wrote:
>> > Sure. Russell
>> >
>> > After you restart_syscall patch are merged, I will send out another patch.
>> Here is what I ended up with after merging.
>> It has been tested on OMAP3 boards although I could not trigger the failing
>> put_user test case.
>>
>> What do you think?
>
> Looks fine, thanks.
>

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Mikael Pettersson]

Jean Pihet writes:
 > --- a/arch/arm/kernel/signal.c
 > +++ b/arch/arm/kernel/signal.c
 > @@ -643,6 +643,7 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
 > *regs, int syscall)
 >  	struct k_sigaction ka;
 >  	siginfo_t info;
 >  	int signr;
 > +	int ret = 0;
 >  
 >  	/*
 >  	 * We want the common case to go fast, which
 > @@ -685,8 +686,15 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
 > *regs, int syscall)
 >  				regs->ARM_sp -= 4;
 >  				usp = (u32 __user *)regs->ARM_sp;
 >  
 > -				put_user(regs->ARM_pc, usp);
 > -				regs->ARM_pc = KERN_RESTART_CODE;
 > +				ret |= put_user(regs->ARM_pc, usp);
 > +				if (!ret) {
 > +					flush_icache_range((unsigned long)usp,
 > +						(unsigned long)(usp + 1));
 > +					regs->ARM_pc = KERN_RESTART_CODE;
 > +				} else {
 > +					regs->ARM_sp += 4;
 > +					force_sigsegv(0, current);
 > +				}
 >  #endif

I don't like this 'ret' variable:
- it's only for OABI, but declared at the top and initialised also for EABI
- you update it with |=, but clearly there's no useful previous value

I'd drop 'ret' and just do:

				if (put_user(regs->ARM_pc, usp) == 0) {
					flush...
				} else {
					... force_sigsegv
				}

(Note: == 0 not !, since ! is often read as "failed" whereas for
put_user() 0 means Ok and non-zero means -errno.)

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

Hi,

On Tuesday 27 October 2009 16:42:43 Mikael Pettersson wrote:
...
>
> I don't like this 'ret' variable:
> - it's only for OABI, but declared at the top and initialised also for EABI
> - you update it with |=, but clearly there's no useful previous value
>
> I'd drop 'ret' and just do:
>
> 				if (put_user(regs->ARM_pc, usp) == 0) {
> 					flush...
> 				} else {
> 					... force_sigsegv
> 				}
>
> (Note: == 0 not !, since ! is often read as "failed" whereas for
> put_user() 0 means Ok and non-zero means -errno.)
Yes that improves the code readability. Thanks for the suggestion.

Here is an updated patch.

Regards,
Jean


>From 885a5952dd38bc0c023e52f731ea2681d43132ba Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Tue, 27 Oct 2009 10:09:22 +0100
Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT

Using OABI, do_signal need to copy restart_syscall to user stack.
It is possible that put_user fail. This triggers flush_icache page
fault and crash kernel.

Signed-off-by: janboe <janboe.ye@gmail.com>

Merged from
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
on top of
http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2

Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
suspend/resume.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 arch/arm/kernel/signal.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index f330974..4366cc0 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -676,8 +676,14 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 				regs->ARM_sp -= 4;
 				usp = (u32 __user *)regs->ARM_sp;
 
-				put_user(regs->ARM_pc, usp);
-				regs->ARM_pc = KERN_RESTART_CODE;
+				if (put_user(regs->ARM_pc, usp) == 0) {
+					flush_icache_range((unsigned long)usp,
+						(unsigned long)(usp + 1));
+					regs->ARM_pc = KERN_RESTART_CODE;
+				} else {
+					regs->ARM_sp += 4;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 
1.6.2.5.168.g3823

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Tue, 27 Oct 2009, Jean Pihet wrote:

> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index f330974..4366cc0 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -676,8 +676,14 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
> *regs, int syscall)
>  				regs->ARM_sp -= 4;
>  				usp = (u32 __user *)regs->ARM_sp;
>  
> -				put_user(regs->ARM_pc, usp);
> -				regs->ARM_pc = KERN_RESTART_CODE;
> +				if (put_user(regs->ARM_pc, usp) == 0) {
> +					flush_icache_range((unsigned long)usp,
> +						(unsigned long)(usp + 1));

Why are you flushing the icache?  There is no code on the stack anymore.


Nicolas

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

On Tuesday 27 October 2009 19:08:07 Nicolas Pitre wrote:
> On Tue, 27 Oct 2009, Jean Pihet wrote:
> > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > index f330974..4366cc0 100644
> > --- a/arch/arm/kernel/signal.c
> > +++ b/arch/arm/kernel/signal.c
> > @@ -676,8 +676,14 @@ static int do_signal(sigset_t *oldset, struct
> > pt_regs *regs, int syscall)
> >  				regs->ARM_sp -= 4;
> >  				usp = (u32 __user *)regs->ARM_sp;
> >
> > -				put_user(regs->ARM_pc, usp);
> > -				regs->ARM_pc = KERN_RESTART_CODE;
> > +				if (put_user(regs->ARM_pc, usp) == 0) {
> > +					flush_icache_range((unsigned long)usp,
> > +						(unsigned long)(usp + 1));
>
> Why are you flushing the icache?  There is no code on the stack anymore.
Yes indeed there is no more code modified.
Side question: does the put_user requires a flush of some sort? If not, why?

Is it OK to re-send a patch with the call to flush_icache_range removed?

Thanks!
> Nicolas

Jean

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Tue, 27 Oct 2009, Jean Pihet wrote:

> On Tuesday 27 October 2009 19:08:07 Nicolas Pitre wrote:
> > On Tue, 27 Oct 2009, Jean Pihet wrote:
> > > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > > index f330974..4366cc0 100644
> > > --- a/arch/arm/kernel/signal.c
> > > +++ b/arch/arm/kernel/signal.c
> > > @@ -676,8 +676,14 @@ static int do_signal(sigset_t *oldset, struct
> > > pt_regs *regs, int syscall)
> > >  				regs->ARM_sp -= 4;
> > >  				usp = (u32 __user *)regs->ARM_sp;
> > >
> > > -				put_user(regs->ARM_pc, usp);
> > > -				regs->ARM_pc = KERN_RESTART_CODE;
> > > +				if (put_user(regs->ARM_pc, usp) == 0) {
> > > +					flush_icache_range((unsigned long)usp,
> > > +						(unsigned long)(usp + 1));
> >
> > Why are you flushing the icache?  There is no code on the stack anymore.
> Yes indeed there is no more code modified.
> Side question: does the put_user requires a flush of some sort? If not, why?

No because it stores data into the d-cache directly at the virtual 
address to be used by user space.  Previously the d-cache needed to 
be cleaned for data to hit main memory and the i-cache invalidated for 
the newly stored _code_ to be seen by the instruction path.  Since there 
is no code involved anymore the cache flushes are useless.

> Is it OK to re-send a patch with the call to flush_icache_range removed?

Yes.


Nicolas

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

Nicolas,

On Tuesday 27 October 2009 19:59:36 Nicolas Pitre wrote:
...
> > Side question: does the put_user requires a flush of some sort? If not,
> > why?
>
> No because it stores data into the d-cache directly at the virtual
> address to be used by user space.  Previously the d-cache needed to
> be cleaned for data to hit main memory and the i-cache invalidated for
> the newly stored _code_ to be seen by the instruction path.  Since there
> is no code involved anymore the cache flushes are useless.
>
> > Is it OK to re-send a patch with the call to flush_icache_range removed?
>
> Yes.
Ok here is the updated patch. Let's hope it is the good one that time ;-)

Can it be merged? It applies cleanly on top of Russell's latest patch 
(http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2).
>
>
> Nicolas

Regards,
Jean

---
>From d862e03679f7ea44fca1bf1ea256bbade3fbe76a Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Tue, 27 Oct 2009 10:09:22 +0100
Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT

Using OABI, do_signal need to copy restart_syscall to user stack.
It is possible that put_user fail. This triggers flush_icache page
fault and crash kernel.

Signed-off-by: janboe <janboe.ye@gmail.com>

Merged from
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
on top of
http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2

Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
suspend/resume.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 arch/arm/kernel/signal.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index f330974..ea9722a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 				regs->ARM_sp -= 4;
 				usp = (u32 __user *)regs->ARM_sp;
 
-				put_user(regs->ARM_pc, usp);
-				regs->ARM_pc = KERN_RESTART_CODE;
+				if (put_user(regs->ARM_pc, usp) == 0) {
+					regs->ARM_pc = KERN_RESTART_CODE;
+				} else {
+					regs->ARM_sp += 4;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 
1.6.2.5.168.g3823

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Tue, 27 Oct 2009, Jean Pihet wrote:

> Nicolas,
> 
> On Tuesday 27 October 2009 19:59:36 Nicolas Pitre wrote:
> ...
> > > Side question: does the put_user requires a flush of some sort? If not,
> > > why?
> >
> > No because it stores data into the d-cache directly at the virtual
> > address to be used by user space.  Previously the d-cache needed to
> > be cleaned for data to hit main memory and the i-cache invalidated for
> > the newly stored _code_ to be seen by the instruction path.  Since there
> > is no code involved anymore the cache flushes are useless.
> >
> > > Is it OK to re-send a patch with the call to flush_icache_range removed?
> >
> > Yes.
> Ok here is the updated patch. Let's hope it is the good one that time ;-)

Well, the commit message is certainly wrong. There is no copying of 
restart_syscall to user stack anymore.

And is there a reason for touching sp in the error case?




> 
> Can it be merged? It applies cleanly on top of Russell's latest patch 
> (http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2).
> >
> >
> > Nicolas
> 
> Regards,
> Jean
> 
> ---
> From d862e03679f7ea44fca1bf1ea256bbade3fbe76a Mon Sep 17 00:00:00 2001
> From: Jean Pihet <jpihet@mvista.com>
> Date: Tue, 27 Oct 2009 10:09:22 +0100
> Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT
> 
> Using OABI, do_signal need to copy restart_syscall to user stack.
> It is possible that put_user fail. This triggers flush_icache page
> fault and crash kernel.
> 
> Signed-off-by: janboe <janboe.ye@gmail.com>
> 
> Merged from
> http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
> on top of
> http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2
> 
> Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
> suspend/resume.
> 
> Signed-off-by: Jean Pihet <jpihet@mvista.com>
> ---
>  arch/arm/kernel/signal.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index f330974..ea9722a 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
> *regs, int syscall)
>  				regs->ARM_sp -= 4;
>  				usp = (u32 __user *)regs->ARM_sp;
>  
> -				put_user(regs->ARM_pc, usp);
> -				regs->ARM_pc = KERN_RESTART_CODE;
> +				if (put_user(regs->ARM_pc, usp) == 0) {
> +					regs->ARM_pc = KERN_RESTART_CODE;
> +				} else {
> +					regs->ARM_sp += 4;
> +					force_sigsegv(0, current);
> +				}
>  #endif
>  			}
>  		}
> -- 
> 1.6.2.5.168.g3823
> 
> 

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Tue, Oct 27, 2009 at 03:35:31PM -0400, Nicolas Pitre wrote:
> On Tue, 27 Oct 2009, Jean Pihet wrote:
> 
> > Nicolas,
> > 
> > On Tuesday 27 October 2009 19:59:36 Nicolas Pitre wrote:
> > ...
> > > > Side question: does the put_user requires a flush of some sort? If not,
> > > > why?
> > >
> > > No because it stores data into the d-cache directly at the virtual
> > > address to be used by user space.  Previously the d-cache needed to
> > > be cleaned for data to hit main memory and the i-cache invalidated for
> > > the newly stored _code_ to be seen by the instruction path.  Since there
> > > is no code involved anymore the cache flushes are useless.
> > >
> > > > Is it OK to re-send a patch with the call to flush_icache_range removed?
> > >
> > > Yes.
> > Ok here is the updated patch. Let's hope it is the good one that time ;-)
> 
> Well, the commit message is certainly wrong. There is no copying of 
> restart_syscall to user stack anymore.
> 
> And is there a reason for touching sp in the error case?

Returning 'sp' seems to be a sane thing to do - otherwise debuggers
analysing core dumps could be confused about the location of data on
the stack.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Tue, 27 Oct 2009, Russell King - ARM Linux wrote:

> On Tue, Oct 27, 2009 at 03:35:31PM -0400, Nicolas Pitre wrote:
> > And is there a reason for touching sp in the error case?
> 
> Returning 'sp' seems to be a sane thing to do - otherwise debuggers
> analysing core dumps could be confused about the location of data on
> the stack.

Good point.


Nicolas

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

On Tuesday 27 October 2009 20:35:31 Nicolas Pitre wrote:
> On Tue, 27 Oct 2009, Jean Pihet wrote:
> > Nicolas,
> >
> > On Tuesday 27 October 2009 19:59:36 Nicolas Pitre wrote:
> > ...
> >
> > > > Side question: does the put_user requires a flush of some sort? If
> > > > not, why?
> > >
> > > No because it stores data into the d-cache directly at the virtual
> > > address to be used by user space.  Previously the d-cache needed to
> > > be cleaned for data to hit main memory and the i-cache invalidated for
> > > the newly stored _code_ to be seen by the instruction path.  Since
> > > there is no code involved anymore the cache flushes are useless.
> > >
> > > > Is it OK to re-send a patch with the call to flush_icache_range
> > > > removed?
> > >
> > > Yes.
> >
> > Ok here is the updated patch. Let's hope it is the good one that time ;-)
>
> Well, the commit message is certainly wrong. There is no copying of
> restart_syscall to user stack anymore.
Ok thanks for reviewing it! Here is a respin of the patch. Only the commit 
description has been touched.

>
> And is there a reason for touching sp in the error case?
>
> > Can it be merged? It applies cleanly on top of Russell's latest patch
> > (http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2).
Same question: can those 2 patches be merged in? Can they be acked-by?

> >
> > > Nicolas
> >
Thanks,
Jean
> >

---
>From 28336b68b2e2507ba0922c55147e5e72ec1a88dc Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Tue, 27 Oct 2009 10:09:22 +0100
Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT

Using OABI, the call to put_user in do_signal can fail. In that case
flush_icache page faults and the calling app goes in an infinite loop.

The solution is to check if put_user fails and force the app to
seg fault in that case.

Signed-off-by: janboe <janboe.ye@gmail.com>

Merged from
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
on top of
http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2

Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
suspend/resume.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 arch/arm/kernel/signal.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index f330974..ea9722a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 				regs->ARM_sp -= 4;
 				usp = (u32 __user *)regs->ARM_sp;
 
-				put_user(regs->ARM_pc, usp);
-				regs->ARM_pc = KERN_RESTART_CODE;
+				if (put_user(regs->ARM_pc, usp) == 0) {
+					regs->ARM_pc = KERN_RESTART_CODE;
+				} else {
+					regs->ARM_sp += 4;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 
1.6.2.5.168.g3823

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Wed, 28 Oct 2009, Jean Pihet wrote:

> On Tuesday 27 October 2009 20:35:31 Nicolas Pitre wrote:
> > Well, the commit message is certainly wrong. There is no copying of
> > restart_syscall to user stack anymore.
> Ok thanks for reviewing it! Here is a respin of the patch. Only the commit 
> description has been touched.
[...]
> Using OABI, the call to put_user in do_signal can fail. In that case
> flush_icache page faults and the calling app goes in an infinite loop.

Still wrong.  Why flush_icache?


Nicolas

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

On Wednesday 28 October 2009 17:13:28 Nicolas Pitre wrote:
> On Wed, 28 Oct 2009, Jean Pihet wrote:
> > On Tuesday 27 October 2009 20:35:31 Nicolas Pitre wrote:
> > > Well, the commit message is certainly wrong. There is no copying of
> > > restart_syscall to user stack anymore.
> >
> > Ok thanks for reviewing it! Here is a respin of the patch. Only the
> > commit description has been touched.
>
> [...]
>
> > Using OABI, the call to put_user in do_signal can fail. In that case
> > flush_icache page faults and the calling app goes in an infinite loop.
>
> Still wrong.  Why flush_icache?
Indeed that is wrong (again!). I had put it in the problem description but 
that does not apply anymore.

Thanks, will send an update.
>
>
> Nicolas

Jean

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

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

On Wednesday 28 October 2009 17:23:01 Jean Pihet wrote:
...
> > Still wrong.  Why flush_icache?
>
> Indeed that is wrong (again!). I had put it in the problem description but
> that does not apply anymore.
>
> Thanks, will send an update.
Here is the updated patch. Is it OK?

Jean
>
> > Nicolas
>
> Jean
>

---
>From 28336b68b2e2507ba0922c55147e5e72ec1a88dc Mon Sep 17 00:00:00 2001
From: Jean Pihet <jpihet@mvista.com>
Date: Tue, 27 Oct 2009 10:09:22 +0100
Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT

Source: Janboe Ye <janboe.ye@gmail.com>
MR: 36048
Type: Defect Fix
Disposition: Submitted to linux-arm-kernel ML
ChangeID: 689ddf707d2232e9cf01387ac1264bc8812b9ffd
Description:

Using OABI, the call to put_user in do_signal can fail causing the calling
app to hang.

The solution is to check if put_user fails and force the app to
seg fault in that case.

Signed-off-by: janboe <janboe.ye@gmail.com>

Merged from
http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
on top of
http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2

Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
suspend/resume.

Signed-off-by: Jean Pihet <jpihet@mvista.com>
---
 arch/arm/kernel/signal.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index f330974..ea9722a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
*regs, int syscall)
 				regs->ARM_sp -= 4;
 				usp = (u32 __user *)regs->ARM_sp;
 
-				put_user(regs->ARM_pc, usp);
-				regs->ARM_pc = KERN_RESTART_CODE;
+				if (put_user(regs->ARM_pc, usp) == 0) {
+					regs->ARM_pc = KERN_RESTART_CODE;
+				} else {
+					regs->ARM_sp += 4;
+					force_sigsegv(0, current);
+				}
 #endif
 			}
 		}
-- 
1.6.2.5.168.g3823

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Nicolas Pitre]

On Wed, 28 Oct 2009, Jean Pihet wrote:

> On Wednesday 28 October 2009 17:23:01 Jean Pihet wrote:
> ...
> > > Still wrong.  Why flush_icache?
> >
> > Indeed that is wrong (again!). I had put it in the problem description but
> > that does not apply anymore.
> >
> > Thanks, will send an update.
> Here is the updated patch. Is it OK?

ACK.

> ---
> From 28336b68b2e2507ba0922c55147e5e72ec1a88dc Mon Sep 17 00:00:00 2001
> From: Jean Pihet <jpihet@mvista.com>
> Date: Tue, 27 Oct 2009 10:09:22 +0100
> Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT
> 
> Source: Janboe Ye <janboe.ye@gmail.com>
> MR: 36048
> Type: Defect Fix
> Disposition: Submitted to linux-arm-kernel ML
> ChangeID: 689ddf707d2232e9cf01387ac1264bc8812b9ffd
> Description:
> 
> Using OABI, the call to put_user in do_signal can fail causing the calling
> app to hang.
> 
> The solution is to check if put_user fails and force the app to
> seg fault in that case.
> 
> Signed-off-by: janboe <janboe.ye@gmail.com>
> 
> Merged from
> http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.html
> on top of
> http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2
> 
> Tested with multiple sleeping apps/threads (using the nanosleep syscall) and
> suspend/resume.
> 
> Signed-off-by: Jean Pihet <jpihet@mvista.com>
> ---
>  arch/arm/kernel/signal.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index f330974..ea9722a 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs 
> *regs, int syscall)
>  				regs->ARM_sp -= 4;
>  				usp = (u32 __user *)regs->ARM_sp;
>  
> -				put_user(regs->ARM_pc, usp);
> -				regs->ARM_pc = KERN_RESTART_CODE;
> +				if (put_user(regs->ARM_pc, usp) == 0) {
> +					regs->ARM_pc = KERN_RESTART_CODE;
> +				} else {
> +					regs->ARM_sp += 4;
> +					force_sigsegv(0, current);
> +				}
>  #endif
>  			}
>  		}
> -- 
> 1.6.2.5.168.g3823
> 
> 

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jamie Lokier]

Jean Pihet wrote:
>  				usp = (u32 __user *)regs->ARM_sp;
>  
> -				put_user(regs->ARM_pc, usp);
> -				regs->ARM_pc = KERN_RESTART_CODE;
> +				if (put_user(regs->ARM_pc, usp) == 0) {
> +					regs->ARM_pc = KERN_RESTART_CODE;
> +				} else {
> +					regs->ARM_sp += 4;
> +					force_sigsegv(0, current);
> +				}
>  #endif
>  			}
>  		}

This one looks good to me.

-- Jamie

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

Russell,

Is this one OK? Can it be merged now that your fix for signal restart has been 
merged into mainline?

Regards,
Jean

On Wednesday 28 October 2009 18:00:47 Jean Pihet wrote:
> On Wednesday 28 October 2009 17:23:01 Jean Pihet wrote:
> ...
>
> > > Still wrong.  Why flush_icache?
> >
> > Indeed that is wrong (again!). I had put it in the problem description
> > but that does not apply anymore.
> >
> > Thanks, will send an update.
>
> Here is the updated patch. Is it OK?
>
> Jean
>
> > > Nicolas
> >
> > Jean
>
> ---
> From 28336b68b2e2507ba0922c55147e5e72ec1a88dc Mon Sep 17 00:00:00 2001
> From: Jean Pihet <jpihet@mvista.com>
> Date: Tue, 27 Oct 2009 10:09:22 +0100
> Subject: ARM: Check put_user fail in do_signal when enable OABI_COMPAT
>
> Source: Janboe Ye <janboe.ye@gmail.com>
> MR: 36048
> Type: Defect Fix
> Disposition: Submitted to linux-arm-kernel ML
> ChangeID: 689ddf707d2232e9cf01387ac1264bc8812b9ffd
> Description:
>
> Using OABI, the call to put_user in do_signal can fail causing the calling
> app to hang.
>
> The solution is to check if put_user fails and force the app to
> seg fault in that case.
>
> Signed-off-by: janboe <janboe.ye@gmail.com>
>
> Merged from
> http://lists.infradead.org/pipermail/linux-arm-kernel/2009-October/002621.h
>tml on top of
> http://marc.info/?l=linux-arm-kernel&m=125638133624452&w=2
>
> Tested with multiple sleeping apps/threads (using the nanosleep syscall)
> and suspend/resume.
>
> Signed-off-by: Jean Pihet <jpihet@mvista.com>
> ---
>  arch/arm/kernel/signal.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index f330974..ea9722a 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -676,8 +676,12 @@ static int do_signal(sigset_t *oldset, struct pt_regs
> *regs, int syscall)
>  				regs->ARM_sp -= 4;
>  				usp = (u32 __user *)regs->ARM_sp;
>
> -				put_user(regs->ARM_pc, usp);
> -				regs->ARM_pc = KERN_RESTART_CODE;
> +				if (put_user(regs->ARM_pc, usp) == 0) {
> +					regs->ARM_pc = KERN_RESTART_CODE;
> +				} else {
> +					regs->ARM_sp += 4;
> +					force_sigsegv(0, current);
> +				}
>  #endif
>  			}
>  		}

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Wed, Nov 04, 2009 at 02:33:31PM +0100, Jean Pihet wrote:
> Is this one OK? Can it be merged now that your fix for signal restart
> has been merged into mainline?

Yes.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

Russell,

On Wednesday 04 November 2009 20:32:44 Russell King - ARM Linux wrote:
> On Wed, Nov 04, 2009 at 02:33:31PM +0100, Jean Pihet wrote:
> > Is this one OK? Can it be merged now that your fix for signal restart
> > has been merged into mainline?
>
> Yes.
May I ask you again to merge the patch? Is this one supposed to reach into 
mainline via your tree?

Regards,
Jean

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Russell King - ARM Linux]

On Mon, Nov 23, 2009 at 02:59:13PM +0100, Jean Pihet wrote:
> Russell,
> 
> On Wednesday 04 November 2009 20:32:44 Russell King - ARM Linux wrote:
> > On Wed, Nov 04, 2009 at 02:33:31PM +0100, Jean Pihet wrote:
> > > Is this one OK? Can it be merged now that your fix for signal restart
> > > has been merged into mainline?
> >
> > Yes.
> May I ask you again to merge the patch? Is this one supposed to reach into 
> mainline via your tree?

I have a patch system to avoid patches being dropped like this... I'm
really *bad* with emailed patches.

Please submit to the patch system, thanks.

[PATCH] check put_user fail in do_signal when enable OABI_COMPACT

[Jean Pihet]

On Monday 23 November 2009 15:06:50 Russell King - ARM Linux wrote:
> On Mon, Nov 23, 2009 at 02:59:13PM +0100, Jean Pihet wrote:
> > Russell,
> >
> > On Wednesday 04 November 2009 20:32:44 Russell King - ARM Linux wrote:
> > > On Wed, Nov 04, 2009 at 02:33:31PM +0100, Jean Pihet wrote:
> > > > Is this one OK? Can it be merged now that your fix for signal restart
> > > > has been merged into mainline?
> > >
> > > Yes.
> >
> > May I ask you again to merge the patch? Is this one supposed to reach
> > into mainline via your tree?
>
> I have a patch system to avoid patches being dropped like this... I'm
> really *bad* with emailed patches.
>
> Please submit to the patch system, thanks.
Ah ok! It is submitted as patch 5793/1.

Thanks,
Jean