[PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Thiemo Seufer]

Hi All,

somebody made wrong assumptions about how compute_return_epc() works.
It does not return the epc but stores it in the register struct.
Return value is -EFAULT or zero.

I've speculated below how the right solution might look, but I
don't know enough about signal handling to be sure.


Thiemo


diff -BurPX /bigdisk/src/dontdiff linux-orig/arch/mips/kernel/traps.c linux/arch/mips/kernel/traps.c
--- linux-orig/arch/mips/kernel/traps.c	Sat Jul 14 18:49:46 2001
+++ linux/arch/mips/kernel/traps.c	Sun Jul 22 08:44:57 2001
@@ -378,8 +378,11 @@
 		else
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
-		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_errno = compute_return_epc(regs);
+		if (info.si_errno)
+			info.si_addr = NULL;
+		else
+			info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -418,8 +421,11 @@
 		else
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
-		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_errno = compute_return_epc(regs);
+		if (info.si_errno)
+			info.si_addr = NULL;
+		else
+			info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Maciej W. Rozycki]

On Tue, 24 Jul 2001, Thiemo Seufer wrote:

> somebody made wrong assumptions about how compute_return_epc() works.

 It was me, I admit...  Thanks for pointing it out. 

> I've speculated below how the right solution might look, but I
> don't know enough about signal handling to be sure.

 I think the following fix is sufficient -- let's just pass EPC and let
the userland handle it.  You don't normally want a "break" in a branch
delay slot -- such a sequence is of questionable utility.  But if it is to
be handled, the KISS approach gives the userland a chance to handle an
exception gracefully.  One may want to emulate overflows somehow, for
example.  Also the code is shorter. 

 Ralf, please apply it.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-mips-2.4.5-20010730-break-1
diff -up --recursive --new-file linux.macro/arch/mips/kernel/traps.c linux/arch/mips/kernel/traps.c
--- linux.macro/arch/mips/kernel/traps.c	Tue Jul 24 04:26:34 2001
+++ linux/arch/mips/kernel/traps.c	Mon Jul 30 18:26:03 2001
@@ -378,7 +378,7 @@ asmlinkage void do_bp(struct pt_regs *re
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -418,7 +418,7 @@ asmlinkage void do_tr(struct pt_regs *re
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Kevin D. Kissell]

Many years ago, I went to a lot of effort to see to it that
FP exceptions on the Fairchild Clipper passed enough
information up through SIGFPE to allow full userland
emulation or fixup and replay-on-return of the operation
on floating point faults (entertaining because there was
a seperate FP PC). And of course no one ever used it!   ;-)

I missed the beginning of this thread, but it looks from the
patch as if this is really about handling integer overflow
exceptions, not FP exceptions.  That's unfortunate.
To begin with you are correct in that we should be
passing the EPC value at the exception (and certainly not
the result  of invoking compute_return_epc(), much less
its side effects!), and the state of the BD bit from the
Cause register, either abstracted into a variable in the
info structure or as a flat-out copy of the Cause register.
I would recommend the former as being less of a security
hole.  Yes, we could blow that off and make the user
decode for branches himself, but it's tacky. But what's
more pernicious is the fact that, being an integer exception,
it could have resulted from an overflow of any of the GPRs,
including those that will be used by the low-level library
code dispatching the signal to the user, and in the generated
code of the user's own signal handler.  If we actually want
to allow the user to fix the operation and saturate (or whatever)
the destination register, enough of the trap context needs
to be passed up to the signal handler *and* back down
on the signal return to allow that manipulation, followed
by a resumption of execution at the instruction following
the one that generated the trap.

            Been there, done that, got the coffee mug,

            Kevin K.

----- Original Message -----
From: "Maciej W. Rozycki" <macro@ds2.pg.gda.pl>
To: "Ralf Baechle" <ralf@uni-koblenz.de>; "Thiemo Seufer"
<ica2_ts@csv.ica.uni-stuttgart.de>
Cc: <linux-mips@oss.sgi.com>
Sent: Monday, July 30, 2001 8:46 PM
Subject: Re: [PATCH] wrong use of compute_return_epc() in
/mips/kernel/traps.c


