Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores
@ 2019-12-03 20:49 Paul Burton
  2019-12-04 11:14 ` David Laight
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Burton @ 2019-12-03 20:49 UTC (permalink / raw)
  To: linux-mips; +Cc: linux-kernel, Paul Burton, stable

Our FPU emulator currently uses __get_user() & __put_user() to perform
emulated loads & stores. This is problematic because __get_user() &
__put_user() are only suitable for naturally aligned memory accesses,
and the address we're accessing is entirely under the control of
userland.

This allows userland to cause a kernel panic by simply performing an
unaligned floating point load or store - the kernel will handle the
address error exception by attempting to emulate the instruction, and in
the process it may generate another address error exception itself.
This time the exception is taken with EPC pointing at the kernels FPU
emulation code, and we hit a die_if_kernel() in
emulate_load_store_insn().

Fix this up by using __copy_from_user() instead of __get_user() and
__copy_to_user() instead of __put_user(). These replacements will handle
arbitrary alignment without problems.

Signed-off-by: Paul Burton <paulburton@kernel.org>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org> # v2.6.12+
---
 arch/mips/math-emu/cp1emu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/mips/math-emu/cp1emu.c b/arch/mips/math-emu/cp1emu.c
index 710e1f804a54..d2009b4b5209 100644
--- a/arch/mips/math-emu/cp1emu.c
+++ b/arch/mips/math-emu/cp1emu.c
@@ -1056,7 +1056,7 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 			*fault_addr = dva;
 			return SIGBUS;
 		}
-		if (__get_user(dval, dva)) {
+		if (__copy_from_user(&dval, dva, sizeof(u64))) {
 			MIPS_FPU_EMU_INC_STATS(errors);
 			*fault_addr = dva;
 			return SIGSEGV;
@@ -1074,7 +1074,7 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 			*fault_addr = dva;
 			return SIGBUS;
 		}
-		if (__put_user(dval, dva)) {
+		if (__copy_to_user(dva, &dval, sizeof(u64))) {
 			MIPS_FPU_EMU_INC_STATS(errors);
 			*fault_addr = dva;
 			return SIGSEGV;
@@ -1090,7 +1090,7 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 			*fault_addr = wva;
 			return SIGBUS;
 		}
-		if (__get_user(wval, wva)) {
+		if (__copy_from_user(&wval, wva, sizeof(u32))) {
 			MIPS_FPU_EMU_INC_STATS(errors);
 			*fault_addr = wva;
 			return SIGSEGV;
@@ -1108,7 +1108,7 @@ static int cop1Emulate(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 			*fault_addr = wva;
 			return SIGBUS;
 		}
-		if (__put_user(wval, wva)) {
+		if (__copy_to_user(wva, &wval, sizeof(u32))) {
 			MIPS_FPU_EMU_INC_STATS(errors);
 			*fault_addr = wva;
 			return SIGSEGV;
@@ -1486,7 +1486,7 @@ static int fpux_emu(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 				*fault_addr = va;
 				return SIGBUS;
 			}
-			if (__get_user(val, va)) {
+			if (__copy_from_user(&val, va, sizeof(u32))) {
 				MIPS_FPU_EMU_INC_STATS(errors);
 				*fault_addr = va;
 				return SIGSEGV;
@@ -1506,7 +1506,7 @@ static int fpux_emu(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 				*fault_addr = va;
 				return SIGBUS;
 			}
-			if (put_user(val, va)) {
+			if (__copy_to_user(va, &val, sizeof(u32))) {
 				MIPS_FPU_EMU_INC_STATS(errors);
 				*fault_addr = va;
 				return SIGSEGV;
@@ -1583,7 +1583,7 @@ static int fpux_emu(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 				*fault_addr = va;
 				return SIGBUS;
 			}
-			if (__get_user(val, va)) {
+			if (__copy_from_user(&val, va, sizeof(u64))) {
 				MIPS_FPU_EMU_INC_STATS(errors);
 				*fault_addr = va;
 				return SIGSEGV;
@@ -1602,7 +1602,7 @@ static int fpux_emu(struct pt_regs *xcp, struct mips_fpu_struct *ctx,
 				*fault_addr = va;
 				return SIGBUS;
 			}
-			if (__put_user(val, va)) {
+			if (__copy_to_user(va, &val, sizeof(u64))) {
 				MIPS_FPU_EMU_INC_STATS(errors);
 				*fault_addr = va;
 				return SIGSEGV;
-- 
2.24.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores
  2019-12-03 20:49 [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores Paul Burton
@ 2019-12-04 11:14 ` David Laight
  2019-12-04 15:40   ` Paul Burton
  0 siblings, 1 reply; 4+ messages in thread
From: David Laight @ 2019-12-04 11:14 UTC (permalink / raw)
  To: Paul Burton, linux-mips; +Cc: linux-kernel, stable

From: Paul Burton
> Sent: 03 December 2019 20:50
> Our FPU emulator currently uses __get_user() & __put_user() to perform
> emulated loads & stores. This is problematic because __get_user() &
> __put_user() are only suitable for naturally aligned memory accesses,
> and the address we're accessing is entirely under the control of
> userland.
> 
> This allows userland to cause a kernel panic by simply performing an
> unaligned floating point load or store - the kernel will handle the
> address error exception by attempting to emulate the instruction, and in
> the process it may generate another address error exception itself.
> This time the exception is taken with EPC pointing at the kernels FPU
> emulation code, and we hit a die_if_kernel() in
> emulate_load_store_insn().

Won't this be true of almost all code that uses get_user() and put_user()
(with or without the leading __).

> Fix this up by using __copy_from_user() instead of __get_user() and
> __copy_to_user() instead of __put_user(). These replacements will handle
> arbitrary alignment without problems.

They'll also kill performance.....

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores
  2019-12-04 11:14 ` David Laight
