[syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    457391b03803 Linux 6.3
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=105b8bb0280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=385e197a58ca4afe
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/c35b5b2731d2/non_bootable_disk-457391b0.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/2a1bf3bafeb6/vmlinux-457391b0.xz
kernel image: https://storage.googleapis.com/syzbot-assets/21f1e3b4a5a9/zImage-457391b0.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+cb76c2983557a07cdb14@syzkaller.appspotmail.com

usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 96, size 116)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 5423 Comm: syz-executor.1 Not tainted 6.3.0-syzkaller #0
Hardware name: ARM-Versatile Express
PC is at usercopy_abort+0x98/0x9c mm/usercopy.c:102
LR is at __wake_up_klogd.part.0+0x7c/0xac kernel/printk/printk.c:3807
pc : [<817b706c>]    lr : [<802aef04>]    psr: 60000013
sp : dfe2de58  ip : dfe2dd98  fp : dfe2de7c
r10: 0000001a  r9 : 00003109  r8 : 83109760
r7 : dde67520  r6 : 00000000  r5 : 00000074  r4 : 00000060
r3 : 00000000  r2 : 00000000  r1 : ddddc584  r0 : 00000066
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 30c5387d  Table: 8671f680  DAC: 00000000
Register r0 information: non-paged memory
Register r1 information: non-slab/vmalloc memory
Register r2 information: NULL pointer
Register r3 information: NULL pointer
Register r4 information: non-paged memory
Register r5 information: non-paged memory
Register r6 information: NULL pointer
Register r7 information: non-slab/vmalloc memory
Register r8 information: slab task_struct start 83109700 pointer offset 96 size 2944
Register r9 information: non-paged memory
Register r10 information: non-paged memory
Register r11 information: 2-page vmalloc region starting at 0xdfe2c000 allocated at kernel_clone+0x9c/0x3d4 kernel/fork.c:2683
Register r12 information: 2-page vmalloc region starting at 0xdfe2c000 allocated at kernel_clone+0x9c/0x3d4 kernel/fork.c:2683
Process syz-executor.1 (pid: 5423, stack limit = 0xdfe2c000)
Stack: (0xdfe2de58 to 0xdfe2e000)
de40:                                                       81da9fcc 81d8176c
de60: 81d94abc 00000060 00000074 00003109 dfe2deac dfe2de80 804956ec 817b6fe0
de80: 00000074 dfe2de90 80216d0c 83109760 00000074 00000000 831097d4 dde67520
dea0: dfe2dee4 dfe2deb0 804b5624 80495620 00000074 0000000f dfe2ded4 83109760
dec0: 00000074 0000000f 00000000 00000000 83108b80 0000001a dfe2defc dfe2dee8
dee0: 80209fc8 804b5454 00000000 83109700 dfe2df74 dfe2df00 8020a728 80209f44
df00: 00000000 00000000 817dae24 802756e8 dfe2df74 dfe2df20 8027c28c 817dae00
df20: dfe2df3c 00000000 00000000 83108b80 80276968 60000013 8178d188 817a1de8
df40: dfe2df5c b463d43b 00000000 83109700 00000000 b463d43b 83109700 00000000
df60: 0000000f 00000000 dfe2dfa4 dfe2df78 80251188 8020a480 00000000 b463d43b
df80: 00000000 00000000 0014c2bc 0000001a 80200288 83108b80 00000000 dfe2dfa8
dfa0: 80200060 80250f20 00000000 00000000 0000000f 0000043c 00000000 00000000
dfc0: 00000000 00000000 0014c2bc 0000001a 7edab3c2 76bcb6d0 7edab534 76bcb20c
dfe0: 76bcb020 76bcb010 00017004 0004dfb0 60000010 0000000f 00000000 00000000
Backtrace: 
[<817b6fd4>] (usercopy_abort) from [<804956ec>] (__check_heap_object+0xd8/0xf4 mm/slub.c:4762)
[<80495614>] (__check_heap_object) from [<804b5624>] (check_heap_object mm/usercopy.c:196 [inline])
[<80495614>] (__check_heap_object) from [<804b5624>] (__check_object_size mm/usercopy.c:251 [inline])
[<80495614>] (__check_heap_object) from [<804b5624>] (__check_object_size+0x1dc/0x2fc mm/usercopy.c:213)
 r8:dde67520 r7:831097d4 r6:00000000 r5:00000074 r4:83109760
