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

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 2eb29d59ddf0 Merge tag 'drm-next-2023-03-03-1' of git://an..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=12e88ebcc80000
kernel config: https://syzkaller.appspot.com/x/.config?x=cab35c936731a347
dashboard link: https://syzkaller.appspot.com/bug?extid=3af17071816b61e807ed
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=10b71504c80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16f02d9cc80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/ea23ee201337/disk-2eb29d59.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/40a9db02dede/vmlinux-2eb29d59.xz
kernel image: https://storage.googleapis.com/syzbot-assets/8d4b574642d0/bzImage-2eb29d59.xz

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

usercopy: Kernel memory exposure attempt detected from page alloc (offset 0, size 4194560)!
------------[ cut here ]------------
kernel BUG at mm/usercopy.c:102!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 5073 Comm: syz-executor309 Not tainted 6.2.0-syzkaller-13277-g2eb29d59ddf0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/16/2023
RIP: 0010:usercopy_abort+0xb7/0xd0 mm/usercopy.c:102
Code: e8 fe e8 a1 ff 49 89 d9 4c 89 e1 48 89 ee 41 56 48 c7 c7 00 73 5b 8a 41 55 41 57 4c 8b 44 24 20 48 8b 54 24 18 e8 59 8b 85 ff <0f> 0b 48 c7 c3 00 71 5b 8a 49 89 df 49 89 d8 e9 71 ff ff ff 0f 1f
RSP: 0018:ffffc90003bcf9e0 EFLAGS: 00010286
RAX: 000000000000005b RBX: ffffffff8a5b7100 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff816931fc RDI: 0000000000000005
RBP: ffffffff8a5b72c0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: ffffffff8a5b7500
R13: 0000000000000000 R14: 0000000000400100 R15: ffffffff8a5b7100
FS: 0000555555b1b300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 0000000075655000 CR4: 0000000000350ee0
Call Trace:
<TASK>
check_heap_object mm/usercopy.c:200 [inline]
__check_object_size mm/usercopy.c:251 [inline]
__check_object_size+0x50a/0x6e0 mm/usercopy.c:213
check_object_size include/linux/thread_info.h:215 [inline]
check_copy_size include/linux/thread_info.h:251 [inline]
copy_to_user include/linux/uaccess.h:168 [inline]
con_font_get drivers/tty/vt/vt.c:4580 [inline]
con_font_op+0x397/0xf10 drivers/tty/vt/vt.c:4674
vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline]
vt_ioctl+0x620/0x2df0 drivers/tty/vt/vt_ioctl.c:752
tty_ioctl+0x773/0x16e0 drivers/tty/tty_io.c:2777
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+0x197/0x210 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f6672fd82d9
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:00007ffc66955e38 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f6672fd82d9
RDX: 0000000020000000 RSI: 0000000000004b72 RDI: 0000000000000003
RBP: 00007f6672f9c0c0 R08: 000000000000000d R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f6672f9c150
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:usercopy_abort+0xb7/0xd0 mm/usercopy.c:102
Code: e8 fe e8 a1 ff 49 89 d9 4c 89 e1 48 89 ee 41 56 48 c7 c7 00 73 5b 8a 41 55 41 57 4c 8b 44 24 20 48 8b 54 24 18 e8 59 8b 85 ff <0f> 0b 48 c7 c3 00 71 5b 8a 49 89 df 49 89 d8 e9 71 ff ff ff 0f 1f
RSP: 0018:ffffc90003bcf9e0 EFLAGS: 00010286
RAX: 000000000000005b RBX: ffffffff8a5b7100 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff816931fc RDI: 0000000000000005
RBP: ffffffff8a5b72c0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: ffffffff8a5b7500
R13: 0000000000000000 R14: 0000000000400100 R15: ffffffff8a5b7100
FS: 0000555555b1b300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020000000 CR3: 0000000075655000 CR4: 0000000000350ee0


---
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 syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[syzbot]

syzbot has bisected this issue to:

commit 24d69384bcd34b9dcaf5dab744bf7096e84d1abd
Author: Samuel Thibault <samuel....@ens-lyon.org>
Date: Thu Jan 19 15:19:16 2023 +0000

VT: Add KD_FONT_OP_SET/GET_TALL operations

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=120b3232c80000
start commit: 2eb29d59ddf0 Merge tag 'drm-next-2023-03-03-1' of git://an..
git tree: upstream
final oops: https://syzkaller.appspot.com/x/report.txt?x=110b3232c80000
console output: https://syzkaller.appspot.com/x/log.txt?x=160b3232c80000

kernel config: https://syzkaller.appspot.com/x/.config?x=cab35c936731a347
dashboard link: https://syzkaller.appspot.com/bug?extid=3af17071816b61e807ed

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10b71504c80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16f02d9cc80000

