Hello,
syzbot found the following issue on:
HEAD commit: fa55b7dcdc43 Linux 5.16-rc1
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15fe2569b00000
kernel config: https://syzkaller.appspot.com/x/.config?x=6d3b8fd1977c1e73
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: i386
Unfortunately, I don't have any reproducer for this issue yet.
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
524155 pages RAM
0 pages HighMem/MovableOnly
163742 pages reserved
0 pages cma reserved
==================================================================
BUG: KASAN: vmalloc-out-of-bounds in fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline]
BUG: KASAN: vmalloc-out-of-bounds in sys_imageblit+0x12f4/0x1430 drivers/video/fbdev/core/sysimgblt.c:275
Write of size 4 at addr ffffc90004631000 by task syz-executor.0/7913
CPU: 0 PID: 7913 Comm: syz-executor.0 Not tainted 5.16.0-rc1-syzkaller #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0xf/0x320 mm/kasan/report.c:247
__kasan_report mm/kasan/report.c:433 [inline]
kasan_report.cold+0x83/0xdf mm/kasan/report.c:450
fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline]
sys_imageblit+0x12f4/0x1430 drivers/video/fbdev/core/sysimgblt.c:275
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2282
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
redraw_screen+0x61f/0x740 drivers/tty/vt/vt.c:1035
fbcon_modechanged+0x58c/0x6c0 drivers/video/fbdev/core/fbcon.c:2182
fbcon_update_vcs+0x3a/0x50 drivers/video/fbdev/core/fbcon.c:2227
do_fb_ioctl+0x62e/0x690 drivers/video/fbdev/core/fbmem.c:1114
fb_compat_ioctl+0x17e/0x610 drivers/video/fbdev/core/fbmem.c:1313
__do_compat_sys_ioctl+0x1c7/0x290 fs/ioctl.c:972
do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
__do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178
do_fast_syscall_32+0x2f/0x70 arch/x86/entry/common.c:203
entry_SYSENTER_compat_after_hwframe+0x4d/0x5c
RIP: 0023:0xf6e67549
Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
RSP: 002b:00000000f44615fc EFLAGS: 00000296 ORIG_RAX: 0000000000000036
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 0000000000004601
RDX: 0000000020000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Memory state around the buggy address:
ffffc90004630f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffc90004630f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffc90004631000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
^
ffffc90004631080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
ffffc90004631100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
==================================================================
----------------
Code disassembly (best guess):
0: 03 74 c0 01 add 0x1(%rax,%rax,8),%esi
4: 10 05 03 74 b8 01 adc %al,0x1b87403(%rip) # 0x1b8740d
a: 10 06 adc %al,(%rsi)
c: 03 74 b4 01 add 0x1(%rsp,%rsi,4),%esi
10: 10 07 adc %al,(%rdi)
12: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi
16: 10 08 adc %cl,(%rax)
18: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi
1c: 00 00 add %al,(%rax)
1e: 00 00 add %al,(%rax)
20: 00 51 52 add %dl,0x52(%rcx)
23: 55 push %rbp
24: 89 e5 mov %esp,%ebp
26: 0f 34 sysenter
28: cd 80 int $0x80
* 2a: 5d pop %rbp <-- trapping instruction
2b: 5a pop %rdx
2c: 59 pop %rcx
2d: c3 retq
2e: 90 nop
2f: 90 nop
30: 90 nop
31: 90 nop
32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
---
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 [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot has found a reproducer for the following issue on:
HEAD commit: 7fc5253f5a13 Add linux-next specific files for 20220120
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=16385270700000
kernel config: https://syzkaller.appspot.com/x/.config?x=94e8da4df9ab6319
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155dde3db00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=125298e0700000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
BUG: unable to handle page fault for address: fffff520008b2208
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 23ffed067 P4D 23ffed067 PUD 10db4067 PMD 1470c4067 PTE 0
Oops: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 3595 Comm: syz-executor362 Not tainted 5.16.0-next-20220120-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline]
RIP: 0010:sys_imageblit+0x656/0x1430 drivers/video/fbdev/core/sysimgblt.c:275
Code: 14 38 48 89 d8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 b6 0c 00 00 8b 44 24 20 23 03 8b 5c 24 18 31 c3 48 89 e8 48 c1 e8 03 <42> 0f b6 14 38 48 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
RSP: 0018:ffffc90002a1f368 EFLAGS: 00010a02
RAX: 1ffff920008b2208 RBX: 0000000000000000 RCX: 0000000000000007
RDX: 0000000000000000 RSI: ffffffff84257bf0 RDI: 0000000000000003
RBP: ffffc90004591040 R08: 000000000000001f R09: ffffffff84257a74
R10: ffffffff84257be1 R11: 0000000000000020 R12: 0000000000000007
R13: 00000000000003ef R14: ffff888146efc7e0 R15: dffffc0000000000
FS: 0000555555c5d300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: fffff520008b2208 CR3: 0000000023b12000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
highlight drivers/tty/vt/selection.c:57 [inline]
clear_selection drivers/tty/vt/selection.c:84 [inline]
clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
fbcon_do_set_font+0x47a/0x760 drivers/video/fbdev/core/fbcon.c:1928
fbcon_set_font+0x817/0xa00 drivers/video/fbdev/core/fbcon.c:2014
con_font_set drivers/tty/vt/vt.c:4666 [inline]
con_font_op+0x73a/0xc90 drivers/tty/vt/vt.c:4710
vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline]
vt_ioctl+0x1e26/0x2b10 drivers/tty/vt/vt_ioctl.c:752
tty_ioctl+0xbbd/0x1660 drivers/tty/tty_io.c:2778
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:874 [inline]
__se_sys_ioctl fs/ioctl.c:860 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f3bac0e1349
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffff160a718 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f3bac0e1349
RDX: 0000000020000000 RSI: 0000000000004b72 RDI: 0000000000000004
RBP: 00007f3bac0a51d0 R08: 000000000000000d R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f3bac0a5260
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in:
CR2: fffff520008b2208
---[ end trace 0000000000000000 ]---
RIP: 0010:fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline]
RIP: 0010:sys_imageblit+0x656/0x1430 drivers/video/fbdev/core/sysimgblt.c:275
Code: 14 38 48 89 d8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 b6 0c 00 00 8b 44 24 20 23 03 8b 5c 24 18 31 c3 48 89 e8 48 c1 e8 03 <42> 0f b6 14 38 48 89 e8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85
RSP: 0018:ffffc90002a1f368 EFLAGS: 00010a02
RAX: 1ffff920008b2208 RBX: 0000000000000000 RCX: 0000000000000007
RDX: 0000000000000000 RSI: ffffffff84257bf0 RDI: 0000000000000003
RBP: ffffc90004591040 R08: 000000000000001f R09: ffffffff84257a74
R10: ffffffff84257be1 R11: 0000000000000020 R12: 0000000000000007
R13: 00000000000003ef R14: ffff888146efc7e0 R15: dffffc0000000000
FS: 0000555555c5d300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: fffff520008b2208 CR3: 0000000023b12000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
0: 14 38 adc $0x38,%al
2: 48 89 d8 mov %rbx,%rax
5: 83 e0 07 and $0x7,%eax
8: 83 c0 03 add $0x3,%eax
b: 38 d0 cmp %dl,%al
d: 7c 08 jl 0x17
f: 84 d2 test %dl,%dl
11: 0f 85 b6 0c 00 00 jne 0xccd
17: 8b 44 24 20 mov 0x20(%rsp),%eax
1b: 23 03 and (%rbx),%eax
1d: 8b 5c 24 18 mov 0x18(%rsp),%ebx
21: 31 c3 xor %eax,%ebx
23: 48 89 e8 mov %rbp,%rax
26: 48 c1 e8 03 shr $0x3,%rax
* 2a: 42 0f b6 14 38 movzbl (%rax,%r15,1),%edx <-- trapping instruction
2f: 48 89 e8 mov %rbp,%rax
32: 83 e0 07 and $0x7,%eax
35: 83 c0 03 add $0x3,%eax
38: 38 d0 cmp %dl,%al
3a: 7c 08 jl 0x44
3c: 84 d2 test %dl,%dl
3e: 0f .byte 0xf
3f: 85 .byte 0x85
syzbot has bisected this issue to:
commit 0499f419b76f94ede08304aad5851144813ac55c
Author: Javier Martinez Canillas <[email protected]>
Date: Mon Jan 10 09:56:25 2022 +0000
video: vga16fb: Only probe for EGA and VGA 16 color graphic cards
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14c71e37b00000
start commit: 7fc5253f5a13 Add linux-next specific files for 20220120
git tree: linux-next
final oops: https://syzkaller.appspot.com/x/report.txt?x=16c71e37b00000
console output: https://syzkaller.appspot.com/x/log.txt?x=12c71e37b00000
kernel config: https://syzkaller.appspot.com/x/.config?x=94e8da4df9ab6319
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155dde3db00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=125298e0700000
Reported-by: [email protected]
Fixes: 0499f419b76f ("video: vga16fb: Only probe for EGA and VGA 16 color graphic cards")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
Here is a simplified reproducer for the issue:
https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
Currently the if block's condition has an unhandled case, where the
result of ret might get greater than vc->vc_scr_end, and therefore
the corresponding handler in else block never gets executed. Which
eventually causes panic in fast_imageblit.
Add this extra check in the conditions to fix this breakage.
#syz-test: https://github.com/torvalds/linux.git e0dccc3b76fb
---
drivers/video/fbdev/core/fbcon.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 1a9aa12cf886..d026f3845b60 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2591,14 +2591,13 @@ static unsigned long fbcon_getxy(struct vc_data *vc, unsigned long pos,
{
unsigned long ret;
int x, y;
+ unsigned long offset = (pos - vc->vc_origin) / 2;
+ x = offset % vc->vc_cols;
+ y = offset / vc->vc_cols;
+ ret = pos + (vc->vc_cols - x) * 2;
- if (pos >= vc->vc_origin && pos < vc->vc_scr_end) {
- unsigned long offset = (pos - vc->vc_origin) / 2;
-
- x = offset % vc->vc_cols;
- y = offset / vc->vc_cols;
- ret = pos + (vc->vc_cols - x) * 2;
- } else {
+ if (!pos >= vc->vc_origin || !pos < vc->vc_scr_end ||
+ !ret < vc->vc_scr_end) {
/* Should not happen */
x = y = 0;
ret = vc->vc_origin;
--
2.36.1
Hello,
syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KASAN: vmalloc-out-of-bounds Write in imageblit
==================================================================
BUG: KASAN: vmalloc-out-of-bounds in fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
BUG: KASAN: vmalloc-out-of-bounds in sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
Write of size 4 at addr ffffc90004301000 by task syz-executor.3/4204
CPU: 1 PID: 4204 Comm: syz-executor.3 Not tainted 5.19.0-rc8-syzkaller-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0xf/0x495 mm/kasan/report.c:313
print_report mm/kasan/report.c:429 [inline]
kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:825 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2328
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:139 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:188
fbcon_putcs+0x314/0x3e0 drivers/video/fbdev/core/fbcon.c:1285
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
redraw_screen+0x61f/0x740 drivers/tty/vt/vt.c:1035
fbcon_do_set_font+0x5eb/0x6f0 drivers/video/fbdev/core/fbcon.c:2435
fbcon_set_font+0x89d/0xab0 drivers/video/fbdev/core/fbcon.c:2522
con_font_set drivers/tty/vt/vt.c:4666 [inline]
con_font_op+0x73a/0xc90 drivers/tty/vt/vt.c:4710
vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline]
vt_ioctl+0x1efa/0x2b20 drivers/tty/vt/vt_ioctl.c:752
tty_ioctl+0xbbd/0x15e0 drivers/tty/tty_io.c:2778
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl fs/ioctl.c:856 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f57f9089109
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f57fa20f168 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f57f919bf60 RCX: 00007f57f9089109
RDX: 0000000020000040 RSI: 0000000000004b72 RDI: 0000000000000004
RBP: 00007f57fa20f1d0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007fffa57859ef R14: 00007f57fa20f300 R15: 0000000000022000
</TASK>
The buggy address belongs to the virtual mapping at
[ffffc90004001000, ffffc90004302000) created by:
drm_gem_shmem_vmap_locked drivers/gpu/drm/drm_gem_shmem_helper.c:319 [inline]
drm_gem_shmem_vmap+0x3d7/0x5a0 drivers/gpu/drm/drm_gem_shmem_helper.c:366
Memory state around the buggy address:
ffffc90004300f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffc90004300f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffc90004301000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
^
ffffc90004301080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
ffffc90004301100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
==================================================================
Tested on:
commit: e0dccc3b Linux 5.19-rc8
git tree: https://github.com/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1156281e080000
kernel config: https://syzkaller.appspot.com/x/.config?x=26034e6fe0075dad
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=1346ae16080000
#syz-test: https://github.com/torvalds/linux.git e0dccc3b76fb
---
drivers/video/fbdev/core/fbcon.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 1a9aa12cf886..d026f3845b60 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2591,14 +2591,13 @@ static unsigned long fbcon_getxy(struct vc_data *vc, unsigned long pos,
{
unsigned long ret;
int x, y;
+ unsigned long offset = (pos - vc->vc_origin) / 2;
+ x = offset % vc->vc_cols;
+ y = offset / vc->vc_cols;
+ ret = pos + (vc->vc_cols - x) * 2;
- if (pos >= vc->vc_origin && pos < vc->vc_scr_end) {
- unsigned long offset = (pos - vc->vc_origin) / 2;
-
- x = offset % vc->vc_cols;
- y = offset / vc->vc_cols;
- ret = pos + (vc->vc_cols - x) * 2;
- } else {
+ if (!pos >= vc->vc_origin || !pos < vc->vc_scr_end ||
+ !ret >= vc->vc_origin || !ret < vc->vc_scr_end) {
/* Should not happen */
x = y = 0;
ret = vc->vc_origin;
--
2.36.1
Hello,
syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KASAN: vmalloc-out-of-bounds Write in imageblit
==================================================================
BUG: KASAN: vmalloc-out-of-bounds in fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
BUG: KASAN: vmalloc-out-of-bounds in sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
Write of size 4 at addr ffffc90004411000 by task syz-executor.5/4188
CPU: 1 PID: 4188 Comm: syz-executor.5 Not tainted 5.19.0-rc8-syzkaller-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0xf/0x495 mm/kasan/report.c:313
print_report mm/kasan/report.c:429 [inline]
kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:825 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2328
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:139 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:188
fbcon_putcs+0x314/0x3e0 drivers/video/fbdev/core/fbcon.c:1285
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
redraw_screen+0x61f/0x740 drivers/tty/vt/vt.c:1035
fbcon_do_set_font+0x5eb/0x6f0 drivers/video/fbdev/core/fbcon.c:2435
fbcon_set_font+0x89d/0xab0 drivers/video/fbdev/core/fbcon.c:2522
con_font_set drivers/tty/vt/vt.c:4666 [inline]
con_font_op+0x73a/0xc90 drivers/tty/vt/vt.c:4710
vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline]
vt_ioctl+0x1efa/0x2b20 drivers/tty/vt/vt_ioctl.c:752
tty_ioctl+0xbbd/0x15e0 drivers/tty/tty_io.c:2778
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl fs/ioctl.c:856 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fb2a0689109
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fb2a1830168 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007fb2a079bf60 RCX: 00007fb2a0689109
RDX: 0000000020000040 RSI: 0000000000004b72 RDI: 0000000000000004
RBP: 00007fb2a18301d0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffca595479f R14: 00007fb2a1830300 R15: 0000000000022000
</TASK>
The buggy address belongs to the virtual mapping at
[ffffc90004111000, ffffc90004412000) created by:
drm_gem_shmem_vmap_locked drivers/gpu/drm/drm_gem_shmem_helper.c:319 [inline]
drm_gem_shmem_vmap+0x3d7/0x5a0 drivers/gpu/drm/drm_gem_shmem_helper.c:366
Memory state around the buggy address:
ffffc90004410f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffc90004410f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffc90004411000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
^
ffffc90004411080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
ffffc90004411100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
==================================================================
Tested on:
commit: e0dccc3b Linux 5.19-rc8
git tree: https://github.com/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=13963ed2080000
kernel config: https://syzkaller.appspot.com/x/.config?x=26034e6fe0075dad
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=14d7b5da080000
On 7/29/22 08:51, Khalid Masum wrote:
> Here is a simplified reproducer for the issue:
>
> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
The reproducer does this:
ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
-> sets the text selection area
ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
-> changes the font size.
It does not crash with current Linus' head (v5.19-rc8).
Kernel v5.16, which was used by this KASAN report, hasn't received backports
since months, so I tried stable kernel v5.15.58 instead, and this
kernel crashed with the reproducer.
The reproducer brings up two issues with current code:
1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
zero-values for ys and ye for the starting lines.
This is wrong, since the API seems to expect a "1" as the very first line for the selection.
This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
But this bug isn't critical itself and is not the reason for the kernel crash.
Without the checks, the ioctl handler simply wraps the coordinate values and converts them
from:
input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
which is the current maximum coordinates for the screen.
Those higher values now trigger issue #2:
After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
then sets a 8x32 console font, and replaces the former 8x16 console font.
With the bigger font the current screen selection is now outside the visible screen
and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
to unhighlight the selection (which starts to render chars outside of the screen):
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
highlight drivers/tty/vt/selection.c:57 [inline]
clear_selection drivers/tty/vt/selection.c:84 [inline]
clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
IMHO the easiest way to prevent this crash is to simply clear the
selection before the various con_font_set() console handlers are called.
Otherwise every console driver needs to add checks and verify if the current
selection still fits with the selected font, which gets tricky because some
of those drivers fiddle with the screen width&height before calling vc_do_resize().
I'll follow up to this mail with patches for both issues shortly.
Helge
The line and column numbers for the selection need to start at 1.
Add the checks to prevent invalid input.
Signed-off-by: Helge Deller <[email protected]>
Reported-by: [email protected]
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index f7755e73696e..58692a9b4097 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
return 0;
}
+ if (!v->xs || !v->ys || !v->xe || !v->ye)
+ return -EINVAL;
+
v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
When changing the console font with ioctl(KDFONTOP) the new font size
can be bigger than the previous font. A previous selection may thus now
be outside of the new screen size and thus trigger out-of-bounds
accesses to graphics memory if the selection is removed in
vc_do_resize().
Prevent such out-of-memory accesses by dropping the selection before the
various con_font_set() console handlers are called.
Signed-off-by: Helge Deller <[email protected]>
Reported-by: [email protected]
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index dfc1f4b445f3..3f09205185a4 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4662,9 +4662,11 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
console_lock();
if (vc->vc_mode != KD_TEXT)
rc = -EINVAL;
- else if (vc->vc_sw->con_font_set)
+ else if (vc->vc_sw->con_font_set) {
+ if (vc_is_sel(vc))
+ clear_selection();
rc = vc->vc_sw->con_font_set(vc, &font, op->flags);
- else
+ } else
rc = -ENOSYS;
console_unlock();
kfree(font.data);
@@ -4691,9 +4693,11 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
console_unlock();
return -EINVAL;
}
- if (vc->vc_sw->con_font_default)
+ if (vc->vc_sw->con_font_default) {
+ if (vc_is_sel(vc))
+ clear_selection();
rc = vc->vc_sw->con_font_default(vc, &font, s);
- else
+ } else
rc = -ENOSYS;
console_unlock();
if (!rc) {
On 7/30/22 23:25, Helge Deller wrote:
> On 7/29/22 08:51, Khalid Masum wrote:
>> Here is a simplified reproducer for the issue:
>>
>> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
>
> The reproducer does this:
Thanks for Looking into this. Being to this, so I have some questions.
> ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
How did you find out the selection values? From strace and man pages I
know the third argument is an address.
> -> sets the text selection area
> ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
Same here, It would be very helpful if you could tell me how.
> -> changes the font size.
>
> It does not crash with current Linus' head (v5.19-rc8).
I tested in 5.19-rc8 in Qemu x86_64 and it crashed for me.
> Kernel v5.16, which was used by this KASAN report, hasn't received backports
> since months, so I tried stable kernel v5.15.58 instead, and this
> kernel crashed with the reproducer.
>
> The reproducer brings up two issues with current code:
> 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
> zero-values for ys and ye for the starting lines.
> This is wrong, since the API seems to expect a "1" as the very first line for the selection.
> This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
>
> But this bug isn't critical itself and is not the reason for the kernel crash.
> Without the checks, the ioctl handler simply wraps the coordinate values and converts them
> from:
> input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
> vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
> which is the current maximum coordinates for the screen.
>
> Those higher values now trigger issue #2:
> After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
> then sets a 8x32 console font, and replaces the former 8x16 console font.
> With the bigger font the current screen selection is now outside the visible screen
> and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
> to unhighlight the selection (which starts to render chars outside of the screen):
That makes sense.
> drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
> drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
> bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
> bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
> fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
> do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
> invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
> highlight drivers/tty/vt/selection.c:57 [inline]
> clear_selection drivers/tty/vt/selection.c:84 [inline]
> clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
> vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
>
> IMHO the easiest way to prevent this crash is to simply clear the
> selection before the various con_font_set() console handlers are called.
> Otherwise every console driver needs to add checks and verify if the current
> selection still fits with the selected font, which gets tricky because some
> of those drivers fiddle with the screen width&height before calling vc_do_resize().
>
> I'll follow up to this mail with patches for both issues shortly.
I tested the patches. The crash no longer occurs with the reproducer.
> Helge
Thanks,
-- Khalid Masum
On 7/31/22 16:54, Helge Deller wrote:
> * Khalid Masum <[email protected]>:
>> On 7/30/22 23:25, Helge Deller wrote:
>>> On 7/29/22 08:51, Khalid Masum wrote:
>>>> Here is a simplified reproducer for the issue:
>>>>
>>>> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
>>>
>>> The reproducer does this:
>>
>> Thanks for Looking into this. Being to this, so I have some questions.
>>> ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
>>
>> How did you find out the selection values? From strace and man pages I know
>> the third argument is an address.
>
> Right. It's a pointer into userspace.
> I simply added printk debug code to see what's happening.
> I've attached that patch below.
>
>
>>> -> sets the text selection area
>>> ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
>>
>> Same here, It would be very helpful if you could tell me how.
>
> See patch below.
>
>>> -> changes the font size.
>>>
>>> It does not crash with current Linus' head (v5.19-rc8).
>>
>> I tested in 5.19-rc8 in Qemu x86_64 and it crashed for me.
>
> That's strange, since I tested the same. Maybe I did something wrong.
> Anyway, the patches I sent applies to all kernel versions.
>
>>> Kernel v5.16, which was used by this KASAN report, hasn't received backports
>>> since months, so I tried stable kernel v5.15.58 instead, and this
>>> kernel crashed with the reproducer.
>>>
>>> The reproducer brings up two issues with current code:
>>> 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
>>> zero-values for ys and ye for the starting lines.
>>> This is wrong, since the API seems to expect a "1" as the very first line for the selection.
>>> This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
>>>
>>> But this bug isn't critical itself and is not the reason for the kernel crash.
>>> Without the checks, the ioctl handler simply wraps the coordinate values and converts them
>>> from:
>>> input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
>>> vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
>>> which is the current maximum coordinates for the screen.
>>>
>>> Those higher values now trigger issue #2:
>>> After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
>>> then sets a 8x32 console font, and replaces the former 8x16 console font.
>>> With the bigger font the current screen selection is now outside the visible screen
>>> and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
>>> to unhighlight the selection (which starts to render chars outside of the screen):
>>
>> That makes sense.
>>
>>> drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
>>> drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
>>> bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
>>> bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
>>> fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
>>> do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
>>> invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
>>> highlight drivers/tty/vt/selection.c:57 [inline]
>>> clear_selection drivers/tty/vt/selection.c:84 [inline]
>>> clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
>>> vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
>>>
>>> IMHO the easiest way to prevent this crash is to simply clear the
>>> selection before the various con_font_set() console handlers are called.
>>> Otherwise every console driver needs to add checks and verify if the current
>>> selection still fits with the selected font, which gets tricky because some
>>> of those drivers fiddle with the screen width&height before calling vc_do_resize().
>>>
>>> I'll follow up to this mail with patches for both issues shortly.
>>
>> I tested the patches. The crash no longer occurs with the reproducer.
>
> Thanks for testing!
> Maybe you want to reply to the patches with a Tested-by: tag?
Sure, I will do that.
>
> Below is my debug code.
Thanks for the debug code! It explains a lot.
> Helge
>
>
> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> index 58692a9b4097..0167b368a70f 100644
> --- a/drivers/tty/vt/selection.c
> +++ b/drivers/tty/vt/selection.c
> @@ -333,6 +333,7 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
> v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
> + printk("vc_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
>
> if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
> mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
> @@ -357,6 +358,7 @@ int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
> {
> int ret;
>
> + printk("tiocl_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
> mutex_lock(&vc_sel.lock);
> console_lock();
> ret = vc_selection(vc_cons[fg_console].d, v, tty);
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 3f09205185a4..a0b4570c959a 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3194,6 +3194,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
> return -EFAULT;
> ret = 0;
>
> + printk("tioclinux: type = %d\n", type); // TIOCL_SETSEL
> +
> switch (type)
> {
> case TIOCL_SETSEL:
> @@ -4655,6 +4657,8 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
> if (IS_ERR(font.data))
> return PTR_ERR(font.data);
>
> + pr_err("con_font_set charcount %d w:%d h:%d\n", op->charcount, op->width, op->height);
> +
> font.charcount = op->charcount;
> font.width = op->width;
> font.height = op->height;
> @@ -4709,6 +4713,7 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
>
> int con_font_op(struct vc_data *vc, struct console_font_op *op)
> {
> + pr_warn("con_font_op op = %d\n", op->op);
> switch (op->op) {
> case KD_FONT_OP_SET:
> return con_font_set(vc, op);
Thanks,
-- Khalid Masum
* Khalid Masum <[email protected]>:
> On 7/30/22 23:25, Helge Deller wrote:
> > On 7/29/22 08:51, Khalid Masum wrote:
> > > Here is a simplified reproducer for the issue:
> > >
> > > https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
> >
> > The reproducer does this:
>
> Thanks for Looking into this. Being to this, so I have some questions.
> > ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
>
> How did you find out the selection values? From strace and man pages I know
> the third argument is an address.
Right. It's a pointer into userspace.
I simply added printk debug code to see what's happening.
I've attached that patch below.
> > -> sets the text selection area
> > ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
>
> Same here, It would be very helpful if you could tell me how.
See patch below.
> > -> changes the font size.
> >
> > It does not crash with current Linus' head (v5.19-rc8).
>
> I tested in 5.19-rc8 in Qemu x86_64 and it crashed for me.
That's strange, since I tested the same. Maybe I did something wrong.
Anyway, the patches I sent applies to all kernel versions.
> > Kernel v5.16, which was used by this KASAN report, hasn't received backports
> > since months, so I tried stable kernel v5.15.58 instead, and this
> > kernel crashed with the reproducer.
> >
> > The reproducer brings up two issues with current code:
> > 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
> > zero-values for ys and ye for the starting lines.
> > This is wrong, since the API seems to expect a "1" as the very first line for the selection.
> > This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
> >
> > But this bug isn't critical itself and is not the reason for the kernel crash.
> > Without the checks, the ioctl handler simply wraps the coordinate values and converts them
> > from:
> > input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
> > vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
> > which is the current maximum coordinates for the screen.
> >
> > Those higher values now trigger issue #2:
> > After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
> > then sets a 8x32 console font, and replaces the former 8x16 console font.
> > With the bigger font the current screen selection is now outside the visible screen
> > and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
> > to unhighlight the selection (which starts to render chars outside of the screen):
>
> That makes sense.
>
> > drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
> > drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
> > bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
> > bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
> > fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
> > do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
> > invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
> > highlight drivers/tty/vt/selection.c:57 [inline]
> > clear_selection drivers/tty/vt/selection.c:84 [inline]
> > clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
> > vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
> >
> > IMHO the easiest way to prevent this crash is to simply clear the
> > selection before the various con_font_set() console handlers are called.
> > Otherwise every console driver needs to add checks and verify if the current
> > selection still fits with the selected font, which gets tricky because some
> > of those drivers fiddle with the screen width&height before calling vc_do_resize().
> >
> > I'll follow up to this mail with patches for both issues shortly.
>
> I tested the patches. The crash no longer occurs with the reproducer.
Thanks for testing!
Maybe you want to reply to the patches with a Tested-by: tag?
Below is my debug code.
Helge
diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 58692a9b4097..0167b368a70f 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -333,6 +333,7 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
+ printk("vc_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
if (mouse_reporting() && (v->sel_mode & TIOCL_SELMOUSEREPORT)) {
mouse_report(tty, v->sel_mode & TIOCL_SELBUTTONMASK, v->xs,
@@ -357,6 +358,7 @@ int set_selection_kernel(struct tiocl_selection *v, struct tty_struct *tty)
{
int ret;
+ printk("tiocl_selection = xs:%u ys:%u xe:%u ye:%u mode:%u\n", v->xs, v->ys, v->xe, v->ye, v->sel_mode);
mutex_lock(&vc_sel.lock);
console_lock();
ret = vc_selection(vc_cons[fg_console].d, v, tty);
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 3f09205185a4..a0b4570c959a 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3194,6 +3194,8 @@ int tioclinux(struct tty_struct *tty, unsigned long arg)
return -EFAULT;
ret = 0;
+ printk("tioclinux: type = %d\n", type); // TIOCL_SETSEL
+
switch (type)
{
case TIOCL_SETSEL:
@@ -4655,6 +4657,8 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
if (IS_ERR(font.data))
return PTR_ERR(font.data);
+ pr_err("con_font_set charcount %d w:%d h:%d\n", op->charcount, op->width, op->height);
+
font.charcount = op->charcount;
font.width = op->width;
font.height = op->height;
@@ -4709,6 +4713,7 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
int con_font_op(struct vc_data *vc, struct console_font_op *op)
{
+ pr_warn("con_font_op op = %d\n", op->op);
switch (op->op) {
case KD_FONT_OP_SET:
return con_font_set(vc, op);
On 7/31/22 00:50, Helge Deller wrote:
> When changing the console font with ioctl(KDFONTOP) the new font size
> can be bigger than the previous font. A previous selection may thus now
> be outside of the new screen size and thus trigger out-of-bounds
> accesses to graphics memory if the selection is removed in
> vc_do_resize().
>
> Prevent such out-of-memory accesses by dropping the selection before the
> various con_font_set() console handlers are called.
>
> Signed-off-by: Helge Deller <[email protected]>
Tested-by: Khalid Masum <[email protected]>
> Reported-by: [email protected]
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index dfc1f4b445f3..3f09205185a4 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4662,9 +4662,11 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
> console_lock();
> if (vc->vc_mode != KD_TEXT)
> rc = -EINVAL;
> - else if (vc->vc_sw->con_font_set)
> + else if (vc->vc_sw->con_font_set) {
> + if (vc_is_sel(vc))
> + clear_selection();
> rc = vc->vc_sw->con_font_set(vc, &font, op->flags);
> - else
> + } else
> rc = -ENOSYS;
> console_unlock();
> kfree(font.data);
> @@ -4691,9 +4693,11 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op)
> console_unlock();
> return -EINVAL;
> }
> - if (vc->vc_sw->con_font_default)
> + if (vc->vc_sw->con_font_default) {
> + if (vc_is_sel(vc))
> + clear_selection();
> rc = vc->vc_sw->con_font_default(vc, &font, s);
> - else
> + } else
> rc = -ENOSYS;
> console_unlock();
> if (!rc) {
On 7/30/22 23:25, Helge Deller wrote:
> On 7/29/22 08:51, Khalid Masum wrote:
>> Here is a simplified reproducer for the issue:
>>
>> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
>
> The reproducer does this:
> ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
> -> sets the text selection area
> ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
> -> changes the font size.
>
> It does not crash with current Linus' head (v5.19-rc8).
> Kernel v5.16, which was used by this KASAN report, hasn't received backports
> since months, so I tried stable kernel v5.15.58 instead, and this
> kernel crashed with the reproducer.
>
> The reproducer brings up two issues with current code:
> 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
> zero-values for ys and ye for the starting lines.
> This is wrong, since the API seems to expect a "1" as the very first line for the selection.
Why do you think that API expects a 1?
> This can be easily fixed by adding checks for zero-values and return -EINVAL if found.
>
> But this bug isn't critical itself and is not the reason for the kernel crash.
> Without the checks, the ioctl handler simply wraps the coordinate values and converts them
> from:
> input selection: xs:3 ys:0 xe:0 ye:0 mode:0 to the new:
> vc_selection = xs:2 ys:23 xe:127 ye:23 mode:0
> which is the current maximum coordinates for the screen.
>
> Those higher values now trigger issue #2:
> After the TIOCL_SETSEL the last line on the screen is now selected. The KDFONTOP ioctl
> then sets a 8x32 console font, and replaces the former 8x16 console font.
> With the bigger font the current screen selection is now outside the visible screen
> and this finally triggeres this backtrace, because vc_do_resize() calls clear_selection()
> to unhighlight the selection (which starts to render chars outside of the screen):
>
> drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline]
> drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2288
> bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline]
> bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173
> fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277
> do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
> invert_screen+0x1d4/0x600 drivers/tty/vt/vt.c:800
> highlight drivers/tty/vt/selection.c:57 [inline]
> clear_selection drivers/tty/vt/selection.c:84 [inline]
> clear_selection+0x55/0x70 drivers/tty/vt/selection.c:80
> vc_do_resize+0xe6e/0x1180 drivers/tty/vt/vt.c:1257
>
> IMHO the easiest way to prevent this crash is to simply clear the
> selection before the various con_font_set() console handlers are called.
> Otherwise every console driver needs to add checks and verify if the current
> selection still fits with the selected font, which gets tricky because some
> of those drivers fiddle with the screen width&height before calling vc_do_resize().
>
> I'll follow up to this mail with patches for both issues shortly.
>
> Helge
Thanks,
-- Khalid Masum
On 7/31/22 15:55, Khalid Masum wrote:
> On 7/30/22 23:25, Helge Deller wrote:
>> On 7/29/22 08:51, Khalid Masum wrote:
>>> Here is a simplified reproducer for the issue:
>>>
>>> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
>>
>> The reproducer does this:
>> ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
>> -> sets the text selection area
>> ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
>> -> changes the font size.
>>
>> It does not crash with current Linus' head (v5.19-rc8).
>> Kernel v5.16, which was used by this KASAN report, hasn't received backports
>> since months, so I tried stable kernel v5.15.58 instead, and this
>> kernel crashed with the reproducer.
>>
>> The reproducer brings up two issues with current code:
>> 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
>> zero-values for ys and ye for the starting lines.
>> This is wrong, since the API seems to expect a "1" as the very first line for the selection.
>
> Why do you think that API expects a 1?
because the code in drivers/tty/vt/selection.c indicates this:
See:
static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, struct tty_struct *tty)
{
int ps, pe;
...
v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
...
ps = v->ys * vc->vc_size_row + (v->xs << 1);
pe = v->ye * vc->vc_size_row + (v->xe << 1);
The userspace-provided xs,ys,xe and ye values are decremented by one
before ps and pe (the pointers into the text screen array) are calculated.
So, for the xs...ye values in the range from 1..max-screen-lines/columns
are expected.
Helge
On 7/31/22 21:39, Helge Deller wrote:
> On 7/31/22 15:55, Khalid Masum wrote:
>> On 7/30/22 23:25, Helge Deller wrote:
>>> On 7/29/22 08:51, Khalid Masum wrote:
>>>> Here is a simplified reproducer for the issue:
>>>>
>>>> https://gist.githubusercontent.com/Labnann/923d6b9b3a19848fc129637b839b8a55/raw/a68271fcc724569735fe27f80817e561b3ff629a/reproducer.c
>>>
>>> The reproducer does this:
>>> ioctl(3, TIOCLINUX, TIOCL_SETSEL, selection: xs:3 ys:0 xe:0 ye:0 mode:0) = 0
>>> -> sets the text selection area
>>> ioctl(4, KDFONTOP) with op=0 (con_font_set), charcount=512 width=8 height=32, 0x20000000) = 0
>>> -> changes the font size.
>>>
>>> It does not crash with current Linus' head (v5.19-rc8).
>>> Kernel v5.16, which was used by this KASAN report, hasn't received backports
>>> since months, so I tried stable kernel v5.15.58 instead, and this
>>> kernel crashed with the reproducer.
>>>
>>> The reproducer brings up two issues with current code:
>>> 1. The reproducer uses ioctl(TIOCLINUX, TIOCL_SETSEL) and hands over (invalid)
>>> zero-values for ys and ye for the starting lines.
>>> This is wrong, since the API seems to expect a "1" as the very first line for the selection.
>>
>> Why do you think that API expects a 1?
>
> because the code in drivers/tty/vt/selection.c indicates this:
> See:
> static int vc_selection(struct vc_data *vc, struct tiocl_selection *v, struct tty_struct *tty)
> {
> int ps, pe;
> ...
> v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
> v->ye = min_t(u16, v->ye - 1, vc->vc_rows - 1);
> ...
> ps = v->ys * vc->vc_size_row + (v->xs << 1);
> pe = v->ye * vc->vc_size_row + (v->xe << 1);
>
> The userspace-provided xs,ys,xe and ye values are decremented by one
> before ps and pe (the pointers into the text screen array) are calculated.
> So, for the xs...ye values in the range from 1..max-screen-lines/columns
> are expected.
Oh. I got it. It is pretty simple. Thanks again for explaining.
>
> Helge
Khalid Masum
On Sat, Jul 30, 2022 at 02:12:46PM +0600, Khalid Masum wrote:
> Currently the if block's condition has an unhandled case, where the
> result of ret might get greater than vc->vc_scr_end, and therefore
> the corresponding handler in else block never gets executed. Which
> eventually causes panic in fast_imageblit.
>
> Add this extra check in the conditions to fix this breakage.
>
> #syz-test: https://github.com/torvalds/linux.git e0dccc3b76fb
>
> ---
> drivers/video/fbdev/core/fbcon.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 1a9aa12cf886..d026f3845b60 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2591,14 +2591,13 @@ static unsigned long fbcon_getxy(struct vc_data *vc, unsigned long pos,
> {
> unsigned long ret;
> int x, y;
> + unsigned long offset = (pos - vc->vc_origin) / 2;
> + x = offset % vc->vc_cols;
> + y = offset / vc->vc_cols;
> + ret = pos + (vc->vc_cols - x) * 2;
>
> - if (pos >= vc->vc_origin && pos < vc->vc_scr_end) {
> - unsigned long offset = (pos - vc->vc_origin) / 2;
> -
> - x = offset % vc->vc_cols;
> - y = offset / vc->vc_cols;
> - ret = pos + (vc->vc_cols - x) * 2;
> - } else {
> + if (!pos >= vc->vc_origin || !pos < vc->vc_scr_end ||
> + !ret < vc->vc_scr_end) {
These are precendence bugs. The ! will be done before the >=. Write it
as:
if (pos < vc->vc_origin || pos >= vc->vc_scr_end ||
ret >= vc->vc_scr_end) {
> /* Should not happen */
> x = y = 0;
> ret = vc->vc_origin;
regards,
dan carpenter
On 8/1/22 16:43, Dan Carpenter wrote:
>
> These are precendence bugs. The ! will be done before the >=. Write it
> as:
>
> if (pos < vc->vc_origin || pos >= vc->vc_scr_end ||
> ret >= vc->vc_scr_end) {
>
>
>> /* Should not happen */
>> x = y = 0;
>> ret = vc->vc_origin;
>
> regards,
> dan carpenter
>
Thanks for the catch. I shall send another syz-test patch with these fixed.
thanks,
-- Khalid Masum
#syz test: https://github.com/torvalds/linux.git 3d7cb6b04c3f
---
drivers/video/fbdev/core/fbcon.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 1a9aa12cf886..d026f3845b60 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2591,14 +2591,13 @@ static unsigned long fbcon_getxy(struct vc_data *vc, unsigned long pos,
{
unsigned long ret;
int x, y;
+ unsigned long offset = (pos - vc->vc_origin) / 2;
+ x = offset % vc->vc_cols;
+ y = offset / vc->vc_cols;
+ ret = pos + (vc->vc_cols - x) * 2;
- if (pos >= vc->vc_origin && pos < vc->vc_scr_end) {
- unsigned long offset = (pos - vc->vc_origin) / 2;
-
- x = offset % vc->vc_cols;
- y = offset / vc->vc_cols;
- ret = pos + (vc->vc_cols - x) * 2;
- } else {
+ if (pos < vc->vc_origin || pos >= vc->vc_scr_end ||
+ ret >= vc->vc_scr_end) {
/* Should not happen */
x = y = 0;
ret = vc->vc_origin;
--
2.36.1
Hello,
syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KASAN: vmalloc-out-of-bounds Write in imageblit
==================================================================
BUG: KASAN: vmalloc-out-of-bounds in fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
BUG: KASAN: vmalloc-out-of-bounds in sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
Write of size 4 at addr ffffc90004261000 by task syz-executor.4/4191
CPU: 0 PID: 4191 Comm: syz-executor.4 Not tainted 5.19.0-syzkaller-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/22/2022
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
print_address_description.constprop.0.cold+0xf/0x495 mm/kasan/report.c:313
print_report mm/kasan/report.c:429 [inline]
kasan_report.cold+0xf4/0x1c6 mm/kasan/report.c:491
fast_imageblit drivers/video/fbdev/core/sysimgblt.c:257 [inline]
sys_imageblit+0x1ed0/0x2240 drivers/video/fbdev/core/sysimgblt.c:323
drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:825 [inline]
drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2328
bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:139 [inline]
bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:188
fbcon_putcs+0x314/0x3e0 drivers/video/fbdev/core/fbcon.c:1285
do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676
redraw_screen+0x61f/0x740 drivers/tty/vt/vt.c:1035
fbcon_do_set_font+0x5eb/0x6f0 drivers/video/fbdev/core/fbcon.c:2435
fbcon_set_font+0x89d/0xab0 drivers/video/fbdev/core/fbcon.c:2522
con_font_set drivers/tty/vt/vt.c:4666 [inline]
con_font_op+0x73a/0xc90 drivers/tty/vt/vt.c:4710
vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline]
vt_ioctl+0x1efa/0x2b20 drivers/tty/vt/vt_ioctl.c:752
tty_ioctl+0xbbd/0x15e0 drivers/tty/tty_io.c:2778
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl fs/ioctl.c:856 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7ff9cac89109
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ff9cbd6c168 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007ff9cad9c030 RCX: 00007ff9cac89109
RDX: 0000000020000040 RSI: 0000000000004b72 RDI: 0000000000000004
RBP: 00007ff9cbd6c1d0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffd34c3bf8f R14: 00007ff9cbd6c300 R15: 0000000000022000
</TASK>
The buggy address belongs to the virtual mapping at
[ffffc90003f61000, ffffc90004262000) created by:
drm_gem_shmem_vmap_locked drivers/gpu/drm/drm_gem_shmem_helper.c:319 [inline]
drm_gem_shmem_vmap+0x3d7/0x5a0 drivers/gpu/drm/drm_gem_shmem_helper.c:366
Memory state around the buggy address:
ffffc90004260f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffc90004260f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffc90004261000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
^
ffffc90004261080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
ffffc90004261100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
==================================================================
Tested on:
commit: 3d7cb6b0 Linux 5.19
git tree: https://github.com/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=11b85261080000
kernel config: https://syzkaller.appspot.com/x/.config?x=70dd99d568a89e0
dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=13f0a73c080000
On 30. 07. 22, 20:49, Helge Deller wrote:
> The line and column numbers for the selection need to start at 1.
> Add the checks to prevent invalid input.
>
> Signed-off-by: Helge Deller <[email protected]>
> Reported-by: [email protected]
>
> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> index f7755e73696e..58692a9b4097 100644
> --- a/drivers/tty/vt/selection.c
> +++ b/drivers/tty/vt/selection.c
> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
> return 0;
> }
>
> + if (!v->xs || !v->ys || !v->xe || !v->ye)
> + return -EINVAL;
Hmm, I'm not sure about this. It potentially breaks userspace (by
returning EINVAL now). And the code below should handle this just fine,
right:
> +
> v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
?
thanks,
--
js
suse labs
Hello Jiri,
Thanks for looking into this patch!
On 8/4/22 07:47, Jiri Slaby wrote:
> On 30. 07. 22, 20:49, Helge Deller wrote:
>> The line and column numbers for the selection need to start at 1.
>> Add the checks to prevent invalid input.
>>
>> Signed-off-by: Helge Deller <[email protected]>
>> Reported-by: [email protected]
>>
>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>> index f7755e73696e..58692a9b4097 100644
>> --- a/drivers/tty/vt/selection.c
>> +++ b/drivers/tty/vt/selection.c
>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>> return 0;
>> }
>>
>> + if (!v->xs || !v->ys || !v->xe || !v->ye)
>> + return -EINVAL;
>
> Hmm, I'm not sure about this. It potentially breaks userspace (by
> returning EINVAL now).
Right.
According to the code below, my interpretation is that all xs/ys/xe/ye values
should be > 0. But of course I might be wrong on this, as I didn't find any
documentation for TIOCL_SETSEL.
And if userspace tries to set an invalid selection (e.g. by selecting row 0),
my patch now returns -EINVAL, while it returned success before.
> And the code below should handle this just fine, right:
>> v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
It "handles it fine" in the sense that it can cope with the
input and will not crash.
But it returns (maybe?) unexpected results...
For example, if a user selects row 0 (where I assume he wanted to set
the first line), he instead selects the last row.
I'm not sure if this is the expected behaviour.
Do you know of any userspace program which breaks because of this?
Helge
On 8/4/22 09:15, Helge Deller wrote:
> Hello Jiri,
>
> Thanks for looking into this patch!
>
> On 8/4/22 07:47, Jiri Slaby wrote:
>> On 30. 07. 22, 20:49, Helge Deller wrote:
>>> The line and column numbers for the selection need to start at 1.
>>> Add the checks to prevent invalid input.
>>>
>>> Signed-off-by: Helge Deller <[email protected]>
>>> Reported-by: [email protected]
>>>
>>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>>> index f7755e73696e..58692a9b4097 100644
>>> --- a/drivers/tty/vt/selection.c
>>> +++ b/drivers/tty/vt/selection.c
>>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>>> return 0;
>>> }
>>>
>>> + if (!v->xs || !v->ys || !v->xe || !v->ye)
>>> + return -EINVAL;
>>
>> Hmm, I'm not sure about this. It potentially breaks userspace (by
>> returning EINVAL now).
>
> Right.
> According to the code below, my interpretation is that all xs/ys/xe/ye values
> should be > 0. But of course I might be wrong on this, as I didn't find any
> documentation for TIOCL_SETSEL.
>
> And if userspace tries to set an invalid selection (e.g. by selecting row 0),
> my patch now returns -EINVAL, while it returned success before.
>
>> And the code below should handle this just fine, right:
>>> v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
>
> It "handles it fine" in the sense that it can cope with the
> input and will not crash.
> But it returns (maybe?) unexpected results...
After some more thinking maybe you are right.
In case a user provided invalid values in the past, simply an unexpected
selection was set, but nothing broke.
Since the patch doesn't fix any critical issue, we could just drop this patch
and leave it as is.
Helge
On 04. 08. 22, 10:44, Helge Deller wrote:
> On 8/4/22 09:15, Helge Deller wrote:
>> Hello Jiri,
>>
>> Thanks for looking into this patch!
>>
>> On 8/4/22 07:47, Jiri Slaby wrote:
>>> On 30. 07. 22, 20:49, Helge Deller wrote:
>>>> The line and column numbers for the selection need to start at 1.
>>>> Add the checks to prevent invalid input.
>>>>
>>>> Signed-off-by: Helge Deller <[email protected]>
>>>> Reported-by: [email protected]
>>>>
>>>> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
>>>> index f7755e73696e..58692a9b4097 100644
>>>> --- a/drivers/tty/vt/selection.c
>>>> +++ b/drivers/tty/vt/selection.c
>>>> @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>>>> return 0;
>>>> }
>>>>
>>>> + if (!v->xs || !v->ys || !v->xe || !v->ye)
>>>> + return -EINVAL;
>>>
>>> Hmm, I'm not sure about this. It potentially breaks userspace (by
>>> returning EINVAL now).
>>
>> Right.
>> According to the code below, my interpretation is that all xs/ys/xe/ye values
>> should be > 0. But of course I might be wrong on this, as I didn't find any
>> documentation for TIOCL_SETSEL.
>>
>> And if userspace tries to set an invalid selection (e.g. by selecting row 0),
>> my patch now returns -EINVAL, while it returned success before.
>>
>>> And the code below should handle this just fine, right:
>>>> v->xs = min_t(u16, v->xs - 1, vc->vc_cols - 1);
>>>> v->ys = min_t(u16, v->ys - 1, vc->vc_rows - 1);
>>>> v->xe = min_t(u16, v->xe - 1, vc->vc_cols - 1);
>>
>> It "handles it fine" in the sense that it can cope with the
>> input and will not crash.
>> But it returns (maybe?) unexpected results...
>
> After some more thinking maybe you are right.
> In case a user provided invalid values in the past, simply an unexpected
> selection was set, but nothing broke.
> Since the patch doesn't fix any critical issue, we could just drop this patch
> and leave it as is.
We can still do a trial and revert it if something breaks... It's just
that _noone_ knows with all this undocumented stuff ;).
But in fact, 0 currently means full row/column. Isn't it on purpose?
Today, we are out of luck, codesearch.debian.net gives no clue about users:
https://codesearch.debian.net/search?q=%5CbTIOCL_SETSEL%5Cb&literal=0
thanks,
--
js
suse labs
On Thu, Aug 04, 2022 at 11:22:26AM +0200, Jiri Slaby wrote:
> On 04. 08. 22, 10:44, Helge Deller wrote:
> > On 8/4/22 09:15, Helge Deller wrote:
> > > On 8/4/22 07:47, Jiri Slaby wrote:
> > > > On 30. 07. 22, 20:49, Helge Deller wrote:
> > > > > The line and column numbers for the selection need to start at 1.
> > > > > Add the checks to prevent invalid input.
> > > > > --- a/drivers/tty/vt/selection.c
> > > > > +++ b/drivers/tty/vt/selection.c
> > > > > @@ -326,6 +326,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
> > > > > + if (!v->xs || !v->ys || !v->xe || !v->ye)
> > > > > + return -EINVAL;
> > > >
> > > > Hmm, I'm not sure about this. It potentially breaks userspace (by
> > > > returning EINVAL now).
> We can still do a trial and revert it if something breaks... It's just that
> _noone_ knows with all this undocumented stuff ;).
>
> But in fact, 0 currently means full row/column. Isn't it on purpose?
>
> Today, we are out of luck, codesearch.debian.net gives no clue about users:
> https://codesearch.debian.net/search?q=%5CbTIOCL_SETSEL%5Cb&literal=0
That's because the macro is undocumented.
"man ioctl_console" says:
TIOCLINUX, subcode=2
Set selection. argp points to a [...]
thus everyone writes it as a number.
You'd need to grep for TIOCLINUX; there's not that many references to
check...
Meow!
--
⢀⣴⠾⠻⢶⣦⠀
⣾⠁⢠⠒⠀⣿⡁ Say what you want about Adolf, at least he was the man who
⢿⡄⠘⠷⠚⠋⠀ killed Hitler. Your turn, Vlad!
⠈⠳⣄⠀⠀⠀⠀