[Linux-kernel-mentees] [PATCH syzbot] fbcon: Fix global-out-of-bounds read in fbcon_get_font()

10 views
Skip to first unread message

Peilin Ye

unread,
Sep 16, 2020, 12:47:41ā€ÆPM9/16/20
to syzbot+29d4ed...@syzkaller.appspotmail.com, syzkall...@googlegroups.com
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

The naughty reproducer first sets `vc_font.height` to a large number in
vt_ioctl(), then it calls fbcon_get_font(), which overflows the global
data buffer of our built-in 8x16 font.

In the framebuffer layer we deal with two kinds of fonts: built-in fonts,
as well as user-provided fonts. fbcon_resize() is doing a pretty good job
preventing overflowing user-provided font data buffers, by putting a few
metadata fields before the "real data":

(drivers/video/fbdev/core/fbcon.h)
/* Font */
#define REFCOUNT(fd) (((int *)(fd))[-1])
#define FNTSIZE(fd) (((int *)(fd))[-2])
#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FNTSUM(fd) (((int *)(fd))[-4])
#define FONT_EXTRA_WORDS 4

See how fbcon_resize() is using them:

size = CALC_FONTSZ(vc->vc_font.height, pitch, FNTCHARCNT(vc->vc_font.data));
if (size > FNTSIZE(vc->vc_font.data))
return -EINVAL;

The problem is, we can't use `FNTSIZE` for built-in fonts, since their
data buffers are hard-coded into the kernel image, and using `FNTSIZE`
will cause a global underflow.

On the other hand, we don't keep track of the buffer length in `struct
console_font` at all:

struct console_font {
unsigned int width, height; /* font size */
unsigned int charcount;
unsigned char *data; /* font data with height fixed to 32 */
};

At 2020/08/07 06:27 I sent my first patch to syzbot, adding a `maxlen`
field into `struct console_font`. Unfortunately `struct console_font` is
part of the UAPI, so that was a no-no. :(

However today I was so drunk that I came up with this idea: why not we
prepend some metadata fields to these built-in font data buffers, so that
the framebuffer layer can use `FNTSIZE` on them, too?

This patch is that proof-of-concept. If syzbot is happy, I will do the
same thing for the rest of built-in fonts, clean up the code change, etc.

v1_syz.patch

syzbot

unread,
Sep 16, 2020, 7:09:09ā€ÆPM9/16/20
to syzkall...@googlegroups.com, yepei...@gmail.com
Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING: ODEBUG bug in netdev_run_todo

bond0 (unregistering): (slave bond_slave_1): Releasing backup interface
bond0 (unregistering): (slave bond_slave_0): Releasing backup interface
bond0 (unregistering): Released all slaves
------------[ cut here ]------------
ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x90 arch/x86/include/asm/paravirt.h:775
WARNING: CPU: 1 PID: 28 at lib/debugobjects.c:485 debug_print_object+0x160/0x250 lib/debugobjects.c:485
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 28 Comm: kworker/u4:2 Not tainted 5.9.0-rc5-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: netns cleanup_net
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x198/0x1fd lib/dump_stack.c:118
panic+0x347/0x7c0 kernel/panic.c:231
__warn.cold+0x20/0x46 kernel/panic.c:600
report_bug+0x1bd/0x210 lib/bug.c:198
handle_bug+0x38/0x90 arch/x86/kernel/traps.c:234
exc_invalid_op+0x14/0x40 arch/x86/kernel/traps.c:254
asm_exc_invalid_op+0x12/0x20 arch/x86/include/asm/idtentry.h:536
RIP: 0010:debug_print_object+0x160/0x250 lib/debugobjects.c:485
Code: dd 80 1e 94 88 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 bf 00 00 00 48 8b 14 dd 80 1e 94 88 48 c7 c7 e0 13 94 88 e8 92 f9 a6 fd <0f> 0b 83 05 c3 d5 14 07 01 48 83 c4 20 5b 5d 41 5c 41 5d c3 48 89
RSP: 0018:ffffc90000e378a0 EFLAGS: 00010082
RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
RDX: ffff8880a90e2140 RSI: ffffffff815dba07 RDI: fffff520001c6f06
RBP: 0000000000000001 R08: 0000000000000001 R09: ffff8880ae720f8b
R10: 0000000000000000 R11: 0000000000099a18 R12: ffffffff89be2fa0
R13: ffffffff81639300 R14: dead000000000100 R15: dffffc0000000000
__debug_check_no_obj_freed lib/debugobjects.c:967 [inline]
debug_check_no_obj_freed+0x301/0x41c lib/debugobjects.c:998
kfree+0xfb/0x2b0 mm/slab.c:3755
kvfree+0x42/0x50 mm/util.c:603
device_release+0x71/0x200 drivers/base/core.c:1796
kobject_cleanup lib/kobject.c:705 [inline]
kobject_release lib/kobject.c:736 [inline]
kref_put include/linux/kref.h:65 [inline]
kobject_put+0x171/0x270 lib/kobject.c:753
netdev_run_todo+0x765/0xac0 net/core/dev.c:10113
default_device_exit_batch+0x316/0x3d0 net/core/dev.c:10913
ops_exit_list+0x10d/0x160 net/core/net_namespace.c:189
cleanup_net+0x4ea/0xa00 net/core/net_namespace.c:603
process_one_work+0x94c/0x1670 kernel/workqueue.c:2269
worker_thread+0x64c/0x1120 kernel/workqueue.c:2415
kthread+0x3b5/0x4a0 kernel/kthread.c:292
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Kernel Offset: disabled
Rebooting in 86400 seconds..


Tested on:

commit: 5925fa68 Merge tag 'perf-tools-fixes-for-v5.9-2020-09-16' ..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=169e5b11900000
kernel config: https://syzkaller.appspot.com/x/.config?x=83e0d9e1135feadb
dashboard link: https://syzkaller.appspot.com/bug?extid=29d4ed7f3bdedf2aa2fd
compiler: gcc (GCC) 10.1.0-syz 20200507
patch: https://syzkaller.appspot.com/x/patch.diff?x=12be3b55900000

Peilin Ye

unread,
Sep 16, 2020, 11:52:12ā€ÆPM9/16/20
to syzbot+29d4ed...@syzkaller.appspotmail.com, syzkall...@googlegroups.com
Let's do the same thing for all built-in fonts and see how it goes.
syz_v2.patch

syzbot

unread,
Sep 17, 2020, 12:10:05ā€ÆAM9/17/20
to syzkall...@googlegroups.com, yepei...@gmail.com
Hello,

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

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

Tested on:

commit: 5925fa68 Merge tag 'perf-tools-fixes-for-v5.9-2020-09-16' ..
git tree: upstream
patch: https://syzkaller.appspot.com/x/patch.diff?x=16182db5900000

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

Peilin Ye

unread,
Sep 23, 2020, 6:11:21ā€ÆAM9/23/20
to syzbot+29d4ed...@syzkaller.appspotmail.com, syzkall...@googlegroups.com
fbcon_get_font() is reading out-of-bounds. A malicious user may resize
`vc_font.height` to a large value, causing fbcon_get_font() to overflow
our built-in font data buffers, declared in lib/fonts/font_*.c:

(e.g. lib/fonts/font_8x8.c)
#define FONTDATAMAX 2048

static const unsigned char fontdata_8x8[FONTDATAMAX] = {

/* 0 0x00 '^@' */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
0x00, /* 00000000 */
[...]

In order to perform a reliable range check, fbcon_get_font() needs to know
`FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
do not keep that information in our font descriptor,
`struct console_font`:

(include/uapi/linux/kd.h)
struct console_font {
unsigned int width, height; /* font size */
unsigned int charcount;
unsigned char *data; /* font data with height fixed to 32 */
};

To make things worse, `struct console_font` is part of the UAPI, so we
cannot add a new field to it.

Fortunately, the framebuffer layer itself gives us a hint of how to
resolve this issue without changing UAPI. When allocating a buffer for a
user-provided font, fbcon_set_font() reserves four "extra words" at the
beginning of the buffer:

(drivers/video/fbdev/core/fbcon.c)
new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
[...]
new_data += FONT_EXTRA_WORDS * sizeof(int);
FNTSIZE(new_data) = size;
FNTCHARCNT(new_data) = charcount;
REFCOUNT(new_data) = 0; /* usage counter */
[...]
FNTSUM(new_data) = csum;

Later, to get the size of a data buffer, the framebuffer layer simply
calls FNTSIZE() on it:

(drivers/video/fbdev/core/fbcon.h)
/* Font */
#define REFCOUNT(fd) (((int *)(fd))[-1])
#define FNTSIZE(fd) (((int *)(fd))[-2])
#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FNTSUM(fd) (((int *)(fd))[-4])
#define FONT_EXTRA_WORDS 4

However, this is only for user-provided fonts. Let us do the same thing
for built-in fonts, prepend these "extra words" (including `FONTDATAMAX`)
to their data buffers, so that other subsystems, like the framebuffer
layer, can use FNTSIZE() on them.

Similarly, console/newport_con is using three extra words instead of four:

(drivers/video/console/newport_con.c)
/* borrowed from fbcon.c */
#define REFCOUNT(fd) (((int *)(fd))[-1])
#define FNTSIZE(fd) (((int *)(fd))[-2])
#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FONT_EXTRA_WORDS 3

To keep things simple, move all these macro definitions to <linux/font.h>,
and make both the framebuffer layer and console/newport_con depend on it.