[<804b5448>] (__check_object_size) from [<80209fc8>] (check_object_size include/linux/thread_info.h:215 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (__copy_from_user include/linux/uaccess.h:79 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (user_regset_copyin include/linux/regset.h:268 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (fpa_set+0x90/0xb0 arch/arm/kernel/ptrace.c:589)
 r10:0000001a r9:83108b80 r8:00000000 r7:00000000 r6:0000000f r5:00000074
 r4:83109760
[<80209f38>] (fpa_set) from [<8020a728>] (copy_regset_from_user include/linux/regset.h:337 [inline])
[<80209f38>] (fpa_set) from [<8020a728>] (arch_ptrace+0x2b4/0x40c arch/arm/kernel/ptrace.c:764)
 r5:83109700 r4:00000000
[<8020a474>] (arch_ptrace) from [<80251188>] (__do_sys_ptrace kernel/ptrace.c:1296 [inline])
[<8020a474>] (arch_ptrace) from [<80251188>] (sys_ptrace+0x274/0x4f4 kernel/ptrace.c:1269)
 r7:00000000 r6:0000000f r5:00000000 r4:83109700
[<80250f14>] (sys_ptrace) from [<80200060>] (ret_fast_syscall+0x0/0x1c arch/arm/mm/proc-v7.S:66)
Exception stack(0xdfe2dfa8 to 0xdfe2dff0)
dfa0:                   00000000 00000000 0000000f 0000043c 00000000 00000000
dfc0: 00000000 00000000 0014c2bc 0000001a 7edab3c2 76bcb6d0 7edab534 76bcb20c
dfe0: 76bcb020 76bcb010 00017004 0004dfb0
 r9:83108b80 r8:80200288 r7:0000001a r6:0014c2bc r5:00000000 r4:00000000
Code: e3090fd0 e34801da e58dc000 ebfff87f (e7f001f2) 
---[ end trace 0000000000000000 ]---
----------------
Code disassembly (best guess):
   0:	e3090fd0 	movw	r0, #40912	; 0x9fd0
   4:	e34801da 	movt	r0, #33242	; 0x81da
   8:	e58dc000 	str	ip, [sp]
   c:	ebfff87f 	bl	0xffffe210
* 10:	e7f001f2 	udf	#18 <-- trapping instruction


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit:    457391b03803 Linux 6.3
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13565c32280000
kernel config:  https://syzkaller.appspot.com/x/.config?x=385e197a58ca4afe
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: arm
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16426cb8280000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=124f0d7a280000

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/c35b5b2731d2/non_bootable_disk-457391b0.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/2a1bf3bafeb6/vmlinux-457391b0.xz
kernel image: https://storage.googleapis.com/syzbot-assets/21f1e3b4a5a9/zImage-457391b0.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+cb76c2983557a07cdb14@syzkaller.appspotmail.com

usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 96, size 116)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 3090 Comm: syz-executor177 Not tainted 6.3.0-syzkaller #0
Hardware name: ARM-Versatile Express
PC is at usercopy_abort+0x98/0x9c mm/usercopy.c:102
LR is at __wake_up_klogd.part.0+0x7c/0xac kernel/printk/printk.c:3807
pc : [<817b706c>]    lr : [<802aef04>]    psr: 60000013
sp : dfaade58  ip : dfaadd98  fp : dfaade7c
r10: 0000001a  r9 : 00003e4d  r8 : 83e4dc60
r7 : dde85220  r6 : 00000000  r5 : 00000074  r4 : 00000060
r3 : 00000000  r2 : 00000000  r1 : ddddc584  r0 : 00000066
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 30c5387d  Table: 841e40c0  DAC: 00000000
Register r0 information: non-paged memory
Register r1 information: non-slab/vmalloc memory
Register r2 information: NULL pointer
Register r3 information: NULL pointer
Register r4 information: non-paged memory
Register r5 information: non-paged memory
Register r6 information: NULL pointer
Register r7 information: non-slab/vmalloc memory
Register r8 information: slab task_struct start 83e4dc00 pointer offset 96 size 2944
Register r9 information: non-paged memory
Register r10 information: non-paged memory
Register r11 information: 2-page vmalloc region starting at 0xdfaac000 allocated at kernel_clone+0x9c/0x3d4 kernel/fork.c:2683
Register r12 information: 2-page vmalloc region starting at 0xdfaac000 allocated at kernel_clone+0x9c/0x3d4 kernel/fork.c:2683
Process syz-executor177 (pid: 3090, stack limit = 0xdfaac000)
Stack: (0xdfaade58 to 0xdfaae000)
de40:                                                       81da9fcc 81d8176c
de60: 81d94abc 00000060 00000074 00003e4d dfaadeac dfaade80 804956ec 817b6fe0
de80: 00000074 dfaade90 80216d0c 83e4dc60 00000074 00000000 83e4dcd4 dde85220
dea0: dfaadee4 dfaadeb0 804b5624 80495620 00000074 0000000f dfaaded4 83e4dc60
dec0: 00000074 0000000f 00000000 00000000 833edc00 0000001a dfaadefc dfaadee8
dee0: 80209fc8 804b5454 00000000 83e4dc00 dfaadf74 dfaadf00 8020a728 80209f44
df00: 00000000 00000000 817dae24 802756e8 dfaadf74 dfaadf20 8027c28c 817dae00
df20: dfaadf3c 00000000 00000000 833edc00 80276968 60000013 8178d188 817a1de8
df40: dfaadf5c a154bf06 00000000 83e4dc00 00000000 a154bf06 83e4dc00 00000000
df60: 0000000f 00000000 dfaadfa4 dfaadf78 80251188 8020a480 00000000 a154bf06
df80: 00000000 00000000 000118c0 0000001a 80200288 833edc00 00000000 dfaadfa8
dfa0: 80200060 80250f20 00000000 00000000 0000000f 00000c13 00000000 00000000
dfc0: 00000000 00000000 000118c0 0000001a 000f4240 00000000 7ec23ca4 00003a97
dfe0: 7ec23c90 7ec23c80 00010624 0002a910 00000010 0000000f 00000000 00000000
Backtrace: 
[<817b6fd4>] (usercopy_abort) from [<804956ec>] (__check_heap_object+0xd8/0xf4 mm/slub.c:4762)
[<80495614>] (__check_heap_object) from [<804b5624>] (check_heap_object mm/usercopy.c:196 [inline])
[<80495614>] (__check_heap_object) from [<804b5624>] (__check_object_size mm/usercopy.c:251 [inline])
[<80495614>] (__check_heap_object) from [<804b5624>] (__check_object_size+0x1dc/0x2fc mm/usercopy.c:213)
 r8:dde85220 r7:83e4dcd4 r6:00000000 r5:00000074 r4:83e4dc60
[<804b5448>] (__check_object_size) from [<80209fc8>] (check_object_size include/linux/thread_info.h:215 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (__copy_from_user include/linux/uaccess.h:79 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (user_regset_copyin include/linux/regset.h:268 [inline])
[<804b5448>] (__check_object_size) from [<80209fc8>] (fpa_set+0x90/0xb0 arch/arm/kernel/ptrace.c:589)
 r10:0000001a r9:833edc00 r8:00000000 r7:00000000 r6:0000000f r5:00000074
 r4:83e4dc60
[<80209f38>] (fpa_set) from [<8020a728>] (copy_regset_from_user include/linux/regset.h:337 [inline])
[<80209f38>] (fpa_set) from [<8020a728>] (arch_ptrace+0x2b4/0x40c arch/arm/kernel/ptrace.c:764)
 r5:83e4dc00 r4:00000000
[<8020a474>] (arch_ptrace) from [<80251188>] (__do_sys_ptrace kernel/ptrace.c:1296 [inline])
[<8020a474>] (arch_ptrace) from [<80251188>] (sys_ptrace+0x274/0x4f4 kernel/ptrace.c:1269)
 r7:00000000 r6:0000000f r5:00000000 r4:83e4dc00
[<80250f14>] (sys_ptrace) from [<80200060>] (ret_fast_syscall+0x0/0x1c arch/arm/mm/proc-v7.S:66)
Exception stack(0xdfaadfa8 to 0xdfaadff0)
dfa0:                   00000000 00000000 0000000f 00000c13 00000000 00000000
dfc0: 00000000 00000000 000118c0 0000001a 000f4240 00000000 7ec23ca4 00003a97
dfe0: 7ec23c90 7ec23c80 00010624 0002a910
 r9:833edc00 r8:80200288 r7:0000001a r6:000118c0 r5:00000000 r4:00000000
Code: e3090fd0 e34801da e58dc000 ebfff87f (e7f001f2) 
---[ end trace 0000000000000000 ]---
----------------
Code disassembly (best guess):
   0:	e3090fd0 	movw	r0, #40912	; 0x9fd0
   4:	e34801da 	movt	r0, #33242	; 0x81da
   8:	e58dc000 	str	ip, [sp]
   c:	ebfff87f 	bl	0xffffe210
* 10:	e7f001f2 	udf	#18 <-- trapping instruction


---
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..29b28961637c 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -573,8 +573,10 @@ static int fpa_get(struct task_struct *target,
 		   const struct user_regset *regset,
 		   struct membuf to)
 {
-	return membuf_write(&to, &task_thread_info(target)->fpstate,
-				 sizeof(struct user_fp));
+	struct thread_info *thread = task_thread_info(target);
+
+	return membuf_write(&to, &thread->fpstate,
+				 sizeof(thread->fpstate));
 }
 
 static int fpa_set(struct task_struct *target,
@@ -586,7 +588,7 @@ static int fpa_set(struct task_struct *target,
 
 	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 		&thread->fpstate,
-		0, sizeof(struct user_fp));
+		0, sizeof(thread->fpstate));
 }
 
 #ifdef CONFIG_VFP

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
BUG: bad usercopy in fpa_set

usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 80, size 140)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 3917 Comm: syz-executor.0 Not tainted 6.8.0-rc7-syzkaller #0
Hardware name: ARM-Versatile Express
PC is at usercopy_abort+0x98/0x9c mm/usercopy.c:102
LR is at __wake_up_klogd.part.0+0x7c/0xac kernel/printk/printk.c:3899
pc : [<8183e740>]    lr : [<802b7f34>]    psr: 60000113
sp : df9d5e50  ip : df9d5d98  fp : df9d5e74
r10: 0000001a  r9 : 83d59800  r8 : 84ccd450
r7 : ddea5c20  r6 : 00000000  r5 : 0000008c  r4 : 00000050
r3 : 83d59800  r2 : 00000000  r1 : 00000000  r0 : 00000066
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 30c5387d  Table: 84b822c0  DAC: 00000000
Register r0 information: non-paged memory
Register r1 information: NULL pointer
Register r2 information: NULL pointer
Register r3 information: slab task_struct start 83d59800 pointer offset 0 size 3072
Register r4 information: non-paged memory
Register r5 information: non-paged memory
Register r6 information: NULL pointer
Register r7 information: non-slab/vmalloc memory
Register r8 information: slab task_struct start 84ccd400 pointer offset 80 size 3072
Register r9 information: slab task_struct start 83d59800 pointer offset 0 size 3072
Register r10 information: non-paged memory
Register r11 information: 2-page vmalloc region starting at 0xdf9d4000 allocated at kernel_clone+0xac/0x3c8 kernel/fork.c:2902
Register r12 information: 2-page vmalloc region starting at 0xdf9d4000 allocated at kernel_clone+0xac/0x3c8 kernel/fork.c:2902
Process syz-executor.0 (pid: 3917, stack limit = 0xdf9d4000)
Stack: (0xdf9d5e50 to 0xdf9d6000)
5e40:                                     81fda684 81fadca8 81fc2424 00000050
5e60: 0000008c 83d59800 df9d5ea4 df9d5e78 804a922c 8183e6b4 0000008c df9d5e88
5e80: 80216278 84ccd450 0000008c 00000000 84ccd4dc ddea5c20 df9d5edc df9d5ea8
5ea0: 804e1c20 804a9160 0000008c 00000001 df9d5ecc 84ccd450 0000008c 00000001
5ec0: 00000000 00000000 83d59800 0000001a df9d5ef4 df9d5ee0 8020a090 804e1a40
5ee0: 00000000 0000000c df9d5f6c df9d5ef8 8020a680 8020a01c 00000000 00000000
5f00: df9d5f1c df9d5f10 81862d34 802798b0 df9d5f6c df9d5f20 8027f524 81862d10
5f20: df9d5f54 00000000 8027b25c 60000013 818110f0 81827f88 df9d5f54 b2f514c9
5f40: 0000000f 84ccd400 0000000f b2f514c9 84ccd400 0000000f 00000001 00000000
5f60: df9d5fa4 df9d5f70 80253494 8020a398 8020301c b2f514c9 df9d5fac 00000000
5f80: 00000000 0014c2cc 0000001a 80200288 83d59800 0000001a 00000000 df9d5fa8
5fa0: 80200060 80253268 00000000 00000000 0000000f 00000004 00000001 00000000
5fc0: 00000000 00000000 0014c2cc 0000001a 7e8da326 7e8da327 003d0f00 76bf70fc
5fe0: 76bf6f08 76bf6ef8 000167e8 00050bd0 60000010 0000000f 00000000 00000000
Backtrace: 
[<8183e6a8>] (usercopy_abort) from [<804a922c>] (__check_heap_object+0xd8/0xf4 mm/slub.c:5386)
[<804a9154>] (__check_heap_object) from [<804e1c20>] (check_heap_object mm/usercopy.c:196 [inline])
[<804a9154>] (__check_heap_object) from [<804e1c20>] (__check_object_size mm/usercopy.c:251 [inline])
[<804a9154>] (__check_heap_object) from [<804e1c20>] (__check_object_size+0x1ec/0x30c mm/usercopy.c:213)
 r8:ddea5c20 r7:84ccd4dc r6:00000000 r5:0000008c r4:84ccd450
[<804e1a34>] (__check_object_size) from [<8020a090>] (check_object_size include/linux/thread_info.h:215 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (__copy_from_user include/linux/uaccess.h:101 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (user_regset_copyin include/linux/regset.h:268 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (fpa_set+0x80/0xa0 arch/arm/kernel/ptrace.c:589)
 r10:0000001a r9:83d59800 r8:00000000 r7:00000000 r6:00000001 r5:0000008c
 r4:84ccd450
[<8020a010>] (fpa_set) from [<8020a680>] (copy_regset_from_user include/linux/regset.h:337 [inline])
[<8020a010>] (fpa_set) from [<8020a680>] (arch_ptrace+0x2f4/0x3e4 arch/arm/kernel/ptrace.c:764)
 r5:0000000c r4:00000000
[<8020a38c>] (arch_ptrace) from [<80253494>] (__do_sys_ptrace kernel/ptrace.c:1288 [inline])
[<8020a38c>] (arch_ptrace) from [<80253494>] (sys_ptrace+0x238/0x4dc kernel/ptrace.c:1261)
 r7:00000000 r6:00000001 r5:0000000f r4:84ccd400
[<8025325c>] (sys_ptrace) from [<80200060>] (ret_fast_syscall+0x0/0x1c arch/arm/mm/proc-v7.S:66)
Exception stack(0xdf9d5fa8 to 0xdf9d5ff0)
5fa0:                   00000000 00000000 0000000f 00000004 00000001 00000000
5fc0: 00000000 00000000 0014c2cc 0000001a 7e8da326 7e8da327 003d0f00 76bf70fc
5fe0: 76bf6f08 76bf6ef8 000167e8 00050bd0
 r10:0000001a r9:83d59800 r8:80200288 r7:0000001a r6:0014c2cc r5:00000000
 r4:00000000
Code: e30a0688 e34801fd e58dc000 ebfff35b (e7f001f2) 
---[ end trace 0000000000000000 ]---
----------------
Code disassembly (best guess):
   0:	e30a0688 	movw	r0, #42632	@ 0xa688
   4:	e34801fd 	movt	r0, #33277	@ 0x81fd
   8:	e58dc000 	str	ip, [sp]
   c:	ebfff35b 	bl	0xffffcd80
* 10:	e7f001f2 	udf	#18 <-- trapping instruction


Tested on:

commit:         90d35da6 Linux 6.8-rc7
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17fdb512180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=57d422b95aec4095
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm
patch:          https://syzkaller.appspot.com/x/patch.diff?x=16e3042e180000

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..b47e56a87dfa 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -573,8 +573,10 @@ static int fpa_get(struct task_struct *target,
 		   const struct user_regset *regset,
 		   struct membuf to)
 {
-	return membuf_write(&to, &task_thread_info(target)->fpstate,
-				 sizeof(struct user_fp));
+	struct thread_info *thread = task_thread_info(target);
+
+	return membuf_write(&to, &thread->fpstate,
+				 sizeof(thread->fpstate));
 }
 
 static int fpa_set(struct task_struct *target,
@@ -586,7 +588,7 @@ static int fpa_set(struct task_struct *target,
 
 	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
 		&thread->fpstate,
-		0, sizeof(struct user_fp));
+		0, sizeof(thread->fpstate));
 }
 
 #ifdef CONFIG_VFP
@@ -690,7 +692,7 @@ static const struct user_regset arm_regsets[] = {
 		 * of sizes, so pretend that the registers are word-sized:
 		 */
 		.core_note_type = NT_PRFPREG,
-		.n = sizeof(struct user_fp) / sizeof(u32),
+		.n = sizeof(union fp_state) / sizeof(u32),
 		.size = sizeof(u32),
 		.align = sizeof(u32),
 		.regset_get = fpa_get,

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
BUG: bad usercopy in fpa_set

usercopy: Kernel memory overwrite attempt detected to SLUB object 'task_struct' (offset 80, size 140)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 3920 Comm: syz-executor.0 Not tainted 6.8.0-rc7-syzkaller #0
Hardware name: ARM-Versatile Express
PC is at usercopy_abort+0x98/0x9c mm/usercopy.c:102
LR is at __wake_up_klogd.part.0+0x7c/0xac kernel/printk/printk.c:3899
pc : [<8183e740>]    lr : [<802b7f34>]    psr: 60000013
sp : df9e9e50  ip : df9e9d98  fp : df9e9e74
r10: 0000001a  r9 : 840c9800  r8 : 83735450
r7 : dde752c0  r6 : 00000000  r5 : 0000008c  r4 : 00000050
r3 : 840c9800  r2 : 00000000  r1 : 00000000  r0 : 00000066
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 30c5387d  Table: 84841ec0  DAC: fffffffd
Register r0 information: non-paged memory
Register r1 information: NULL pointer
Register r2 information: NULL pointer
Register r3 information: slab task_struct start 840c9800 pointer offset 0 size 3072
Register r4 information: non-paged memory
Register r5 information: non-paged memory
Register r6 information: NULL pointer
Register r7 information: non-slab/vmalloc memory
Register r8 information: slab task_struct start 83735400 pointer offset 80 size 3072
Register r9 information: slab task_struct start 840c9800 pointer offset 0 size 3072
Register r10 information: non-paged memory
Register r11 information: 2-page vmalloc region starting at 0xdf9e8000 allocated at kernel_clone+0xac/0x3c8 kernel/fork.c:2902
Register r12 information: 2-page vmalloc region starting at 0xdf9e8000 allocated at kernel_clone+0xac/0x3c8 kernel/fork.c:2902
Process syz-executor.0 (pid: 3920, stack limit = 0xdf9e8000)
Stack: (0xdf9e9e50 to 0xdf9ea000)
9e40:                                     81fda684 81fadca8 81fc2424 00000050
9e60: 0000008c 840c9800 df9e9ea4 df9e9e78 804a922c 8183e6b4 0000008c df9e9e88
9e80: 80216278 83735450 0000008c 00000000 837354dc dde752c0 df9e9edc df9e9ea8
9ea0: 804e1c20 804a9160 0000008c 00000001 df9e9ecc 83735450 0000008c 00000001
9ec0: 00000000 00000000 840c9800 0000001a df9e9ef4 df9e9ee0 8020a090 804e1a40
9ee0: 00000000 0000000c df9e9f6c df9e9ef8 8020a680 8020a01c 00000000 00000000
9f00: df9e9f1c df9e9f10 81862d34 802798b0 df9e9f6c df9e9f20 8027f524 81862d10
9f20: df9e9f54 00000000 8027b25c 60000013 818110f0 81827f88 df9e9f54 553a7b00
9f40: 0000000f 83735400 0000000f 553a7b00 83735400 0000000f 00000001 00000000
9f60: df9e9fa4 df9e9f70 80253494 8020a398 8020301c 553a7b00 df9e9fac 00000000
9f80: 00000000 0014c2cc 0000001a 80200288 840c9800 0000001a 00000000 df9e9fa8
9fa0: 80200060 80253268 00000000 00000000 0000000f 00000004 00000001 00000000
9fc0: 00000000 00000000 0014c2cc 0000001a 7e859326 7e859327 003d0f00 76bd60fc
9fe0: 76bd5f08 76bd5ef8 000167e8 00050bd0 60000010 0000000f 00000000 00000000
Backtrace: 
[<8183e6a8>] (usercopy_abort) from [<804a922c>] (__check_heap_object+0xd8/0xf4 mm/slub.c:5386)
[<804a9154>] (__check_heap_object) from [<804e1c20>] (check_heap_object mm/usercopy.c:196 [inline])
[<804a9154>] (__check_heap_object) from [<804e1c20>] (__check_object_size mm/usercopy.c:251 [inline])
[<804a9154>] (__check_heap_object) from [<804e1c20>] (__check_object_size+0x1ec/0x30c mm/usercopy.c:213)
 r8:dde752c0 r7:837354dc r6:00000000 r5:0000008c r4:83735450
[<804e1a34>] (__check_object_size) from [<8020a090>] (check_object_size include/linux/thread_info.h:215 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (__copy_from_user include/linux/uaccess.h:101 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (user_regset_copyin include/linux/regset.h:268 [inline])
[<804e1a34>] (__check_object_size) from [<8020a090>] (fpa_set+0x80/0xa0 arch/arm/kernel/ptrace.c:589)
 r10:0000001a r9:840c9800 r8:00000000 r7:00000000 r6:00000001 r5:0000008c
 r4:83735450
[<8020a010>] (fpa_set) from [<8020a680>] (copy_regset_from_user include/linux/regset.h:337 [inline])
[<8020a010>] (fpa_set) from [<8020a680>] (arch_ptrace+0x2f4/0x3e4 arch/arm/kernel/ptrace.c:764)
 r5:0000000c r4:00000000
[<8020a38c>] (arch_ptrace) from [<80253494>] (__do_sys_ptrace kernel/ptrace.c:1288 [inline])
[<8020a38c>] (arch_ptrace) from [<80253494>] (sys_ptrace+0x238/0x4dc kernel/ptrace.c:1261)
 r7:00000000 r6:00000001 r5:0000000f r4:83735400
[<8025325c>] (sys_ptrace) from [<80200060>] (ret_fast_syscall+0x0/0x1c arch/arm/mm/proc-v7.S:66)
Exception stack(0xdf9e9fa8 to 0xdf9e9ff0)
9fa0:                   00000000 00000000 0000000f 00000004 00000001 00000000
9fc0: 00000000 00000000 0014c2cc 0000001a 7e859326 7e859327 003d0f00 76bd60fc
9fe0: 76bd5f08 76bd5ef8 000167e8 00050bd0
 r10:0000001a r9:840c9800 r8:80200288 r7:0000001a r6:0014c2cc r5:00000000
 r4:00000000
Code: e30a0688 e34801fd e58dc000 ebfff35b (e7f001f2) 
---[ end trace 0000000000000000 ]---
----------------
Code disassembly (best guess):
   0:	e30a0688 	movw	r0, #42632	@ 0xa688
   4:	e34801fd 	movt	r0, #33277	@ 0x81fd
   8:	e58dc000 	str	ip, [sp]
   c:	ebfff35b 	bl	0xffffcd80
* 10:	e7f001f2 	udf	#18 <-- trapping instruction


Tested on:

commit:         90d35da6 Linux 6.8-rc7
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=132bc62a180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=57d422b95aec4095
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm
patch:          https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 326864f79d18..0f70a68d730a 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -37,14 +37,11 @@ struct thread_struct {
 	struct debug_info	debug;
 };
 
-/*
- * Everything usercopied to/from thread_struct is statically-sized, so
- * no hardened usercopy whitelist is needed.
- */
 static inline void arch_thread_struct_whitelist(unsigned long *offset,
 						unsigned long *size)
 {
-	*offset = *size = 0;
+	*offset = offsetof(struct task_struct, thread_info);
+	*size = sizeof(struct thread_info);
 }
 
 #define INIT_THREAD  {	}

Re: [syzbot] [arm] BUG: bad usercopy in fpa_set

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

./include/linux/stddef.h:16:33: error: invalid use of undefined type 'struct task_struct'
./arch/arm/include/asm/processor.h:44:24: error: invalid application of 'sizeof' to incomplete type 'struct thread_info'


Tested on:

commit:         8cb4a9a8 x86/cpufeatures: Add CPUID_LNX_5 to track rec..
git tree:       upstream
kernel config:  https://syzkaller.appspot.com/x/.config?x=f0bf9b3a0ca93c59
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm
patch:          https://syzkaller.appspot.com/x/patch.diff?x=157e1bb5180000

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..347611ae762f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
 		   const void *kbuf, const void __user *ubuf)
 {
 	struct thread_info *thread = task_thread_info(target);
+	const unsigned int pos0 = pos;
+	char buf[sizeof(struct user_fp)];
+	int ret;
 
-	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-		&thread->fpstate,
-		0, sizeof(struct user_fp));
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+				 buf, 0, sizeof(struct user_fp));
+	if (!ret)
+		memcpy(&thread->fpstate, buf, pos - pos0);
+	return ret;
 }
 
 #ifdef CONFIG_VFP

Re: [syzbot] [arm] BUG: bad usercopy in fpa_set

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+cb76c2983557a07cdb14@syzkaller.appspotmail.com

Tested on:

commit:         8cb4a9a8 x86/cpufeatures: Add CPUID_LNX_5 to track rec..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14d6c52d180000
kernel config:  https://syzkaller.appspot.com/x/.config?x=39e01873cab062c1
dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
compiler:       arm-linux-gnueabi-gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
userspace arch: arm
patch:          https://syzkaller.appspot.com/x/patch.diff?x=14c42099180000

Note: testing is done by a robot and is best-effort only.

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

Hello.

syzbot is reporting kernel memory overwrite attempt at fpa_set().
I guessed that the amount to copy from/to should be sizeof(union fp_state)
than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm
is using offset == 0 and size == sizeof(union fp_state). But my guess did not
solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ).

On 2023/05/05 21:53, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    457391b03803 Linux 6.3
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=105b8bb0280000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=385e197a58ca4afe
> dashboard link: https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14
> compiler:       arm-linux-gnueabi-gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> userspace arch: arm

#syz set subsystems: arm

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

On Tue, Mar 05, 2024 at 08:27:07PM +0900, Tetsuo Handa wrote:
> Hello.
> 
> syzbot is reporting kernel memory overwrite attempt at fpa_set().
> I guessed that the amount to copy from/to should be sizeof(union fp_state)
> than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm
> is using offset == 0 and size == sizeof(union fp_state). But my guess did not
> solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ).

This is silly.

sizeof(struct user_fp) is:

	8 * (
		1 bit for sign1, 15 bits unused => 2 bytes
		1 bit for sign2, 14 bits unused, 1 bit for j => 2 bytes
		31 bits for mantissa1 => 4 bytes
		32 bits for mantissa0 => 4 bytes
	) +
	4 bytes for fpsr
	4 bytes for fpcr
	8 bytes for ftype
	4 bytes for init_flag

This totals 8 * 12 + 4 + 4 + 8 + 4 = 116 bytes or 29 32-bit quantities,
or 29 "unsigned int"s.

This is copied into union fp_state. This union is made up of one of
several different formats depending on the FP being used. user_fp
doesn't reflect this. However, one of these, struct fp_soft_struct,
is specifically sized to ensure that user_fp is _smaller_.

struct fp_soft_struct is 35 unsigned int's. This is 140 bytes. This
is larger than sizeof(user_fp).

Therefore, there is _no way_ for fpa_set() to overwrite anything
outside of thread_info->fpstate, because sizeof(struct user_fp)
is smaller than sizeof(thread->fpstate).

Syzbot appears to be wrong in this instance.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

On 2024/04/04 1:12, Russell King (Oracle) wrote:
> Therefore, there is _no way_ for fpa_set() to overwrite anything
> outside of thread_info->fpstate, because sizeof(struct user_fp)
> is smaller than sizeof(thread->fpstate).
> 
> Syzbot appears to be wrong in this instance.
> 

Thanks for clarification.

I came to suspect that commit 08626a6056aa ("arm: Implement thread_struct
whitelist for hardened usercopy") missed that ptrace(PTRACE_SETFPREGS)
needs to declare a usercopy whitelist. It seems to me that
https://syzkaller.appspot.com/text?tag=Patch&x=14c42099180000 can fix
this problem, but I'm not sure whether this is safe/correct. Can you check?

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Mark Rutland]

On Wed, Apr 03, 2024 at 05:12:07PM +0100, Russell King (Oracle) wrote:
> On Tue, Mar 05, 2024 at 08:27:07PM +0900, Tetsuo Handa wrote:
> > Hello.
> > 
> > syzbot is reporting kernel memory overwrite attempt at fpa_set().
> > I guessed that the amount to copy from/to should be sizeof(union fp_state)
> > than sizeof(struct user_fp), for arch_ptrace(PTRACE_[SG]ETFPREGS) for arm
> > is using offset == 0 and size == sizeof(union fp_state). But my guess did not
> > solve the issue ( https://syzkaller.appspot.com/x/patch.diff?x=11e46dbc180000 ).
> 
> This is silly.
> 
> sizeof(struct user_fp) is:
> 
> 	8 * (
> 		1 bit for sign1, 15 bits unused => 2 bytes
> 		1 bit for sign2, 14 bits unused, 1 bit for j => 2 bytes
> 		31 bits for mantissa1 => 4 bytes
> 		32 bits for mantissa0 => 4 bytes
> 	) +
> 	4 bytes for fpsr
> 	4 bytes for fpcr
> 	8 bytes for ftype
> 	4 bytes for init_flag
> 
> This totals 8 * 12 + 4 + 4 + 8 + 4 = 116 bytes or 29 32-bit quantities,
> or 29 "unsigned int"s.
> 
> This is copied into union fp_state. This union is made up of one of
> several different formats depending on the FP being used. user_fp
> doesn't reflect this. However, one of these, struct fp_soft_struct,
> is specifically sized to ensure that user_fp is _smaller_.
> 
> struct fp_soft_struct is 35 unsigned int's. This is 140 bytes. This
> is larger than sizeof(user_fp).
> 
> Therefore, there is _no way_ for fpa_set() to overwrite anything
> outside of thread_info->fpstate, because sizeof(struct user_fp)
> is smaller than sizeof(thread->fpstate).
> 
> Syzbot appears to be wrong in this instance.

I believe the problem here is that HARDENED_USERCOPY tries to prevent any
usercopy to/from task_struct except for fields that are explicitly whitelisted
(which all need to be in one contiguous range). That was added in commit:

  5905429ad85657c2 ("fork: Provide usercopy whitelisting for task_struct")

However, architectures only have the option to provide
arch_thread_struct_whitelist() to whitelist some fields in thread_struct, not
thread_info where the fp_state lives. On arm arch_thread_struct_whitelist()
whitelists precisely nothing:

  static inline void arch_thread_struct_whitelist(unsigned long *offset,
                                                 unsigned long *size)
  {
         *offset = *size = 0;
  }

... which was added in commit:

  08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")

That commit says that all accesses are bounce-buffered and bypass the check,
but AFAICT the fpa_set() code hasn't changed since then, so either that was
wrong or the user_regset_copyin() code has changed.

Kees, I believe you need to look at this.

See the dashboard page at:

  https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14

Mark.

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

On 2024/04/15 18:02, Mark Rutland wrote:
>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> 
> That commit says that all accesses are bounce-buffered and bypass the check,
> but AFAICT the fpa_set() code hasn't changed since then, so either that was
> wrong or the user_regset_copyin() code has changed.

Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
> On 2024/04/15 18:02, Mark Rutland wrote:
> >   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> > 
> > That commit says that all accesses are bounce-buffered and bypass the check,
> > but AFAICT the fpa_set() code hasn't changed since then, so either that was
> > wrong or the user_regset_copyin() code has changed.
> 
> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?

Have you visited that URL? It doesn't point to an email containing a
patch, so sorry, I don't know what patch you're referring to.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Tetsuo Handa]

On 2024/04/15 18:44, Russell King (Oracle) wrote:
> On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
>> On 2024/04/15 18:02, Mark Rutland wrote:
>>>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
>>>
>>> That commit says that all accesses are bounce-buffered and bypass the check,
>>> but AFAICT the fpa_set() code hasn't changed since then, so either that was
>>> wrong or the user_regset_copyin() code has changed.
>>
>> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?
> 
> Have you visited that URL? It doesn't point to an email containing a
> patch, so sorry, I don't know what patch you're referring to.
> 

Containing a link to a diff. ;-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index c421a899fc84..347611ae762f 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
 		   const void *kbuf, const void __user *ubuf)
 {
 	struct thread_info *thread = task_thread_info(target);
+	const unsigned int pos0 = pos;
+	char buf[sizeof(struct user_fp)];
+	int ret;
 
-	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-		&thread->fpstate,
-		0, sizeof(struct user_fp));
+	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
+				 buf, 0, sizeof(struct user_fp));
+	if (!ret)
+		memcpy(&thread->fpstate, buf, pos - pos0);
+	return ret;
 }
 
 #ifdef CONFIG_VFP

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Russell King (Oracle)]

