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 <[email protected]>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: <[email protected]> # 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
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)
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
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)
On Wed, 4 Dec 2019, David Laight wrote:
> > 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.
As I recall we only emulate unaligned accesses with a subset of integer
load/store instructions (and then only if TIF_FIXADE is set, which is the
default), and never with FP load/store instructions. Consequently I see
no point in doing this in the FP emulator either and I think these ought
to just send SIGBUS instead. Otherwise you'll end up with user code that
works differently depending on whether the FP hardware is real or
emulated, which is really bad.
FWIW,
Maciej
Hi Maciej,
On Thu, Dec 26, 2019 at 03:01:06AM +0000, Maciej W. Rozycki wrote:
> On Wed, 4 Dec 2019, David Laight wrote:
> > > 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.
>
> As I recall we only emulate unaligned accesses with a subset of integer
> load/store instructions (and then only if TIF_FIXADE is set, which is the
> default), and never with FP load/store instructions. Consequently I see
> no point in doing this in the FP emulator either and I think these ought
> to just send SIGBUS instead. Otherwise you'll end up with user code that
> works differently depending on whether the FP hardware is real or
> emulated, which is really bad.
That might simplify things here, but it's incorrect. I'm fairly certain
the intent is that emulate_load_store_insn() handles all non-FP loads &
stores (though looking at it we're missing some instructions added in
r6). More importantly though we've been emulating FP loads & stores
since v3.10 which introduced the change alongside microMIPS support in
commit 102cedc32a6e ("MIPS: microMIPS: Floating point support."). The
commit contains no description of why, and I'm not aware of any reason
microMIPS specifically would need this so I suspect that commit bundled
this change for no good reason...
It's also worth noting that some hardware will handle unaligned FP
loads/stores, which means having the emulator reject them will result in
more of a visible difference to userland. ie. on some hardware they'll
work just fine, but on some you'd get SIGBUS. So I do think emulating
them makes some sense - just as for non-FP loads & stores it lets
userland not care whether the hardware will handle them, so long as it's
not performance critical code. If we knew that had never been used then
perhaps we could enforce the alignment requirement (and maybe that's
what you recall doing), but since we've been emulating them for the past
6 years it's too late for that now.
Thanks,
Paul
Hi Paul,
Sorry to take so long; it took me a while to track down the discussion I
had in mind, and I was quite busy too. Also greetings from linux.conf.au!
> > As I recall we only emulate unaligned accesses with a subset of integer
> > load/store instructions (and then only if TIF_FIXADE is set, which is the
> > default), and never with FP load/store instructions. Consequently I see
> > no point in doing this in the FP emulator either and I think these ought
> > to just send SIGBUS instead. Otherwise you'll end up with user code that
> > works differently depending on whether the FP hardware is real or
> > emulated, which is really bad.
>
> That might simplify things here, but it's incorrect. I'm fairly certain
> the intent is that emulate_load_store_insn() handles all non-FP loads &
> stores (though looking at it we're missing some instructions added in
> r6). More importantly though we've been emulating FP loads & stores
> since v3.10 which introduced the change alongside microMIPS support in
> commit 102cedc32a6e ("MIPS: microMIPS: Floating point support."). The
> commit contains no description of why, and I'm not aware of any reason
> microMIPS specifically would need this so I suspect that commit bundled
> this change for no good reason...
See the thread of discussion starting from this submission:
<https://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20120615234641.6938B58FE7C%40mail.viric.name>
and in particular Ralf's response (not referred directly due to the
monthly archive rollover):
<https://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20120731134001.GA14151%40linux-mips.org>
I think Ralf's argument still stands and I find it regrettable that an
unwanted feature was sneaked in with a trick along with a submission
supposed to only add a different, unrelated feature.
I can't even track down a public submission/review of the change you
refer, which is not how things are supposed to work with Linux! And
neither the `Signed-off-by' tags help figuring out what the route of the
change was to get there upstream. At that time there was supposed to be
Ralf's tag there, as it was him who was the sole port maintainer.
> It's also worth noting that some hardware will handle unaligned FP
> loads/stores, which means having the emulator reject them will result in
> more of a visible difference to userland. ie. on some hardware they'll
> work just fine, but on some you'd get SIGBUS. So I do think emulating
> them makes some sense - just as for non-FP loads & stores it lets
> userland not care whether the hardware will handle them, so long as it's
> not performance critical code. If we knew that had never been used then
> perhaps we could enforce the alignment requirement (and maybe that's
> what you recall doing), but since we've been emulating them for the past
> 6 years it's too late for that now.
I don't think it's ever too late to remove a broken feature that everyone
knows is not a part of the architecture and the emulation of which has
never been advertised as a part of the Linux ABI either. You just don't
make it a part of the ABI when you sneak in a feature without a proper
review, we do not accept the fait accompli method in Linux development.
The presence of unaligned FP data is a sign of user code breakage and
whoever caused that breakage will best know that ASAP by seeing their
program trap (they can emulate the trap in their software by installing a
suitable signal handler if they are so desperate to have unaligned FP data
handled).
So I think that not only the new submission should be rejected, but also
parts of commit 102cedc32a6e ("MIPS: microMIPS: Floating point support.")
reverted that are not a part of actual microMIPS support. If someone
relied on it by accident or ignorance, they'll simply have to adjust.
Maciej