Define `FONTDATAMAX` for font_6x10.c and font_acorn_8x8.c.

Finally, since console/newport_con now uses four "extra words", initialize
the fourth one when allocating new buffers in newport_set_font().

syz_v3.patch

syzbot

unread,
Sep 23, 2020, 7:07:07ā€ÆAM9/23/20
to syzkall...@googlegroups.com, yepei...@gmail.com
Hello,

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

drivers/video/fbdev/core/fbcon_rotate.c:35:36: error: implicit declaration of function 'FNTCHARCNT' [-Werror=implicit-function-declaration]


Tested on:

commit: 805c6d3c Merge branch 'fixes' of git://git.kernel.org/pub/..
git tree: upstream
dashboard link: https://syzkaller.appspot.com/bug?extid=29d4ed7f3bdedf2aa2fd
compiler: gcc (GCC) 10.1.0-syz 20200507
patch: https://syzkaller.appspot.com/x/patch.diff?x=111d4b1d900000

Peilin Ye

unread,
Sep 23, 2020, 8:12:52ā€ÆAM9/23/20
to syzbot+29d4ed...@syzkaller.appspotmail.com, syzkall...@googlegroups.com
---
Change in v4:
- Include <linux/font.h> for drivers/video/fbdev/core/fbcon_rotate.c

syzbot

unread,
Sep 23, 2020, 10:08:11ā€ÆAM9/23/20
to syzkall...@googlegroups.com, yepei...@gmail.com
Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KASAN: global-out-of-bounds Read in fbcon_get_font

==================================================================
BUG: KASAN: global-out-of-bounds in memcpy include/linux/string.h:406 [inline]
BUG: KASAN: global-out-of-bounds in fbcon_get_font+0x2f4/0x6f0 drivers/video/fbdev/core/fbcon.c:2303
Read of size 31 at addr ffffffff8896ac7c by task syz-executor.3/8236

CPU: 0 PID: 8236 Comm: syz-executor.3 Not tainted 5.9.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x198/0x1fd lib/dump_stack.c:118
print_address_description.constprop.0.cold+0x5/0x497 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
check_memory_region_inline mm/kasan/generic.c:186 [inline]
check_memory_region+0x13d/0x180 mm/kasan/generic.c:192
memcpy+0x20/0x60 mm/kasan/common.c:105
memcpy include/linux/string.h:406 [inline]
fbcon_get_font+0x2f4/0x6f0 drivers/video/fbdev/core/fbcon.c:2303
con_font_get drivers/tty/vt/vt.c:4571 [inline]
con_font_op+0x1f5/0x1110 drivers/tty/vt/vt.c:4730
do_fontx_ioctl drivers/tty/vt/vt_ioctl.c:514 [inline]
vt_io_ioctl drivers/tty/vt/vt_ioctl.c:611 [inline]
vt_ioctl+0x1b6d/0x2cc0 drivers/tty/vt/vt_ioctl.c:856
tty_ioctl+0x1019/0x15f0 drivers/tty/tty_io.c:2656
vfs_ioctl fs/ioctl.c:48 [inline]
__do_sys_ioctl fs/ioctl.c:753 [inline]
__se_sys_ioctl fs/ioctl.c:739 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45c8a9
Code: 8d b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 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 0f 83 5b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f8e78c94c78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000004e5860 RCX: 000000000045c8a9
RDX: 0000000020000000 RSI: 0000000000004b6b RDI: 0000000000000003
RBP: 000000000078bf00 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 0000000000000330 R14: 00000000004c59d5 R15: 00007f8e78c956d4

The buggy address belongs to the variable:
fontdata_8x16+0xffc/0x1120

Memory state around the buggy address:
ffffffff8896ab80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff8896ac00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffffff8896ac80: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 05 f9 f9 f9
^
ffffffff8896ad00: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 00 00 03 f9
ffffffff8896ad80: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================


Tested on:

commit: 805c6d3c Merge branch 'fixes' of git://git.kernel.org/pub/..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16df70d3900000
kernel config: https://syzkaller.appspot.com/x/.config?x=d1ab92e9b47d3c7f

Peilin Ye

unread,
Sep 23, 2020, 10:42:44ā€ÆAM9/23/20
to syzbot+29d4ed...@syzkaller.appspotmail.com, syzkall...@googlegroups.com
syz_v4.patch

syzbot

unread,
Sep 23, 2020, 2:12:05ā€ÆPM9/23/20
to syzkall...@googlegroups.com, yepei...@gmail.com
Hello,

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

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

Tested on:

commit: 805c6d3c Merge branch 'fixes' of git://git.kernel.org/pub/..
git tree: upstream
patch: https://syzkaller.appspot.com/x/patch.diff?x=125df08d900000
Reply all
Reply to author
Forward
0 new messages