On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote:
> On 2024/04/15 18:44, Russell King (Oracle) wrote:
> > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
> >> On 2024/04/15 18:02, Mark Rutland wrote:
> >>>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> >>>
> >>> That commit says that all accesses are bounce-buffered and bypass the check,
> >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was
> >>> wrong or the user_regset_copyin() code has changed.
> >>
> >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?
> > 
> > Have you visited that URL? It doesn't point to an email containing a
> > patch, so sorry, I don't know what patch you're referring to.
> > 
> 
> Containing a link to a diff. ;-)
> 
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index c421a899fc84..347611ae762f 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
>  		   const void *kbuf, const void __user *ubuf)
>  {
>  	struct thread_info *thread = task_thread_info(target);
> +	const unsigned int pos0 = pos;
> +	char buf[sizeof(struct user_fp)];
> +	int ret;
>  
> -	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> -		&thread->fpstate,
> -		0, sizeof(struct user_fp));
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +				 buf, 0, sizeof(struct user_fp));
> +	if (!ret)
> +		memcpy(&thread->fpstate, buf, pos - pos0);
> +	return ret;
>  }
>  
>  #ifdef CONFIG_VFP

No, not unless there is really no other option. It's hacking around the
issue, creating two copy operations of the data (one onto the stack)
rather than solving it properly - and I will not put up with that kind
of mentality - it's a completely broken approach to open source
software. If there is a problem, always fix it using the correct fix,
never try to sticky-plaster around a problem.