> On Tue, 24 Jul 2001, Thiemo Seufer wrote:
>
> > somebody made wrong assumptions about how compute_return_epc() works.
>
>  It was me, I admit...  Thanks for pointing it out.
>
> > I've speculated below how the right solution might look, but I
> > don't know enough about signal handling to be sure.
>
>  I think the following fix is sufficient -- let's just pass EPC and let
> the userland handle it.  You don't normally want a "break" in a branch
> delay slot -- such a sequence is of questionable utility.  But if it is to
> be handled, the KISS approach gives the userland a chance to handle an
> exception gracefully.  One may want to emulate overflows somehow, for
> example.  Also the code is shorter.
>
>  Ralf, please apply it.
>
>   Maciej
>
> --
> +  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
> +--------------------------------------------------------------+
> +        e-mail: macro@ds2.pg.gda.pl, PGP key available        +
>
> patch-mips-2.4.5-20010730-break-1
> diff -up --recursive --new-file linux.macro/arch/mips/kernel/traps.c
linux/arch/mips/kernel/traps.c
> --- linux.macro/arch/mips/kernel/traps.c Tue Jul 24 04:26:34 2001
> +++ linux/arch/mips/kernel/traps.c Mon Jul 30 18:26:03 2001
> @@ -378,7 +378,7 @@ asmlinkage void do_bp(struct pt_regs *re
>   info.si_code = FPE_INTOVF;
>   info.si_signo = SIGFPE;
>   info.si_errno = 0;
> - info.si_addr = (void *)compute_return_epc(regs);
> + info.si_addr = (void *)regs->cp0_epc;
>   force_sig_info(SIGFPE, &info, current);
>   break;
>   default:
> @@ -418,7 +418,7 @@ asmlinkage void do_tr(struct pt_regs *re
>   info.si_code = FPE_INTOVF;
>   info.si_signo = SIGFPE;
>   info.si_errno = 0;
> - info.si_addr = (void *)compute_return_epc(regs);
> + info.si_addr = (void *)regs->cp0_epc;
>   force_sig_info(SIGFPE, &info, current);
>   break;
>   default:
>

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Kevin D. Kissell]

Many years ago, I went to a lot of effort to see to it that
FP exceptions on the Fairchild Clipper passed enough
information up through SIGFPE to allow full userland
emulation or fixup and replay-on-return of the operation
on floating point faults (entertaining because there was
a seperate FP PC). And of course no one ever used it!   ;-)

I missed the beginning of this thread, but it looks from the
patch as if this is really about handling integer overflow
exceptions, not FP exceptions.  That's unfortunate.
To begin with you are correct in that we should be
passing the EPC value at the exception (and certainly not
the result  of invoking compute_return_epc(), much less
its side effects!), and the state of the BD bit from the
Cause register, either abstracted into a variable in the
info structure or as a flat-out copy of the Cause register.
I would recommend the former as being less of a security
hole.  Yes, we could blow that off and make the user
decode for branches himself, but it's tacky. But what's
more pernicious is the fact that, being an integer exception,
it could have resulted from an overflow of any of the GPRs,
including those that will be used by the low-level library
code dispatching the signal to the user, and in the generated
code of the user's own signal handler.  If we actually want
to allow the user to fix the operation and saturate (or whatever)
the destination register, enough of the trap context needs
to be passed up to the signal handler *and* back down
on the signal return to allow that manipulation, followed
by a resumption of execution at the instruction following
the one that generated the trap.

            Been there, done that, got the coffee mug,

            Kevin K.

----- Original Message -----
From: "Maciej W. Rozycki" <macro@ds2.pg.gda.pl>
To: "Ralf Baechle" <ralf@uni-koblenz.de>; "Thiemo Seufer"
<ica2_ts@csv.ica.uni-stuttgart.de>
Cc: <linux-mips@oss.sgi.com>
Sent: Monday, July 30, 2001 8:46 PM
Subject: Re: [PATCH] wrong use of compute_return_epc() in
/mips/kernel/traps.c


