[PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[Michael Schmitz]

As mentioned by Finn Thain in his patch to improve put_user
exception handling on 040, a similar problem exists on 030
processors.

A moves instruction that crosses a page boundary from a
mapped page into an unmapped one will cause a mid-instruction
bus error exception (frame format b), with the PC pointing
two instructions past the faulting movesl instruction.

Our exception handling in __generic_copy_to_user only covers
the instruction immediately following the faulting one. As
a result, fixup_exception in send_fault_sig does not detect
this case, and cause send_fault_sig to oops.

Addition of a NOP does avert the fault in this case also.

I have tested this fault scenario with a transfer beginning
at a single byte at the end of a mapped page only, which IMO
does violate m68k alignment rules. I cannot remember ever
seeing such faults reported from conforming software before.

Fault Oops:

[3432825.940000] Unable to handle kernel access at virtual address 45e80efc
[3432826.000000] Oops: 00000000
[3432826.040000] Modules linked in: ne 8390p
[3432826.110000] PC: [<003217de>] __generic_copy_to_user+0x22/0x40
[3432826.150000] SR: 2200  SP: 3ae57f7e  a2: 00781db0
[3432826.200000] d0: 00000002    d1: 00000002    d2: 2f686f6d    d3: 009d7000
[3432826.250000] d4: 00000400    d5: c0014fff    a0: 009d7ff6    a1: c0015003
[3432826.310000] Process badaddr (pid: 7538, task=94e249df)
[3432826.350000] Frame format=B ssw=0739 isc=6706 isb=0001 daddr=c0015000 dobuf=2f686f6d
[3432826.420000] baddr=003217d8 dibuf=2f686f6d ver=f
[3432826.460000] Stack from 00c7df88:
[3432826.460000]         0000000e 0010f1b6 c0014fff 009d7ff2 0000000e 00000400 c0014fff c0014fff
[3432826.460000]         00000400 00674cd0 00da76c0 00674cd0 0073bbd0 009d7ff2 00000ff2 effffc4c
[3432826.460000]         000026c6 c0014fff 00000400 c0014fff c0014fff 00000400 80002dec effffcec
[3432826.460000]         80002db0 000000b7 000000b7 00000000 0208c00b 91700080
[3432826.690000] Call Trace: [<0010f1b6>] sys_getcwd+0xfe/0x11c
[3432826.750000]  [<000026c6>] syscall+0x8/0xc
[3432826.790000]
[3432826.830000] Code: 226f 0008 4a80 670a 2418 0e99 2800 5380 <66f6> 0801 0001 6706 3418 0e59 2800 0801 0000 6706 1418 0e19 2800 241f 4e75 2f02

Disassembly (from a different kernel image but same code, fault PC 0x1a3476 here):

001a3454 <__generic_copy_to_user>:
  1a3454:       2f02            movel %d2,%sp@-
  1a3456:       222f 0010       movel %sp@(16),%d1
  1a345a:       2001            movel %d1,%d0
  1a345c:       e488            lsrl #2,%d0
  1a345e:       7403            moveq #3,%d2
  1a3460:       c282            andl %d2,%d1
  1a3462:       206f 000c       moveal %sp@(12),%a0
  1a3466:       226f 0008       moveal %sp@(8),%a1
  1a346a:       4a80            tstl %d0
  1a346c:       670a            beqs 1a3478 <__generic_copy_to_user+0x24>
  1a346e:       2418            movel %a0@+,%d2
  1a3470:       0e99 2800       movesl %d2,%a1@+
  1a3474:       5380            subql #1,%d0
  1a3476:       66f6            bnes 1a346e <__generic_copy_to_user+0x1a>
  1a3478:       0801 0001       btst #1,%d1
  1a347c:       6706            beqs 1a3484 <__generic_copy_to_user+0x30>
  1a347e:       3418            movew %a0@+,%d2
  1a3480:       0e59 2800       movesw %d2,%a1@+
  1a3484:       0801 0000       btst #0,%d1
  1a3488:       6706            beqs 1a3490 <__generic_copy_to_user+0x3c>
  1a348a:       1418            moveb %a0@+,%d2
  1a348c:       0e19 2800       movesb %d2,%a1@+
  1a3490:       241f            movel %sp@+,%d2
  1a3492:       4e75            rts

Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-m68k@lists.linux-m68k.org
Link: https://lore.kernel.org/all/e0f23460779e6d16e2633486ac4841790ef2aca0.1713176294.git.fthain@linux-m68k.org
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/lib/uaccess.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
index 7646e461aa62..48643ef46900 100644
--- a/arch/m68k/lib/uaccess.c
+++ b/arch/m68k/lib/uaccess.c
@@ -60,20 +60,23 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
 
 	asm volatile ("\n"
 		"	tst.l	%0\n"
-		"	jeq	4f\n"
+		"	jeq	5f\n"
 		"1:	move.l	(%1)+,%3\n"
 		"2:	"MOVES".l	%3,(%2)+\n"
-		"3:	subq.l	#1,%0\n"
+		"3:	nop\n"
+		"4:	subq.l	#1,%0\n"
 		"	jne	1b\n"
-		"4:	btst	#1,%5\n"
-		"	jeq	6f\n"
-		"	move.w	(%1)+,%3\n"
-		"5:	"MOVES".w	%3,(%2)+\n"
-		"6:	btst	#0,%5\n"
+		"5:	btst	#1,%5\n"
 		"	jeq	8f\n"
+		"	move.w	(%1)+,%3\n"
+		"6:	"MOVES".w	%3,(%2)+\n"
+		"7:	nop\n"
+		"8:	btst	#0,%5\n"
+		"	jeq	11f\n"
 		"	move.b	(%1)+,%3\n"
-		"7:	"MOVES".b  %3,(%2)+\n"
-		"8:\n"
+		"9:	"MOVES".b  %3,(%2)+\n"
+		"10:	nop\n"
+		"11:\n"
 		"	.section .fixup,\"ax\"\n"
 		"	.even\n"
 		"20:	lsl.l	#2,%0\n"
@@ -85,10 +88,13 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
 		"	.align	4\n"
 		"	.long	2b,20b\n"
 		"	.long	3b,20b\n"
-		"	.long	5b,50b\n"
+		"	.long	4b,20b\n"
 		"	.long	6b,50b\n"
 		"	.long	7b,50b\n"
 		"	.long	8b,50b\n"
+		"	.long	9b,50b\n"
+		"	.long	10b,50b\n"
+		"	.long	11b,50b\n"
 		"	.previous"
 		: "=d" (res), "+a" (from), "+a" (to), "=&d" (tmp)
 		: "0" (n / 4), "d" (n & 3));
-- 
2.17.1

Re: [PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[Geert Uytterhoeven]

Hi Michael,

On Tue, Apr 16, 2024 at 10:36 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> I have tested this fault scenario with a transfer beginning
> at a single byte at the end of a mapped page only, which IMO
> does violate m68k alignment rules. I cannot remember ever
> seeing such faults reported from conforming software before.

68020+ does support unaligned data accesses, so IMHO such transfers
are valid.  For compatibility with 68000/68008/68010/68012, data is
recommended to be 2-byte aligned.
Instructions must always be aligned to 2 bytes, though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[Michael Schmitz]

Hi Geert,

Am 17.04.2024 um 18:59 schrieb Geert Uytterhoeven:
> Hi Michael,
>
> On Tue, Apr 16, 2024 at 10:36 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> I have tested this fault scenario with a transfer beginning
>> at a single byte at the end of a mapped page only, which IMO
>> does violate m68k alignment rules. I cannot remember ever
>> seeing such faults reported from conforming software before.
>
> 68020+ does support unaligned data accesses, so IMHO such transfers
> are valid.  For compatibility with 68000/68008/68010/68012, data is
> recommended to be 2-byte aligned.
> Instructions must always be aligned to 2 bytes, though.

Thanks - won't test this with two bytes offset from the page end then.

Unaligned access allowed means this is a valid fault scenario and Finn's 
fix is necessary...

Cheers,

	Michael

>
> Gr{oetje,eeting}s,
>
>                         Geert
>

Re: [PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[Eero Tamminen]

Hi,

On 17.4.2024 9.59, Geert Uytterhoeven wrote:
> On Tue, Apr 16, 2024 at 10:36 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> I have tested this fault scenario with a transfer beginning
>> at a single byte at the end of a mapped page only, which IMO
>> does violate m68k alignment rules. I cannot remember ever
>> seeing such faults reported from conforming software before.
> 
> 68020+ does support unaligned data accesses, so IMHO such transfers
> are valid.

But is that valid also across page boundaries?

I vaguely remember (from 15-20 years ago) there being ARM errata about 
some things being undefined when doing unaligned 16/32-bit data access 
across page boundary, so maybe it's not so clear cut on m68k either?


> For compatibility with 68000/68008/68010/68012, data is
> recommended to be 2-byte aligned.
> Instructions must always be aligned to 2 bytes, though.


	- Eero

PS. Quick googling found following ARM spec comment "a watchpoint might 
be undetected if it occurs on the second part of an unaligned access 
that crosses a 4K page boundary", but I remember issue being not just 
with watchpoints back then.

Re: [PATCH RFC] m68k: Handle __generic_copy_to_user faults more carefully

[Andreas Schwab]

On Apr 17 2024, Eero Tamminen wrote:

> But is that valid also across page boundaries?

Yes, the cpu will split misaligned accesses into sequences of aligned
accesses.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."