It seems there is a way for architectures to tell the code what is
safe to write to, and it seems that a misunderstanding meant this
wasn't implemented. So let's see whether it's possible to fix that
first.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Mark Rutland]

On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote:
> On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote:
> > On 2024/04/15 18:44, Russell King (Oracle) wrote:
> > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
> > >> On 2024/04/15 18:02, Mark Rutland wrote:
> > >>>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> > >>>
> > >>> That commit says that all accesses are bounce-buffered and bypass the check,
> > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was
> > >>> wrong or the user_regset_copyin() code has changed.
> > >>
> > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?
> > > 
> > > Have you visited that URL? It doesn't point to an email containing a
> > > patch, so sorry, I don't know what patch you're referring to.
> > > 
> > 
> > Containing a link to a diff. ;-)
> > 
> > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> > index c421a899fc84..347611ae762f 100644
> > --- a/arch/arm/kernel/ptrace.c
> > +++ b/arch/arm/kernel/ptrace.c
> > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
> >  		   const void *kbuf, const void __user *ubuf)
> >  {
> >  	struct thread_info *thread = task_thread_info(target);
> > +	const unsigned int pos0 = pos;
> > +	char buf[sizeof(struct user_fp)];
> > +	int ret;
> >  
> > -	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > -		&thread->fpstate,
> > -		0, sizeof(struct user_fp));
> > +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > +				 buf, 0, sizeof(struct user_fp));
> > +	if (!ret)
> > +		memcpy(&thread->fpstate, buf, pos - pos0);
> > +	return ret;
> >  }
> >  
> >  #ifdef CONFIG_VFP
> 
> No, not unless there is really no other option. It's hacking around the
> issue, creating two copy operations of the data (one onto the stack)
> rather than solving it properly - and I will not put up with that kind
> of mentality - it's a completely broken approach to open source
> software. If there is a problem, always fix it using the correct fix,
> never try to sticky-plaster around a problem.
> 
> It seems there is a way for architectures to tell the code what is
> safe to write to, and it seems that a misunderstanding meant this
> wasn't implemented. So let's see whether it's possible to fix that
> first.