@ 2019-12-04 15:40   ` Paul Burton
  2019-12-04 16:18     ` David Laight
  0 siblings, 1 reply; 4+ messages in thread
From: Paul Burton @ 2019-12-04 15:40 UTC (permalink / raw)
  To: David Laight; +Cc: linux-mips, linux-kernel, stable

Hi David,

On Wed, Dec 04, 2019 at 11:14:08AM +0000, David Laight wrote:
> From: Paul Burton
> > Sent: 03 December 2019 20:50
> > Our FPU emulator currently uses __get_user() & __put_user() to perform
> > emulated loads & stores. This is problematic because __get_user() &
> > __put_user() are only suitable for naturally aligned memory accesses,
> > and the address we're accessing is entirely under the control of
> > userland.
> > 
> > This allows userland to cause a kernel panic by simply performing an
> > unaligned floating point load or store - the kernel will handle the
> > address error exception by attempting to emulate the instruction, and in
> > the process it may generate another address error exception itself.
> > This time the exception is taken with EPC pointing at the kernels FPU
> > emulation code, and we hit a die_if_kernel() in
> > emulate_load_store_insn().
> 
> Won't this be true of almost all code that uses get_user() and put_user()
> (with or without the leading __).

Only if the address being accessed is under the control of userland to
the extent that it can create an unaligned address. You're right that
may be more widespread though; it needs checking...

We used to have separate get_user_unaligned() & put_user_unaligned()
which would suggest that it's expected that get_user() & put_user()
require their accesses be aligned, but they were removed by commit
3170d8d226c2 ("kill {__,}{get,put}_user_unaligned()") in v4.13.

But perhaps we should just take the second AdEL exception & recover via
the fixups table. We definitely don't right now... Needs further
investigation...

> > Fix this up by using __copy_from_user() instead of __get_user() and
> > __copy_to_user() instead of __put_user(). These replacements will handle
> > arbitrary alignment without problems.
> 
> They'll also kill performance.....

Sure they're heavier, but if you're hitting the FPU emulator you're
already slow - trapping to the kernel for instruction emulation is
hardly a hot path. If you care about performance at all then this is
already a code path to avoid at all costs.

Thanks,
    Paul

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores
  2019-12-04 15:40   ` Paul Burton
@ 2019-12-04 16:18     ` David Laight
  0 siblings, 0 replies; 4+ messages in thread
From: David Laight @ 2019-12-04 16:18 UTC (permalink / raw)
  To: Paul Burton; +Cc: linux-mips, linux-kernel, stable

From: Paul Burton
> Sent: 04 December 2019 15:41
> On Wed, Dec 04, 2019 at 11:14:08AM +0000, David Laight wrote:
> > From: Paul Burton
> > > Sent: 03 December 2019 20:50
> > > Our FPU emulator currently uses __get_user() & __put_user() to perform
> > > emulated loads & stores. This is problematic because __get_user() &
> > > __put_user() are only suitable for naturally aligned memory accesses,
> > > and the address we're accessing is entirely under the control of
> > > userland.
> > >
> > > This allows userland to cause a kernel panic by simply performing an
> > > unaligned floating point load or store - the kernel will handle the
> > > address error exception by attempting to emulate the instruction, and in
> > > the process it may generate another address error exception itself.
> > > This time the exception is taken with EPC pointing at the kernels FPU
> > > emulation code, and we hit a die_if_kernel() in
> > > emulate_load_store_insn().
> >
> > Won't this be true of almost all code that uses get_user() and put_user()
> > (with or without the leading __).
> 
> Only if the address being accessed is under the control of userland to
> the extent that it can create an unaligned address. You're right that
> may be more widespread though; it needs checking...

Look at (for example) the recvmmsg() code or epoll_wait().

I'd expect all get/put_user() to be potentially unaligned.
The user might have to try hard (to avoid all the faults in userspace)
but any buffer passed to the kernel can potentially be misaligned and
nothing (I've seen) is documented as returning EFAULT/SIGSEGV
for such unaligned buffers.

In 'days of yore...' SPARC systems would have done a SIGSEGV for
any misaligned access in userspace.
Not sure why Linux ever thought it was necessary to 'fixup' such faults.
OTOH it is too late to change that behaviour (at least for existing ports).

> We used to have separate get_user_unaligned() & put_user_unaligned()
> which would suggest that it's expected that get_user() & put_user()
> require their accesses be aligned, but they were removed by commit
> 3170d8d226c2 ("kill {__,}{get,put}_user_unaligned()") in v4.13.
> 
> But perhaps we should just take the second AdEL exception & recover via
> the fixups table. We definitely don't right now... Needs further
> investigation...

get/put_user can fault because the user page is absent (etc).
So there must be code to 'expect' a fault on those instructions.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 20:49 [PATCH] MIPS: Use __copy_{to,from}_user() for emulated FP loads/stores Paul Burton
2019-12-04 11:14 ` David Laight
2019-12-04 15:40   ` Paul Burton
2019-12-04 16:18     ` David Laight

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git