> On Tue, 24 Jul 2001, Thiemo Seufer wrote:
>
> > somebody made wrong assumptions about how compute_return_epc() works.
>
>  It was me, I admit...  Thanks for pointing it out.
>
> > I've speculated below how the right solution might look, but I
> > don't know enough about signal handling to be sure.
>
>  I think the following fix is sufficient -- let's just pass EPC and let
> the userland handle it.  You don't normally want a "break" in a branch
> delay slot -- such a sequence is of questionable utility.  But if it is to
> be handled, the KISS approach gives the userland a chance to handle an
> exception gracefully.  One may want to emulate overflows somehow, for
> example.  Also the code is shorter.
>
>  Ralf, please apply it.
>
>   Maciej
>
> --
> +  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
> +--------------------------------------------------------------+
> +        e-mail: macro@ds2.pg.gda.pl, PGP key available        +
>
> patch-mips-2.4.5-20010730-break-1
> diff -up --recursive --new-file linux.macro/arch/mips/kernel/traps.c
linux/arch/mips/kernel/traps.c
> --- linux.macro/arch/mips/kernel/traps.c Tue Jul 24 04:26:34 2001
> +++ linux/arch/mips/kernel/traps.c Mon Jul 30 18:26:03 2001
> @@ -378,7 +378,7 @@ asmlinkage void do_bp(struct pt_regs *re
>   info.si_code = FPE_INTOVF;
>   info.si_signo = SIGFPE;
>   info.si_errno = 0;
> - info.si_addr = (void *)compute_return_epc(regs);
> + info.si_addr = (void *)regs->cp0_epc;
>   force_sig_info(SIGFPE, &info, current);
>   break;
>   default:
> @@ -418,7 +418,7 @@ asmlinkage void do_tr(struct pt_regs *re
>   info.si_code = FPE_INTOVF;
>   info.si_signo = SIGFPE;
>   info.si_errno = 0;
> - info.si_addr = (void *)compute_return_epc(regs);
> + info.si_addr = (void *)regs->cp0_epc;
>   force_sig_info(SIGFPE, &info, current);
>   break;
>   default:
>

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Maciej W. Rozycki]

On Mon, 30 Jul 2001, Kevin D. Kissell wrote:

> Many years ago, I went to a lot of effort to see to it that
> FP exceptions on the Fairchild Clipper passed enough
> information up through SIGFPE to allow full userland
> emulation or fixup and replay-on-return of the operation
> on floating point faults (entertaining because there was
> a seperate FP PC). And of course no one ever used it!   ;-)

 That's why I took the KISS approach.  Current code is cheap and satisfies
the specs I use, i.e. glibc's info pages.  If anyone needs anything more
one is free to improve handlers. 

> I missed the beginning of this thread, but it looks from the
> patch as if this is really about handling integer overflow
> exceptions, not FP exceptions.  That's unfortunate.

 The patch is about break and trap instruction handlers, indeed. 

> To begin with you are correct in that we should be
> passing the EPC value at the exception (and certainly not
> the result  of invoking compute_return_epc(), much less
> its side effects!), and the state of the BD bit from the
> Cause register, either abstracted into a variable in the
> info structure or as a flat-out copy of the Cause register.

 I don't think passing BD is needed here.  Enough information is
available.  A SIGFPE due to a branch or a jump instruction is impossible. 
Unless you have a chip on fire, in which case problems with signal
handlers are insignificant.

> I would recommend the former as being less of a security
> hole.  Yes, we could blow that off and make the user
> decode for branches himself, but it's tacky. But what's
> more pernicious is the fact that, being an integer exception,
> it could have resulted from an overflow of any of the GPRs,
> including those that will be used by the low-level library
> code dispatching the signal to the user, and in the generated
> code of the user's own signal handler.  If we actually want
> to allow the user to fix the operation and saturate (or whatever)
> the destination register, enough of the trap context needs
> to be passed up to the signal handler *and* back down
> on the signal return to allow that manipulation, followed
> by a resumption of execution at the instruction following
> the one that generated the trap.

 Let's just leave it as an excercise to one writing some emulation code. 
A general-purpose handler in the kernel or libc (which no one will
probably use) is an overkill and by defining own strict coding rules (be
it a calling convention, shadow registers or whatever) one can create
software that suits one's specific needs. 

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Kevin D. Kissell]

>  That's why I took the KISS approach.  Current code is cheap and satisfies
> the specs I use, i.e. glibc's info pages.  If anyone needs anything more
> one is free to improve handlers.

The handlers cannot spontaneously generate information known
only to the kernel.