Reported-by: syzbot+3af170...@syzkaller.appspotmail.com
Fixes: 24d69384bcd3 ("VT: Add KD_FONT_OP_SET/GET_TALL operations")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

[Kees Cook]

On Fri, Mar 03, 2023 at 01:37:55PM -0800, syzbot wrote:
> dashboard link: https://syzkaller.appspot.com/bug?extid=3af17071816b61e807ed
> [...]

> usercopy: Kernel memory exposure attempt detected from page alloc (offset 0, size 4194560)!

> [...]

> Call Trace:
> <TASK>
> check_heap_object mm/usercopy.c:200 [inline]
> __check_object_size mm/usercopy.c:251 [inline]
> __check_object_size+0x50a/0x6e0 mm/usercopy.c:213
> check_object_size include/linux/thread_info.h:215 [inline]
> check_copy_size include/linux/thread_info.h:251 [inline]
> copy_to_user include/linux/uaccess.h:168 [inline]
> con_font_get drivers/tty/vt/vt.c:4580 [inline]
> con_font_op+0x397/0xf10 drivers/tty/vt/vt.c:4674

This is coming from the folio checking:

} else if (folio_test_large(folio)) {
offset = ptr - folio_address(folio);
if (n > folio_size(folio) - offset)
usercopy_abort("page alloc", NULL, to_user, offset, n);
}

triggered by copy_to_user of the font.data allocation:

#define max_font_width 64
#define max_font_height 128
#define max_font_glyphs 512
#define max_font_size (max_font_glyphs*max_font_width*max_font_height)
...
font.data = kvmalloc(max_font_size, GFP_KERNEL);
...
if (op->data && copy_to_user(op->data, font.data, c))
rc = -EFAULT;

it is correctly seeing "c" (4194560 in the report) as larger than
"max_font_size" (4194304, seen reported by "folio_size(folio)"). The
"c" calculation comes from:

unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
...
rc = vc->vc_sw->con_font_get(vc, &font, vpitch);
...
c = (font.width+7)/8 * vpitch * font.charcount;

So yes, 4194560 is larger than 4194304, and a memory exposure was,
in fact, blocked here.

Given the recent work in this area, I'm not sure which calculation is
wrong, max_font_size or c. Samuel?

-Kees

--
Kees Cook

[Jiri Slaby]

On 05. 03. 23, 18:54, Samuel Thibault wrote:
> Kees Cook, le ven. 03 mars 2023 14:07:04 -0800, a ecrit:

>> #define max_font_width 64
>> #define max_font_height 128
>> #define max_font_glyphs 512
>> #define max_font_size (max_font_glyphs*max_font_width*max_font_height)
>> ...
>> font.data = kvmalloc(max_font_size, GFP_KERNEL);
>> ...
>> if (op->data && copy_to_user(op->data, font.data, c))
>> rc = -EFAULT;
>>
>> it is correctly seeing "c" (4194560 in the report) as larger than
>> "max_font_size" (4194304, seen reported by "folio_size(folio)"). The
>> "c" calculation comes from:
>>
>> unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
>> ...
>> rc = vc->vc_sw->con_font_get(vc, &font, vpitch);
>> ...
>> c = (font.width+7)/8 * vpitch * font.charcount;
>>
>> So yes, 4194560 is larger than 4194304, and a memory exposure was,
>> in fact, blocked here.
>>
>> Given the recent work in this area, I'm not sure which calculation is
>> wrong, max_font_size or c. Samuel?
>

> They are not wrong. It's the vpitch value (coming from userland's
> op.height) which is out of bound and missing a check.
>
> The patch below should be fixing it, could you check?
>
> I don't know how I am supposed to properly reference the syzbot report
> etc., could somebody used to the process handle submitting the fix?

It's as simple as adding:
Reported-by: syzbot+3af170...@syzkaller.appspotmail.com
to the tags.

> VT: Protect KD_FONT_OP_GET_TALL from unbound access
>
> In ioctl(KD_FONT_OP_GET_TALL), userland tells through op->height which
> vpitch should be used to copy over the font. In con_font_get, we were
> not checking that it is within the maximum height value, and thus
> userland could make the vc->vc_sw->con_font_get(vc, &font, vpitch);
> call possibly overflow the allocated max_font_size bytes, and the
> copy_to_user(op->data, font.data, c) call possibly read out of that
> allocated buffer.
>
> By checking vpitch against max_font_height, the max_font_size buffer
> will always be large enough for the vc->vc_sw->con_font_get(vc, &font,
> vpitch) call (since we already prevent loading a font larger than that),
> and c = (font.width+7)/8 * vpitch * font.charcount will always remain
> below max_font_size.

>
> Fixes: 24d69384bcd3 ("VT: Add KD_FONT_OP_SET/GET_TALL operations")