I completely agree.

We'll have to wait for Kees to wake up, but IIUC one assumption here was that
thread_info was particularly sensitive, and hence any state to be copied
to/from userspace would live in thread_struct. Either we need to remove that
assumption, or we need to move things so that we can use
arch_thread_struct_whitelist().

Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly
mechanical change to move fp_state (and vfp_state!) into thread_struct. That
would mean that the TI_FPSTATE offset would grow, but assuming that still fits
into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist()
could be used to handle these structures.

Mark.

Re: [syzbot] [hardening?] [mm?] BUG: bad usercopy in fpa_set

[Kees Cook]

On Mon, Apr 15, 2024 at 12:43:59PM +0100, Mark Rutland wrote:
> On Mon, Apr 15, 2024 at 11:27:02AM +0100, Russell King (Oracle) wrote:
> > On Mon, Apr 15, 2024 at 06:58:30PM +0900, Tetsuo Handa wrote:
> > > On 2024/04/15 18:44, Russell King (Oracle) wrote:
> > > > On Mon, Apr 15, 2024 at 06:38:33PM +0900, Tetsuo Handa wrote:
> > > >> On 2024/04/15 18:02, Mark Rutland wrote:
> > > >>>   08626a6056aad824 ("arm: Implement thread_struct whitelist for hardened usercopy")
> > > >>>
> > > >>> That commit says that all accesses are bounce-buffered and bypass the check,
> > > >>> but AFAICT the fpa_set() code hasn't changed since then, so either that was
> > > >>> wrong or the user_regset_copyin() code has changed.
> > > >>
> > > >> Then, can we go with https://lkml.kernel.org/r/0b49d91b-511f-449e-b7c3-93b2ccce6c49@I-love.SAKURA.ne.jp ?
> > > > 
> > > > Have you visited that URL? It doesn't point to an email containing a
> > > > patch, so sorry, I don't know what patch you're referring to.
> > > > 
> > > 
> > > Containing a link to a diff. ;-)
> > > 
> > > diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> > > index c421a899fc84..347611ae762f 100644
> > > --- a/arch/arm/kernel/ptrace.c
> > > +++ b/arch/arm/kernel/ptrace.c
> > > @@ -583,10 +583,15 @@ static int fpa_set(struct task_struct *target,
> > >  		   const void *kbuf, const void __user *ubuf)
> > >  {
> > >  	struct thread_info *thread = task_thread_info(target);
> > > +	const unsigned int pos0 = pos;
> > > +	char buf[sizeof(struct user_fp)];
> > > +	int ret;
> > >  
> > > -	return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > > -		&thread->fpstate,
> > > -		0, sizeof(struct user_fp));
> > > +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> > > +				 buf, 0, sizeof(struct user_fp));
> > > +	if (!ret)
> > > +		memcpy(&thread->fpstate, buf, pos - pos0);
> > > +	return ret;
> > >  }
> > >  
> > >  #ifdef CONFIG_VFP
> > 
> > No, not unless there is really no other option. It's hacking around the
> > issue, creating two copy operations of the data (one onto the stack)
> > rather than solving it properly - and I will not put up with that kind
> > of mentality - it's a completely broken approach to open source
> > software. If there is a problem, always fix it using the correct fix,
> > never try to sticky-plaster around a problem.
> > 
> > It seems there is a way for architectures to tell the code what is
> > safe to write to, and it seems that a misunderstanding meant this
> > wasn't implemented. So let's see whether it's possible to fix that
> > first.
> 
> I completely agree.