> > I missed the beginning of this thread, but it looks from the
> > patch as if this is really about handling integer overflow
> > exceptions, not FP exceptions.  That's unfortunate.
>
>  The patch is about break and trap instruction handlers, indeed.
>
> > To begin with you are correct in that we should be
> > passing the EPC value at the exception (and certainly not
> > the result  of invoking compute_return_epc(), much less
> > its side effects!), and the state of the BD bit from the
> > Cause register, either abstracted into a variable in the
> > info structure or as a flat-out copy of the Cause register.
>
>  I don't think passing BD is needed here.  Enough information is
> available.  A SIGFPE due to a branch or a jump instruction is impossible.
> Unless you have a chip on fire, in which case problems with signal
> handlers are insignificant.
>
> > I would recommend the former as being less of a security
> > hole.  Yes, we could blow that off and make the user
> > decode for branches himself, but it's tacky. But what's
> > more pernicious is the fact that, being an integer exception,
> > it could have resulted from an overflow of any of the GPRs,
> > including those that will be used by the low-level library
> > code dispatching the signal to the user, and in the generated
> > code of the user's own signal handler.  If we actually want
> > to allow the user to fix the operation and saturate (or whatever)
> > the destination register, enough of the trap context needs
> > to be passed up to the signal handler *and* back down
> > on the signal return to allow that manipulation, followed
> > by a resumption of execution at the instruction following
> > the one that generated the trap.
>
>  Let's just leave it as an excercise to one writing some emulation code.

An excercise much akin to squaring the circle.  Think
about it.  Your program has overflowed on an operation
on a0.  The signal is sent, and your signal routine is called,
trashing a0 with the first argument.  For greater amusement,
assume that the instruction was "add a0, a0, a1" (which
is what could easily happen in a C function that begins
by summing its first two arguments).  I simply do not see
how user level code can determine whether the overflow
was postive or negative without knowlege of the inputs,
and those inputs may well have existed only in the registers.
And even assuming that one could deduce the correct value,
how would you propose patching it back into the execution
stream, particularly in the presence of delayed branches?

> A general-purpose handler in the kernel or libc (which no one will
> probably use) is an overkill and by defining own strict coding rules (be
> it a calling convention, shadow registers or whatever) one can create
> software that suits one's specific needs.

Yeah.  Right.  Shadow registers will allow you to be certain
that you haven't trashed your inputs.  They won't help you
resume execution with a corrected value in the destination
register of the faulting instruction.  I have no problem with
declaring that signals  cannot be used to handle overflow
exceptions.  I do have a problem with falsely pretending to
support such handling. We can decide (for now) that the problem
doesn't need to  be solved.  We can decide to call it a "TODO"
for some time  when we're old and grey.   We can solve it now.
I really don't care.  But it's dishonest to say that the current code
solves it,  even  to a first order of approximation.  To trot out an
overused Einstein quote,

"Everything should be as simple as possible. But not simpler".

            Kevin K.

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Kevin D. Kissell]

>  That's why I took the KISS approach.  Current code is cheap and satisfies
> the specs I use, i.e. glibc's info pages.  If anyone needs anything more
> one is free to improve handlers.

The handlers cannot spontaneously generate information known
only to the kernel.

> > I missed the beginning of this thread, but it looks from the
> > patch as if this is really about handling integer overflow
> > exceptions, not FP exceptions.  That's unfortunate.
>
>  The patch is about break and trap instruction handlers, indeed.
>
> > To begin with you are correct in that we should be
> > passing the EPC value at the exception (and certainly not
> > the result  of invoking compute_return_epc(), much less
> > its side effects!), and the state of the BD bit from the
> > Cause register, either abstracted into a variable in the
> > info structure or as a flat-out copy of the Cause register.
>
>  I don't think passing BD is needed here.  Enough information is
> available.  A SIGFPE due to a branch or a jump instruction is impossible.
> Unless you have a chip on fire, in which case problems with signal
> handlers are insignificant.
>
> > I would recommend the former as being less of a security
> > hole.  Yes, we could blow that off and make the user
> > decode for branches himself, but it's tacky. But what's
> > more pernicious is the fact that, being an integer exception,
> > it could have resulted from an overflow of any of the GPRs,
> > including those that will be used by the low-level library
> > code dispatching the signal to the user, and in the generated
> > code of the user's own signal handler.  If we actually want
> > to allow the user to fix the operation and saturate (or whatever)
> > the destination register, enough of the trap context needs
> > to be passed up to the signal handler *and* back down
> > on the signal return to allow that manipulation, followed
> > by a resumption of execution at the instruction following
> > the one that generated the trap.
>
>  Let's just leave it as an excercise to one writing some emulation code.