Reviewed-by: Jiri Slaby <jiri...@kernel.org>

> Signed-off-by: Samuel Thibault <samuel....@ens-lyon.org>
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 57a5c23b51d4..3c2ea9c098f7 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -4545,6 +4545,9 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
> int c;

> unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
>

> + if (vpitch > max_font_height)
> + return -EINVAL;
> +
> if (op->data) {
> font.data = kvmalloc(max_font_size, GFP_KERNEL);
> if (!font.data)

--
js

[Samuel Thibault]

Kees Cook, le ven. 03 mars 2023 14:07:04 -0800, a ecrit:

> #define max_font_width 64
> #define max_font_height 128
> #define max_font_glyphs 512
> #define max_font_size (max_font_glyphs*max_font_width*max_font_height)
> ...
> font.data = kvmalloc(max_font_size, GFP_KERNEL);
> ...
> if (op->data && copy_to_user(op->data, font.data, c))
> rc = -EFAULT;
>
> it is correctly seeing "c" (4194560 in the report) as larger than
> "max_font_size" (4194304, seen reported by "folio_size(folio)"). The
> "c" calculation comes from:
>
> unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
> ...
> rc = vc->vc_sw->con_font_get(vc, &font, vpitch);
> ...
> c = (font.width+7)/8 * vpitch * font.charcount;
>
> So yes, 4194560 is larger than 4194304, and a memory exposure was,
> in fact, blocked here.
>
> Given the recent work in this area, I'm not sure which calculation is
> wrong, max_font_size or c. Samuel?

They are not wrong. It's the vpitch value (coming from userland's
op.height) which is out of bound and missing a check.

The patch below should be fixing it, could you check?

I don't know how I am supposed to properly reference the syzbot report
etc., could somebody used to the process handle submitting the fix?

Samuel

VT: Protect KD_FONT_OP_GET_TALL from unbound access

In ioctl(KD_FONT_OP_GET_TALL), userland tells through op->height which
vpitch should be used to copy over the font. In con_font_get, we were
not checking that it is within the maximum height value, and thus
userland could make the vc->vc_sw->con_font_get(vc, &font, vpitch);
call possibly overflow the allocated max_font_size bytes, and the
copy_to_user(op->data, font.data, c) call possibly read out of that
allocated buffer.

By checking vpitch against max_font_height, the max_font_size buffer
will always be large enough for the vc->vc_sw->con_font_get(vc, &font,
vpitch) call (since we already prevent loading a font larger than that),
and c = (font.width+7)/8 * vpitch * font.charcount will always remain
below max_font_size.

Fixes: 24d69384bcd3 ("VT: Add KD_FONT_OP_SET/GET_TALL operations")

Signed-off-by: Samuel Thibault <samuel....@ens-lyon.org>

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 57a5c23b51d4..3c2ea9c098f7 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4545,6 +4545,9 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
int c;

unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;

[Aleksandr Nogikh]

Hi Samuel,

On Mon, Mar 6, 2023 at 8:36 AM Samuel Thibault
<samuel....@ens-lyon.org> wrote:
>
> The patch below should be fixing it, could you check?
>
> I don't know how I am supposed to properly reference the syzbot report
> etc., could somebody used to the process handle submitting the fix?

As Jiri Slaby correctly said above, you just need to add the
`Reported-by` tag from the syzbot bug report to your patch so that the
bot can recognize the fix later.

If you just want syzbot to check whether the reproducer still triggers
the bug after your changes, you can send an email with the `syz test`
command and the raw diff patch. Here are the instructions:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
and here are many examples:
https://groups.google.com/g/syzkaller-bugs/search?q=%22%23syz%20test%22

--
Aleksandr

[Samuel Thibault]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 24d69384bcd34b9dcaf5dab744bf7096e84d1abd

[syzbot]

Hello,

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

drivers/tty/vt/vt.c:4545:15: error: 'max_font_height' undeclared (first use in this function); did you mean 'max_font_size'?


Tested on:

commit: 24d69384 VT: Add KD_FONT_OP_SET/GET_TALL operations
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

dashboard link: https://syzkaller.appspot.com/bug?extid=3af17071816b61e807ed
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=11e87024c80000

[Samuel Thibault]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 05e2600cb0a4d73b0779cf29512819616252aeeb

[syzbot]

Hello,

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

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

Tested on:

commit: 05e2600c VT: Bump font size limitation to 64x128 pixels
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=17b6b7acc80000
kernel config: https://syzkaller.appspot.com/x/.config?x=cffc15efb411dffa

dashboard link: https://syzkaller.appspot.com/bug?extid=3af17071816b61e807ed
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=10227482c80000

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

[Samuel Thibault]

Aleksandr Nogikh, le lun. 06 mars 2023 11:28:04 +0100, a ecrit:

Thanks! The patch does fix the reproducer case.

Samuel