FWIW, the bound buffer approach is used in other places as well
(especially for places that are not fastpath, like this case), as it
creates an explicit bounds check (i.e. the buffer on the stack). But
yes, if there is a way to specifically allow this during the kmem setup,
let's do that.

The timeline on this is interesting, it first showed up on syzbot about
a year ago (2023/05/05 12:53)
https://syzkaller.appspot.com/bug?extid=cb76c2983557a07cdb14

But hardened usercopy (and commit 08626a6056aad824) are from Jan 2018.
No one noticed for 5 years? :P

> We'll have to wait for Kees to wake up, but IIUC one assumption here was that
> thread_info was particularly sensitive, and hence any state to be copied
> to/from userspace would live in thread_struct. Either we need to remove that
> assumption, or we need to move things so that we can use
> arch_thread_struct_whitelist().
> 
> Given that arm always selects THREAD_INFO_IN_TASK, I think it would be a fairly
> mechanical change to move fp_state (and vfp_state!) into thread_struct. That
> would mean that the TI_FPSTATE offset would grow, but assuming that still fits
> into an ADD immediate, we'd be ok, and then arch_thread_struct_whitelist()
> could be used to handle these structures.

Sure, or use a bounce buffer, since no one noticed this for 5 years
it probably isn't heavy used[1]? But sure, I'm open to whatever --
the point is to not arbitrarily allow usercopy to touch the memory here.

-Kees

[1] We probably can't remove PTRACE_SETFPREGS, but yeah, it's really
uncommon:
https://codesearch.debian.net/search?q=ptrace%28PTRACE_SETFPREGS%2C&literal=1&perpkg=1
- systemtap test suite uses it
- rr is x86 only
- crui doesn't have a arm backend
- python ptrace may use it
- edb-debugger is x86 only
- llvm mentions it in comments only

-- 
Kees Cook