An excercise much akin to squaring the circle.  Think
about it.  Your program has overflowed on an operation
on a0.  The signal is sent, and your signal routine is called,
trashing a0 with the first argument.  For greater amusement,
assume that the instruction was "add a0, a0, a1" (which
is what could easily happen in a C function that begins
by summing its first two arguments).  I simply do not see
how user level code can determine whether the overflow
was postive or negative without knowlege of the inputs,
and those inputs may well have existed only in the registers.
And even assuming that one could deduce the correct value,
how would you propose patching it back into the execution
stream, particularly in the presence of delayed branches?

> A general-purpose handler in the kernel or libc (which no one will
> probably use) is an overkill and by defining own strict coding rules (be
> it a calling convention, shadow registers or whatever) one can create
> software that suits one's specific needs.

Yeah.  Right.  Shadow registers will allow you to be certain
that you haven't trashed your inputs.  They won't help you
resume execution with a corrected value in the destination
register of the faulting instruction.  I have no problem with
declaring that signals  cannot be used to handle overflow
exceptions.  I do have a problem with falsely pretending to
support such handling. We can decide (for now) that the problem
doesn't need to  be solved.  We can decide to call it a "TODO"
for some time  when we're old and grey.   We can solve it now.
I really don't care.  But it's dishonest to say that the current code
solves it,  even  to a first order of approximation.  To trot out an
overused Einstein quote,

"Everything should be as simple as possible. But not simpler".

            Kevin K.

Re: [PATCH] wrong use of compute_return_epc() in /mips/kernel/traps.c

[Maciej W. Rozycki]

On Mon, 30 Jul 2001, Kevin D. Kissell wrote:

> >  That's why I took the KISS approach.  Current code is cheap and satisfies
> > the specs I use, i.e. glibc's info pages.  If anyone needs anything more
> > one is free to improve handlers.
> 
> The handlers cannot spontaneously generate information known
> only to the kernel.

 I'm not sure what you mean here.  Could you please elaborate?

> An excercise much akin to squaring the circle.  Think
> about it.  Your program has overflowed on an operation
> on a0.  The signal is sent, and your signal routine is called,
> trashing a0 with the first argument.  For greater amusement,
> assume that the instruction was "add a0, a0, a1" (which
> is what could easily happen in a C function that begins
> by summing its first two arguments).  I simply do not see
> how user level code can determine whether the overflow
> was postive or negative without knowlege of the inputs,
> and those inputs may well have existed only in the registers.
> And even assuming that one could deduce the correct value,
> how would you propose patching it back into the execution
> stream, particularly in the presence of delayed branches?

 Sure, you are writing of the general case.  That's not solvable given
current interfaces.

> Yeah.  Right.  Shadow registers will allow you to be certain
> that you haven't trashed your inputs.  They won't help you
> resume execution with a corrected value in the destination
> register of the faulting instruction.  I have no problem with

 It actually depends on how you specify your requirements.

> declaring that signals  cannot be used to handle overflow
> exceptions.  I do have a problem with falsely pretending to
> support such handling. We can decide (for now) that the problem
> doesn't need to  be solved.  We can decide to call it a "TODO"
> for some time  when we're old and grey.   We can solve it now.

 Si_addr in the siginfo struct is specified to contain a faulting
instruction's reference.  My patch fixes the code to fullfill the
specification.  The code was meant to do it already when I submitted it
last year.  Somehow I made a bug and now I'm only fixing it. 

 We do nowhere pretend to support handling of overflow exceptions to the
extent that permits emulating of faulting instructions.  Check the docs,
namely the "Program Error Signals" node of glibc's info pages.  It seems
my handler's example was a bit unfortunate, sigh...

> I really don't care.  But it's dishonest to say that the current code
> solves it,  even  to a first order of approximation.  To trot out an
> overused Einstein quote,
> 
> "Everything should be as simple as possible. But not simpler".

 Yes, sure, we may hardcode a zero here, but what the gain would be? 
Providing a real value instead makes us adhere to the spec, it's only
marginally slower and it may be useful for anything, not only extending
precision.  For example one may print the address involved for user's
(developer's) feedback and abort the affected function only as opposed to
dumping a core.

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +