Hello,
syzbot found the following crash on:
HEAD commit: 6cf8298d Add linux-next specific files for 20191209 git tree: linux-next console output: https://syzkaller.appspot.com/x/log.txt?x=168bbb7ae00000 kernel config: https://syzkaller.appspot.com/x/.config?x=682fc0ce6c86e3c7 dashboard link: https://syzkaller.appspot.com/bug?extid=29d4ed7f3bdedf2aa2fd compiler: gcc (GCC) 9.0.0 20181231 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12fff061e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+29d4ed7f3bdedf2aa2fd@syzkaller.appspotmail.com
================================================================== BUG: KASAN: global-out-of-bounds in memcpy include/linux/string.h:425 [inline] BUG: KASAN: global-out-of-bounds in fbcon_get_font+0x2b2/0x5e0 drivers/video/fbdev/core/fbcon.c:2465 Read of size 32 at addr ffffffff88729d80 by task syz-executor.3/9100
CPU: 1 PID: 9100 Comm: syz-executor.3 Not tainted 5.5.0-rc1-next-20191209-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+0x197/0x210 lib/dump_stack.c:118 print_address_description.constprop.0.cold+0x5/0x30b mm/kasan/report.c:374 __kasan_report.cold+0x1b/0x41 mm/kasan/report.c:506 kasan_report+0x12/0x20 mm/kasan/common.c:639 check_memory_region_inline mm/kasan/generic.c:185 [inline] check_memory_region+0x134/0x1a0 mm/kasan/generic.c:192 memcpy+0x24/0x50 mm/kasan/common.c:125 memcpy include/linux/string.h:425 [inline] fbcon_get_font+0x2b2/0x5e0 drivers/video/fbdev/core/fbcon.c:2465 con_font_get drivers/tty/vt/vt.c:4446 [inline] con_font_op+0x20b/0x1270 drivers/tty/vt/vt.c:4605 vt_ioctl+0xd2e/0x26d0 drivers/tty/vt/vt_ioctl.c:913 tty_ioctl+0xa37/0x14f0 drivers/tty/tty_io.c:2660 vfs_ioctl fs/ioctl.c:47 [inline] file_ioctl fs/ioctl.c:545 [inline] do_vfs_ioctl+0x977/0x14e0 fs/ioctl.c:732 ksys_ioctl+0xab/0xd0 fs/ioctl.c:749 __do_sys_ioctl fs/ioctl.c:756 [inline] __se_sys_ioctl fs/ioctl.c:754 [inline] __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x45a6f9 Code: ad 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 7b b6 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f30314dcc78 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 000000000045a6f9 RDX: 0000000000713000 RSI: 0000000000004b60 RDI: 0000000000000004 RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00007f30314dd6d4 R13: 00000000004c6d87 R14: 00000000004dd3e0 R15: 00000000ffffffff
The buggy address belongs to the variable: fontdata_8x16+0x1000/0x1120
Memory state around the buggy address: ffffffff88729c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff88729d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff88729d80: fa fa fa fa 06 fa fa fa fa fa fa fa 05 fa fa fa
^ ffffffff88729e00: fa fa fa fa 06 fa fa fa fa fa fa fa 00 00 03 fa ffffffff88729e80: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00 ==================================================================
--- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches
syzbot has found a reproducer for the following crash on:
HEAD commit: 738d2902 Merge git://git.kernel.org/pub/scm/linux/kernel/g.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=14e396c1e00000 kernel config: https://syzkaller.appspot.com/x/.config?x=874bac2ff63646fa dashboard link: https://syzkaller.appspot.com/bug?extid=29d4ed7f3bdedf2aa2fd compiler: gcc (GCC) 9.0.0 20181231 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17a0b866e00000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15fc963ee00000
Bisection is inconclusive: the bug happens on the oldest tested release.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15b75532e00000 final crash: https://syzkaller.appspot.com/x/report.txt?x=17b75532e00000 console output: https://syzkaller.appspot.com/x/log.txt?x=13b75532e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+29d4ed7f3bdedf2aa2fd@syzkaller.appspotmail.com
================================================================== BUG: KASAN: global-out-of-bounds in memcpy include/linux/string.h:380 [inline] BUG: KASAN: global-out-of-bounds in fbcon_get_font+0x2b2/0x5e0 drivers/video/fbdev/core/fbcon.c:2465 Read of size 32 at addr ffffffff88729e80 by task syz-executor334/10190
CPU: 0 PID: 10190 Comm: syz-executor334 Not tainted 5.5.0-rc4-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+0x197/0x210 lib/dump_stack.c:118 print_address_description.constprop.0.cold+0x5/0x30b mm/kasan/report.c:374 __kasan_report.cold+0x1b/0x41 mm/kasan/report.c:506 kasan_report+0x12/0x20 mm/kasan/common.c:639 check_memory_region_inline mm/kasan/generic.c:185 [inline] check_memory_region+0x134/0x1a0 mm/kasan/generic.c:192 memcpy+0x24/0x50 mm/kasan/common.c:125 memcpy include/linux/string.h:380 [inline] fbcon_get_font+0x2b2/0x5e0 drivers/video/fbdev/core/fbcon.c:2465 con_font_get drivers/tty/vt/vt.c:4446 [inline] con_font_op+0x20b/0x1270 drivers/tty/vt/vt.c:4605 vt_ioctl+0xd2e/0x26d0 drivers/tty/vt/vt_ioctl.c:913 tty_ioctl+0xa37/0x14f0 drivers/tty/tty_io.c:2660 vfs_ioctl fs/ioctl.c:47 [inline] file_ioctl fs/ioctl.c:545 [inline] do_vfs_ioctl+0x977/0x14e0 fs/ioctl.c:732 ksys_ioctl+0xab/0xd0 fs/ioctl.c:749 __do_sys_ioctl fs/ioctl.c:756 [inline] __se_sys_ioctl fs/ioctl.c:754 [inline] __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:754 do_syscall_64+0xfa/0x790 arch/x86/entry/common.c:294 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x4412d9 Code: e8 3c ad 02 00 48 83 c4 18 c3 0f 1f 80 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 0f 83 9b 09 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffc49b97f18 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 00000000004a2487 RCX: 00000000004412d9 RDX: 0000000020000200 RSI: 0000000000004b60 RDI: 0000000000000004 RBP: 0000000000018ac7 R08: 000000000000000d R09: 00000000004002c8 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000402100 R13: 0000000000402190 R14: 0000000000000000 R15: 0000000000000000
The buggy address belongs to the variable: fontdata_8x16+0x1000/0x1120
Memory state around the buggy address: ffffffff88729d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffffffff88729e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff88729e80: fa fa fa fa 06 fa fa fa fa fa fa fa 05 fa fa fa
^ ffffffff88729f00: fa fa fa fa 06 fa fa fa fa fa fa fa 00 00 03 fa ffffffff88729f80: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00 ==================================================================
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), 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 keep track of `FONTDATAMAX`.
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
Currently, this is only done 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 these macros on all fonts, no matter built-in or user-provided. As an example, this series fixes the syzbot issue in fbcon_get_font():
(drivers/video/fbdev/core/fbcon.c) if (font->width <= 8) { j = vc->vc_font.height; + if (font->charcount * j > FNTSIZE(fontdata)) + return -EINVAL; [...]
Similarly, newport_con also use these macros. It only uses three of them:
(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>, use four words instead of three, and initialize the fourth word in newport_set_font() properly.
Many thanks to Greg Kroah-Hartman gregkh@linuxfoundation.org, who reviewed and improved this series!
[1]: KASAN: global-out-of-bounds Read in fbcon_get_font https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecf...
Peilin Ye (3): fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts fbcon: Fix global-out-of-bounds read in fbcon_get_font()
drivers/video/console/newport_con.c | 7 +------ drivers/video/fbdev/core/fbcon.c | 12 ++++++++++++ drivers/video/fbdev/core/fbcon.h | 7 ------- drivers/video/fbdev/core/fbcon_rotate.c | 1 + drivers/video/fbdev/core/tileblit.c | 1 + include/linux/font.h | 13 +++++++++++++ lib/fonts/font_10x18.c | 9 ++++----- lib/fonts/font_6x10.c | 9 +++++---- lib/fonts/font_6x11.c | 9 ++++----- lib/fonts/font_7x14.c | 9 ++++----- lib/fonts/font_8x16.c | 9 ++++----- lib/fonts/font_8x8.c | 9 ++++----- lib/fonts/font_acorn_8x8.c | 9 ++++++--- lib/fonts/font_mini_4x6.c | 8 ++++---- lib/fonts/font_pearl_8x8.c | 9 ++++----- lib/fonts/font_sun12x22.c | 9 ++++----- lib/fonts/font_sun8x16.c | 7 ++++--- lib/fonts/font_ter16x32.c | 9 ++++----- 18 files changed, 79 insertions(+), 67 deletions(-)
drivers/video/console/newport_con.c is borrowing FONT_EXTRA_WORDS macros from drivers/video/fbdev/core/fbcon.h. To keep things simple, move all definitions into <linux/font.h>.
Since newport_con now uses four extra words, initialize the fourth word in newport_set_font() properly.
Cc: stable@vger.kernel.org Signed-off-by: Peilin Ye yepeilin.cs@gmail.com --- drivers/video/console/newport_con.c | 7 +------ drivers/video/fbdev/core/fbcon.h | 7 ------- drivers/video/fbdev/core/fbcon_rotate.c | 1 + drivers/video/fbdev/core/tileblit.c | 1 + include/linux/font.h | 8 ++++++++ 5 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c index 72f146d047d9..cd51b7a17a21 100644 --- a/drivers/video/console/newport_con.c +++ b/drivers/video/console/newport_con.c @@ -35,12 +35,6 @@
#define FONT_DATA ((unsigned char *)font_vga_8x16.data)
-/* 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 - static unsigned char *font_data[MAX_NR_CONSOLES];
static struct newport_regs *npregs; @@ -522,6 +516,7 @@ static int newport_set_font(int unit, struct console_font *op) FNTSIZE(new_data) = size; FNTCHARCNT(new_data) = op->charcount; REFCOUNT(new_data) = 0; /* usage counter */ + FNTSUM(new_data) = 0;
p = new_data; for (i = 0; i < op->charcount; i++) { diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h index 20dea853765f..5ee153ba977e 100644 --- a/drivers/video/fbdev/core/fbcon.h +++ b/drivers/video/fbdev/core/fbcon.h @@ -152,13 +152,6 @@ static inline int attr_col_ec(int shift, struct vc_data *vc, #define attr_bgcol_ec(bgshift, vc, info) attr_col_ec(bgshift, vc, info, 0) #define attr_fgcol_ec(fgshift, vc, info) attr_col_ec(fgshift, vc, info, 1)
-/* 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 - /* * Scroll Method */ diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c index c0d445294aa7..ac72d4f85f7d 100644 --- a/drivers/video/fbdev/core/fbcon_rotate.c +++ b/drivers/video/fbdev/core/fbcon_rotate.c @@ -14,6 +14,7 @@ #include <linux/fb.h> #include <linux/vt_kern.h> #include <linux/console.h> +#include <linux/font.h> #include <asm/types.h> #include "fbcon.h" #include "fbcon_rotate.h" diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c index 1dfaff0881fb..257e94feeeb6 100644 --- a/drivers/video/fbdev/core/tileblit.c +++ b/drivers/video/fbdev/core/tileblit.c @@ -13,6 +13,7 @@ #include <linux/fb.h> #include <linux/vt_kern.h> #include <linux/console.h> +#include <linux/font.h> #include <asm/types.h> #include "fbcon.h"
diff --git a/include/linux/font.h b/include/linux/font.h index 51b91c8b69d5..40ed008d7dad 100644 --- a/include/linux/font.h +++ b/include/linux/font.h @@ -59,4 +59,12 @@ extern const struct font_desc *get_default_font(int xres, int yres, /* Max. length for the name of a predefined font */ #define MAX_FONT_NAME 32
+/* Extra word getters */ +#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 + #endif /* _VIDEO_FONT_H */
syzbot has reported an issue in the framebuffer layer, where a malicious user may overflow our built-in font data buffers.
In order to perform a reliable range check, subsystems need to know `FONTDATAMAX` for each built-in font. Unfortunately, our font descriptor, `struct console_font` does not contain `FONTDATAMAX`, and is part of the UAPI, making it infeasible to modify it.
For user-provided fonts, the framebuffer layer resolves this issue by reserving four extra words at the beginning of data buffers. Later, whenever a function needs to access them, it simply uses the following macros:
#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
Recently we have gathered all the above macros to <linux/font.h>. Let us do the same thing for built-in fonts, prepend four extra words (including `FONTDATAMAX`) to their data buffers, so that subsystems can use these macros for all fonts, no matter built-in or user-provided.
This patch depends on patch "fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h".
Cc: stable@vger.kernel.org Link: https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecf... Signed-off-by: Peilin Ye yepeilin.cs@gmail.com --- include/linux/font.h | 5 +++++ lib/fonts/font_10x18.c | 9 ++++----- lib/fonts/font_6x10.c | 9 +++++---- lib/fonts/font_6x11.c | 9 ++++----- lib/fonts/font_7x14.c | 9 ++++----- lib/fonts/font_8x16.c | 9 ++++----- lib/fonts/font_8x8.c | 9 ++++----- lib/fonts/font_acorn_8x8.c | 9 ++++++--- lib/fonts/font_mini_4x6.c | 8 ++++---- lib/fonts/font_pearl_8x8.c | 9 ++++----- lib/fonts/font_sun12x22.c | 9 ++++----- lib/fonts/font_sun8x16.c | 7 ++++--- lib/fonts/font_ter16x32.c | 9 ++++----- 13 files changed, 56 insertions(+), 54 deletions(-)
diff --git a/include/linux/font.h b/include/linux/font.h index 40ed008d7dad..59faa80f586d 100644 --- a/include/linux/font.h +++ b/include/linux/font.h @@ -67,4 +67,9 @@ extern const struct font_desc *get_default_font(int xres, int yres,
#define FONT_EXTRA_WORDS 4
+struct font_data { + unsigned int extra[FONT_EXTRA_WORDS]; + const unsigned char data[]; +} __packed; + #endif /* _VIDEO_FONT_H */ diff --git a/lib/fonts/font_10x18.c b/lib/fonts/font_10x18.c index 532f0ff89a96..0e2deac97da0 100644 --- a/lib/fonts/font_10x18.c +++ b/lib/fonts/font_10x18.c @@ -8,8 +8,8 @@
#define FONTDATAMAX 9216
-static const unsigned char fontdata_10x18[FONTDATAMAX] = { - +static struct font_data fontdata_10x18 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, 0x00, /* 0000000000 */ 0x00, 0x00, /* 0000000000 */ @@ -5129,8 +5129,7 @@ static const unsigned char fontdata_10x18[FONTDATAMAX] = { 0x00, 0x00, /* 0000000000 */ 0x00, 0x00, /* 0000000000 */ 0x00, 0x00, /* 0000000000 */ - -}; +} };
const struct font_desc font_10x18 = { @@ -5138,7 +5137,7 @@ const struct font_desc font_10x18 = { .name = "10x18", .width = 10, .height = 18, - .data = fontdata_10x18, + .data = fontdata_10x18.data, #ifdef __sparc__ .pref = 5, #else diff --git a/lib/fonts/font_6x10.c b/lib/fonts/font_6x10.c index 09b2cc03435b..87da8acd07db 100644 --- a/lib/fonts/font_6x10.c +++ b/lib/fonts/font_6x10.c @@ -1,8 +1,10 @@ // SPDX-License-Identifier: GPL-2.0 #include <linux/font.h>
-static const unsigned char fontdata_6x10[] = { +#define FONTDATAMAX 2560
+static struct font_data fontdata_6x10 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ @@ -3074,14 +3076,13 @@ static const unsigned char fontdata_6x10[] = { 0x00, /* 00000000 */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ - -}; +} };
const struct font_desc font_6x10 = { .idx = FONT6x10_IDX, .name = "6x10", .width = 6, .height = 10, - .data = fontdata_6x10, + .data = fontdata_6x10.data, .pref = 0, }; diff --git a/lib/fonts/font_6x11.c b/lib/fonts/font_6x11.c index d7136c33f1f0..5e975dfa10a5 100644 --- a/lib/fonts/font_6x11.c +++ b/lib/fonts/font_6x11.c @@ -9,8 +9,8 @@
#define FONTDATAMAX (11*256)
-static const unsigned char fontdata_6x11[FONTDATAMAX] = { - +static struct font_data fontdata_6x11 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ @@ -3338,8 +3338,7 @@ static const unsigned char fontdata_6x11[FONTDATAMAX] = { 0x00, /* 00000000 */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ - -}; +} };
const struct font_desc font_vga_6x11 = { @@ -3347,7 +3346,7 @@ const struct font_desc font_vga_6x11 = { .name = "ProFont6x11", .width = 6, .height = 11, - .data = fontdata_6x11, + .data = fontdata_6x11.data, /* Try avoiding this font if possible unless on MAC */ .pref = -2000, }; diff --git a/lib/fonts/font_7x14.c b/lib/fonts/font_7x14.c index 89752d0b23e8..86d298f38505 100644 --- a/lib/fonts/font_7x14.c +++ b/lib/fonts/font_7x14.c @@ -8,8 +8,8 @@
#define FONTDATAMAX 3584
-static const unsigned char fontdata_7x14[FONTDATAMAX] = { - +static struct font_data fontdata_7x14 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 0000000 */ 0x00, /* 0000000 */ @@ -4105,8 +4105,7 @@ static const unsigned char fontdata_7x14[FONTDATAMAX] = { 0x00, /* 0000000 */ 0x00, /* 0000000 */ 0x00, /* 0000000 */ - -}; +} };
const struct font_desc font_7x14 = { @@ -4114,6 +4113,6 @@ const struct font_desc font_7x14 = { .name = "7x14", .width = 7, .height = 14, - .data = fontdata_7x14, + .data = fontdata_7x14.data, .pref = 0, }; diff --git a/lib/fonts/font_8x16.c b/lib/fonts/font_8x16.c index b7ab1f5fbdb8..37cedd36ca5e 100644 --- a/lib/fonts/font_8x16.c +++ b/lib/fonts/font_8x16.c @@ -10,8 +10,8 @@
#define FONTDATAMAX 4096
-static const unsigned char fontdata_8x16[FONTDATAMAX] = { - +static struct font_data fontdata_8x16 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ @@ -4619,8 +4619,7 @@ static const unsigned char fontdata_8x16[FONTDATAMAX] = { 0x00, /* 00000000 */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ - -}; +} };
const struct font_desc font_vga_8x16 = { @@ -4628,7 +4627,7 @@ const struct font_desc font_vga_8x16 = { .name = "VGA8x16", .width = 8, .height = 16, - .data = fontdata_8x16, + .data = fontdata_8x16.data, .pref = 0, }; EXPORT_SYMBOL(font_vga_8x16); diff --git a/lib/fonts/font_8x8.c b/lib/fonts/font_8x8.c index 2328ebc8bab5..8ab695538395 100644 --- a/lib/fonts/font_8x8.c +++ b/lib/fonts/font_8x8.c @@ -9,8 +9,8 @@
#define FONTDATAMAX 2048
-static const unsigned char fontdata_8x8[FONTDATAMAX] = { - +static struct font_data fontdata_8x8 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ @@ -2570,8 +2570,7 @@ static const unsigned char fontdata_8x8[FONTDATAMAX] = { 0x00, /* 00000000 */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ - -}; +} };
const struct font_desc font_vga_8x8 = { @@ -2579,6 +2578,6 @@ const struct font_desc font_vga_8x8 = { .name = "VGA8x8", .width = 8, .height = 8, - .data = fontdata_8x8, + .data = fontdata_8x8.data, .pref = 0, }; diff --git a/lib/fonts/font_acorn_8x8.c b/lib/fonts/font_acorn_8x8.c index 0ff0e85d4481..069b3e80c434 100644 --- a/lib/fonts/font_acorn_8x8.c +++ b/lib/fonts/font_acorn_8x8.c @@ -3,7 +3,10 @@
#include <linux/font.h>
-static const unsigned char acorndata_8x8[] = { +#define FONTDATAMAX 2048 + +static struct font_data acorndata_8x8 = { +{ 0, 0, FONTDATAMAX, 0 }, { /* 00 */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* ^@ */ /* 01 */ 0x7e, 0x81, 0xa5, 0x81, 0xbd, 0x99, 0x81, 0x7e, /* ^A */ /* 02 */ 0x7e, 0xff, 0xbd, 0xff, 0xc3, 0xe7, 0xff, 0x7e, /* ^B */ @@ -260,14 +263,14 @@ static const unsigned char acorndata_8x8[] = { /* FD */ 0x38, 0x04, 0x18, 0x20, 0x3c, 0x00, 0x00, 0x00, /* FE */ 0x00, 0x00, 0x3c, 0x3c, 0x3c, 0x3c, 0x00, 0x00, /* FF */ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 -}; +} };
const struct font_desc font_acorn_8x8 = { .idx = ACORN8x8_IDX, .name = "Acorn8x8", .width = 8, .height = 8, - .data = acorndata_8x8, + .data = acorndata_8x8.data, #ifdef CONFIG_ARCH_ACORN .pref = 20, #else diff --git a/lib/fonts/font_mini_4x6.c b/lib/fonts/font_mini_4x6.c index 838caa1cfef7..1449876c6a27 100644 --- a/lib/fonts/font_mini_4x6.c +++ b/lib/fonts/font_mini_4x6.c @@ -43,8 +43,8 @@ __END__;
#define FONTDATAMAX 1536
-static const unsigned char fontdata_mini_4x6[FONTDATAMAX] = { - +static struct font_data fontdata_mini_4x6 = { + { 0, 0, FONTDATAMAX, 0 }, { /*{*/ /* Char 0: ' ' */ 0xee, /*= [*** ] */ @@ -2145,14 +2145,14 @@ static const unsigned char fontdata_mini_4x6[FONTDATAMAX] = { 0xee, /*= [*** ] */ 0x00, /*= [ ] */ /*}*/ -}; +} };
const struct font_desc font_mini_4x6 = { .idx = MINI4x6_IDX, .name = "MINI4x6", .width = 4, .height = 6, - .data = fontdata_mini_4x6, + .data = fontdata_mini_4x6.data, .pref = 3, };
diff --git a/lib/fonts/font_pearl_8x8.c b/lib/fonts/font_pearl_8x8.c index b15d3c342c5b..32d65551e7ed 100644 --- a/lib/fonts/font_pearl_8x8.c +++ b/lib/fonts/font_pearl_8x8.c @@ -14,8 +14,8 @@
#define FONTDATAMAX 2048
-static const unsigned char fontdata_pearl8x8[FONTDATAMAX] = { - +static struct font_data fontdata_pearl8x8 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ @@ -2575,14 +2575,13 @@ static const unsigned char fontdata_pearl8x8[FONTDATAMAX] = { 0x00, /* 00000000 */ 0x00, /* 00000000 */ 0x00, /* 00000000 */ - -}; +} };
const struct font_desc font_pearl_8x8 = { .idx = PEARL8x8_IDX, .name = "PEARL8x8", .width = 8, .height = 8, - .data = fontdata_pearl8x8, + .data = fontdata_pearl8x8.data, .pref = 2, }; diff --git a/lib/fonts/font_sun12x22.c b/lib/fonts/font_sun12x22.c index 955d6eee3959..641a6b4dca42 100644 --- a/lib/fonts/font_sun12x22.c +++ b/lib/fonts/font_sun12x22.c @@ -3,8 +3,8 @@
#define FONTDATAMAX 11264
-static const unsigned char fontdata_sun12x22[FONTDATAMAX] = { - +static struct font_data fontdata_sun12x22 = { + { 0, 0, FONTDATAMAX, 0 }, { /* 0 0x00 '^@' */ 0x00, 0x00, /* 000000000000 */ 0x00, 0x00, /* 000000000000 */ @@ -6148,8 +6148,7 @@ static const unsigned char fontdata_sun12x22[FONTDATAMAX] = { 0x00, 0x00, /* 000000000000 */ 0x00, 0x00, /* 000000000000 */ 0x00, 0x00, /* 000000000000 */ - -}; +} };
const struct font_desc font_sun_12x22 = { @@ -6157,7 +6156,7 @@ const struct font_desc font_sun_12x22 = { .name = "SUN12x22", .width = 12, .height = 22, - .data = fontdata_sun12x22, + .data = fontdata_sun12x22.data, #ifdef __sparc__ .pref = 5, #else diff --git a/lib/fonts/font_sun8x16.c b/lib/fonts/font_sun8x16.c index 03d71e53954a..193fe6d988e0 100644 --- a/lib/fonts/font_sun8x16.c +++ b/lib/fonts/font_sun8x16.c @@ -3,7 +3,8 @@
#define FONTDATAMAX 4096
-static const unsigned char fontdata_sun8x16[FONTDATAMAX] = { +static struct font_data fontdata_sun8x16 = { +{ 0, 0, FONTDATAMAX, 0 }, { /* */ 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, /* */ 0x00,0x00,0x7e,0x81,0xa5,0x81,0x81,0xbd,0x99,0x81,0x81,0x7e,0x00,0x00,0x00,0x00, /* */ 0x00,0x00,0x7e,0xff,0xdb,0xff,0xff,0xc3,0xe7,0xff,0xff,0x7e,0x00,0x00,0x00,0x00, @@ -260,14 +261,14 @@ static const unsigned char fontdata_sun8x16[FONTDATAMAX] = { /* */ 0x00,0x70,0xd8,0x30,0x60,0xc8,0xf8,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, /* */ 0x00,0x00,0x00,0x00,0x7c,0x7c,0x7c,0x7c,0x7c,0x7c,0x7c,0x00,0x00,0x00,0x00,0x00, /* */ 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, -}; +} };
const struct font_desc font_sun_8x16 = { .idx = SUN8x16_IDX, .name = "SUN8x16", .width = 8, .height = 16, - .data = fontdata_sun8x16, + .data = fontdata_sun8x16.data, #ifdef __sparc__ .pref = 10, #else diff --git a/lib/fonts/font_ter16x32.c b/lib/fonts/font_ter16x32.c index 3f0cf1ccdf3a..91b9c283bd9c 100644 --- a/lib/fonts/font_ter16x32.c +++ b/lib/fonts/font_ter16x32.c @@ -4,8 +4,8 @@
#define FONTDATAMAX 16384
-static const unsigned char fontdata_ter16x32[FONTDATAMAX] = { - +static struct font_data fontdata_ter16x32 = { + { 0, 0, FONTDATAMAX, 0 }, { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x7f, 0xfc, 0x7f, 0xfc, 0x70, 0x1c, 0x70, 0x1c, 0x70, 0x1c, 0x70, 0x1c, @@ -2054,8 +2054,7 @@ static const unsigned char fontdata_ter16x32[FONTDATAMAX] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 255 */ - -}; +} };
const struct font_desc font_ter_16x32 = { @@ -2063,7 +2062,7 @@ const struct font_desc font_ter_16x32 = { .name = "TER16x32", .width = 16, .height = 32, - .data = fontdata_ter16x32, + .data = fontdata_ter16x32.data, #ifdef __sparc__ .pref = 5, #else
fbcon_get_font() is reading out-of-bounds. A malicious user may resize `vc->vc_font.height` to a large value, causing fbcon_get_font() to read out of `fontdata`.
fbcon_get_font() handles both built-in and user-provided fonts. Fortunately, recently we have added FONT_EXTRA_WORDS support for built-in fonts, so fix it by adding range checks using FNTSIZE().
This patch depends on patch "fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h", and patch "Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts".
Cc: stable@vger.kernel.org Reported-and-tested-by: syzbot+29d4ed7f3bdedf2aa2fd@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecf... Signed-off-by: Peilin Ye yepeilin.cs@gmail.com --- drivers/video/fbdev/core/fbcon.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 66167830fefd..bda24c64e646 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -2492,6 +2492,9 @@ static int fbcon_get_font(struct vc_data *vc, struct console_font *font)
if (font->width <= 8) { j = vc->vc_font.height; + if (font->charcount * j > FNTSIZE(fontdata)) + return -EINVAL; + for (i = 0; i < font->charcount; i++) { memcpy(data, fontdata, j); memset(data + j, 0, 32 - j); @@ -2500,6 +2503,9 @@ static int fbcon_get_font(struct vc_data *vc, struct console_font *font) } } else if (font->width <= 16) { j = vc->vc_font.height * 2; + if (font->charcount * j > FNTSIZE(fontdata)) + return -EINVAL; + for (i = 0; i < font->charcount; i++) { memcpy(data, fontdata, j); memset(data + j, 0, 64 - j); @@ -2507,6 +2513,9 @@ static int fbcon_get_font(struct vc_data *vc, struct console_font *font) fontdata += j; } } else if (font->width <= 24) { + if (font->charcount * (vc->vc_font.height * sizeof(u32)) > FNTSIZE(fontdata)) + return -EINVAL; + for (i = 0; i < font->charcount; i++) { for (j = 0; j < vc->vc_font.height; j++) { *data++ = fontdata[0]; @@ -2519,6 +2528,9 @@ static int fbcon_get_font(struct vc_data *vc, struct console_font *font) } } else { j = vc->vc_font.height * 4; + if (font->charcount * j > FNTSIZE(fontdata)) + return -EINVAL; + for (i = 0; i < font->charcount; i++) { memcpy(data, fontdata, j); memset(data + j, 0, 128 - j);
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), 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 keep track of `FONTDATAMAX`.
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
Currently, this is only done 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 these macros on all fonts, no matter built-in or user-provided. As an example, this series fixes the syzbot issue in fbcon_get_font():
(drivers/video/fbdev/core/fbcon.c) if (font->width <= 8) { j = vc->vc_font.height;
if (font->charcount * j > FNTSIZE(fontdata))return -EINVAL;Similarly, newport_con also use these macros. It only uses three of them:
(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>, use four words instead of three, and initialize the fourth word in newport_set_font() properly.
Many thanks to Greg Kroah-Hartman gregkh@linuxfoundation.org, who reviewed and improved this series!
[1]: KASAN: global-out-of-bounds Read in fbcon_get_font https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecf...
Peilin Ye (3): fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts fbcon: Fix global-out-of-bounds read in fbcon_get_font()
drivers/video/console/newport_con.c | 7 +------ drivers/video/fbdev/core/fbcon.c | 12 ++++++++++++ drivers/video/fbdev/core/fbcon.h | 7 ------- drivers/video/fbdev/core/fbcon_rotate.c | 1 + drivers/video/fbdev/core/tileblit.c | 1 + include/linux/font.h | 13 +++++++++++++ lib/fonts/font_10x18.c | 9 ++++----- lib/fonts/font_6x10.c | 9 +++++---- lib/fonts/font_6x11.c | 9 ++++----- lib/fonts/font_7x14.c | 9 ++++----- lib/fonts/font_8x16.c | 9 ++++----- lib/fonts/font_8x8.c | 9 ++++----- lib/fonts/font_acorn_8x8.c | 9 ++++++--- lib/fonts/font_mini_4x6.c | 8 ++++---- lib/fonts/font_pearl_8x8.c | 9 ++++----- lib/fonts/font_sun12x22.c | 9 ++++----- lib/fonts/font_sun8x16.c | 7 ++++--- lib/fonts/font_ter16x32.c | 9 ++++----- 18 files changed, 79 insertions(+), 67 deletions(-)
Gotta love going backwards in arrays :)
Nice work, whole series is:
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Daniel, can you take this through your tree?
thanks,
greg k-h
On Thu, Sep 24, 2020 at 04:09:37PM +0200, Greg Kroah-Hartman wrote:
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
Peilin Ye (3): fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts fbcon: Fix global-out-of-bounds read in fbcon_get_font()
drivers/video/console/newport_con.c | 7 +------ drivers/video/fbdev/core/fbcon.c | 12 ++++++++++++ drivers/video/fbdev/core/fbcon.h | 7 ------- drivers/video/fbdev/core/fbcon_rotate.c | 1 + drivers/video/fbdev/core/tileblit.c | 1 + include/linux/font.h | 13 +++++++++++++ lib/fonts/font_10x18.c | 9 ++++----- lib/fonts/font_6x10.c | 9 +++++---- lib/fonts/font_6x11.c | 9 ++++----- lib/fonts/font_7x14.c | 9 ++++----- lib/fonts/font_8x16.c | 9 ++++----- lib/fonts/font_8x8.c | 9 ++++----- lib/fonts/font_acorn_8x8.c | 9 ++++++--- lib/fonts/font_mini_4x6.c | 8 ++++---- lib/fonts/font_pearl_8x8.c | 9 ++++----- lib/fonts/font_sun12x22.c | 9 ++++----- lib/fonts/font_sun8x16.c | 7 ++++--- lib/fonts/font_ter16x32.c | 9 ++++----- 18 files changed, 79 insertions(+), 67 deletions(-)
Gotta love going backwards in arrays :)
Nice work, whole series is:
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Thank you for reviewing it!
Peilin Ye
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in font data buffers, declared in lib/fonts/font_*.c:
...
(drivers/video/fbdev/core/fbcon.c) if (font->width <= 8) { j = vc->vc_font.height;
if (font->charcount * j > FNTSIZE(fontdata))return -EINVAL;
Can that still go wrong because the multiply wraps?
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi!
On Thu, Sep 24, 2020 at 02:42:18PM +0000, David Laight wrote:
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in font data buffers, declared in lib/fonts/font_*.c:
...
(drivers/video/fbdev/core/fbcon.c) if (font->width <= 8) { j = vc->vc_font.height;
if (font->charcount * j > FNTSIZE(fontdata))return -EINVAL;Can that still go wrong because the multiply wraps?
Thank you for bringing this up!
The resizing of `vc_font.height` happened in vt_resizex():
(drivers/tty/vt/vt_ioctl.c) if (v.v_clin > 32) return -EINVAL; [...] for (i = 0; i < MAX_NR_CONSOLES; i++) { [...] if (v.v_clin) vcp->vc_font.height = v.v_clin; ^^^^^^^^^^^^^^
It does check if `v.v_clin` is greater than 32. And, currently, all built-in fonts have a `charcount` of 256.
Therefore, for built-in fonts and resizing happened in vt_resizex(), it cannot cause an interger overflow.
However I am not very sure about user-provided fonts, and if there are other functions that can resize `height` or even `charcount` to a really huge value, but I will do more investigation and think about it.
Thank you, Peilin Ye
Smatch has a tool to show where struct members are set.
`~/smatch/smatch_data/db/smdb.py where console_font height`
It's not perfect and this output comes from allmodconfig on yesterday's linux-next.
regards, dan carpenter
drivers/video/console/vgacon.c | vgacon_init | (struct console_font)->height | 0-32 drivers/video/console/vgacon.c | vgacon_adjust_height | (struct console_font)->height | 1-32 drivers/video/fbdev/core/fbcon.c | fbcon_startup | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_init | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_do_set_font | (struct console_font)->height | 0-u32max drivers/video/fbdev/core/fbcon.c | fbcon_set_def_font | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_init | (struct console_font)->height | 0-u32max drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_do_font_op | (struct console_font)->height | 1-32 drivers/tty/vt/vt_ioctl.c | vt_k_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_resizex | (struct console_font)->height | 0-u32max drivers/tty/vt/vt_ioctl.c | vt_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_compat_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt.c | vc_allocate | (struct console_font)->height | 0 drivers/tty/vt/vt.c | vt_resize | (struct console_font)->height | ignore drivers/tty/vt/vt.c | do_con_write | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_unthrottle | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_flush_chars | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_shutdown | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_cleanup | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_init | (struct console_font)->height | 0 drivers/tty/vt/vt.c | con_font_set | (struct console_font)->height | 1-32 drivers/tty/vt/vt.c | con_font_default | (struct console_font)->height | 0-u32max drivers/tty/vt/selection.c | paste_selection | (struct console_font)->height | ignore
On Thu, Sep 24, 2020 at 06:45:16PM +0300, Dan Carpenter wrote:
Smatch has a tool to show where struct members are set.
`~/smatch/smatch_data/db/smdb.py where console_font height`
It's not perfect and this output comes from allmodconfig on yesterday's linux-next.
regards, dan carpenter
Wow, thank you for the really interesting information!
drivers/video/console/vgacon.c | vgacon_init | (struct console_font)->height | 0-32 drivers/video/console/vgacon.c | vgacon_adjust_height | (struct console_font)->height | 1-32 drivers/video/fbdev/core/fbcon.c | fbcon_startup | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_init | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_do_set_font | (struct console_font)->height | 0-u32max drivers/video/fbdev/core/fbcon.c | fbcon_set_def_font | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_init | (struct console_font)->height | 0-u32max
In looking at this one, c->vc_font.height = sisusb->current_font_height;
`sisusb->current_font_height` is only set in sisusbcon_do_font_op(): sisusb->current_font_height = fh;
and...
drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_do_font_op | (struct console_font)->height | 1-32
...sisusbcon_do_font_op() is called in four places, with an `fh` of either 16, `sisusb->font_backup_height`, or `font->height`. The latter two cases all come from sisusbcon_font_set(), whose dispatcher, con_font_set() gurantees that `font->height` is less than or equal to 32, as shown by Smatch here.
drivers/tty/vt/vt_ioctl.c | vt_k_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_resizex | (struct console_font)->height | 0-u32max drivers/tty/vt/vt_ioctl.c | vt_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_compat_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt.c | vc_allocate | (struct console_font)->height | 0 drivers/tty/vt/vt.c | vt_resize | (struct console_font)->height | ignore drivers/tty/vt/vt.c | do_con_write | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_unthrottle | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_flush_chars | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_shutdown | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_cleanup | (struct console_font)->height | ignore drivers/tty/vt/vt.c | con_init | (struct console_font)->height | 0 drivers/tty/vt/vt.c | con_font_set | (struct console_font)->height | 1-32 drivers/tty/vt/vt.c | con_font_default | (struct console_font)->height | 0-u32max drivers/tty/vt/selection.c | paste_selection | (struct console_font)->height | ignore
I will go through the list starting from these "0-u32max" cases. Thanks again!
Peilin Ye
On Thu, Sep 24, 2020 at 04:09:37PM +0200, Greg Kroah-Hartman wrote:
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), 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 keep track of `FONTDATAMAX`.
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
Currently, this is only done 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 these macros on all fonts, no matter built-in or user-provided. As an example, this series fixes the syzbot issue in fbcon_get_font():
(drivers/video/fbdev/core/fbcon.c) if (font->width <= 8) { j = vc->vc_font.height;
if (font->charcount * j > FNTSIZE(fontdata))return -EINVAL;Similarly, newport_con also use these macros. It only uses three of them:
(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>, use four words instead of three, and initialize the fourth word in newport_set_font() properly.
Many thanks to Greg Kroah-Hartman gregkh@linuxfoundation.org, who reviewed and improved this series!
[1]: KASAN: global-out-of-bounds Read in fbcon_get_font https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecf...
Peilin Ye (3): fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts fbcon: Fix global-out-of-bounds read in fbcon_get_font()
drivers/video/console/newport_con.c | 7 +------ drivers/video/fbdev/core/fbcon.c | 12 ++++++++++++ drivers/video/fbdev/core/fbcon.h | 7 ------- drivers/video/fbdev/core/fbcon_rotate.c | 1 + drivers/video/fbdev/core/tileblit.c | 1 + include/linux/font.h | 13 +++++++++++++ lib/fonts/font_10x18.c | 9 ++++----- lib/fonts/font_6x10.c | 9 +++++---- lib/fonts/font_6x11.c | 9 ++++----- lib/fonts/font_7x14.c | 9 ++++----- lib/fonts/font_8x16.c | 9 ++++----- lib/fonts/font_8x8.c | 9 ++++----- lib/fonts/font_acorn_8x8.c | 9 ++++++--- lib/fonts/font_mini_4x6.c | 8 ++++---- lib/fonts/font_pearl_8x8.c | 9 ++++----- lib/fonts/font_sun12x22.c | 9 ++++----- lib/fonts/font_sun8x16.c | 7 ++++--- lib/fonts/font_ter16x32.c | 9 ++++----- 18 files changed, 79 insertions(+), 67 deletions(-)
Gotta love going backwards in arrays :)
Nice work, whole series is:
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Daniel, can you take this through your tree?
Applied to drm-misc-fixes, but just missed the pull request train for -rc7. Should land in Linus' tree next week.
But I did look at the code, and I have regrets. Macros into untyped arrays and negative indices is very old skool C. It's definitely neater than before, but also can't deny that we're doing dental surgery on a living and fire breathing dragon here :-/
I guess I'll just add this to the list of requirements anyone has to resolve before we're going to resurrect scrollback.
Cheers, Daniel
thanks,
greg k-h
On 24. 09. 20, 15:38, Peilin Ye wrote:
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in fbcon_get_font(). A malicious user may resize `vc_font.height` to a large value in vt_ioctl(), 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 keep track of `FONTDATAMAX`.
Hi,
but you still can define struct kernel_console_font containing struct console_font and the 4 more members you need in the kernel. See below.
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);
I might be missing something (like coffee in the morning), but why don't you just: 1) declare struct font_data as { unsigned sum, char_count, size, refcnt; const unsigned char data[]; }
Or maybe "struct console_font font" instead of "const unsigned char data[]", if need be.
2) allocate by: kmalloc(struct_size(struct font_data, data, size));
3) use container_of wherever needed
That is you name the data on negative indexes using struct as you already have to define one.
Then you don't need the ugly macros with negative indexes. And you can pass this structure down e.g. to fbcon_do_set_font, avoiding potential mistakes in accessing data[-1] and similar.
thanks,
Hi all!
On Fri, Sep 25, 2020 at 08:46:04AM +0200, Jiri Slaby wrote:
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 keep track of `FONTDATAMAX`.
Hi,
but you still can define struct kernel_console_font containing struct console_font and the 4 more members you need in the kernel. See below.
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);
I might be missing something (like coffee in the morning), but why don't you just:
- declare struct font_data as
{ unsigned sum, char_count, size, refcnt; const unsigned char data[]; }
Or maybe "struct console_font font" instead of "const unsigned char data[]", if need be.
- allocate by:
kmalloc(struct_size(struct font_data, data, size));
- use container_of wherever needed
That is you name the data on negative indexes using struct as you already have to define one.
Then you don't need the ugly macros with negative indexes. And you can pass this structure down e.g. to fbcon_do_set_font, avoiding potential mistakes in accessing data[-1] and similar.
Sorry that I didn't mention it in the cover letter, but yes, I've tried this - a new `kernel_console_font` would be much cleaner than negative array indexing.
The reason I ended up giving it up was, frankly speaking, these macros are being used at about 30 places, and I am not familiar enough with the framebuffer and newport_con code, so I wasn't confident how to clean them up and plug in `kernel_console_font` properly...
Another reason was that, functions like fbcon_get_font() handle both user fonts and built-in fonts, so I wanted a single solution for both of them. I think we can't really introduce `kernel_console_font` while keeping these macros, that would make the error handling logics etc. very messy.
I'm not very sure what to do now. Should I give it another try cleaning up all the macros?
And thank you for reviewing this!
Peilin Ye
On Fri, Sep 25, 2020 at 06:13:00AM -0400, Peilin Ye wrote:
Hi all!
On Fri, Sep 25, 2020 at 08:46:04AM +0200, Jiri Slaby wrote:
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 keep track of `FONTDATAMAX`.
Hi,
but you still can define struct kernel_console_font containing struct console_font and the 4 more members you need in the kernel. See below.
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);
I might be missing something (like coffee in the morning), but why don't you just:
- declare struct font_data as
{ unsigned sum, char_count, size, refcnt; const unsigned char data[]; }
Or maybe "struct console_font font" instead of "const unsigned char data[]", if need be.
- allocate by:
kmalloc(struct_size(struct font_data, data, size));
- use container_of wherever needed
That is you name the data on negative indexes using struct as you already have to define one.
Then you don't need the ugly macros with negative indexes. And you can pass this structure down e.g. to fbcon_do_set_font, avoiding potential mistakes in accessing data[-1] and similar.
Sorry that I didn't mention it in the cover letter, but yes, I've tried this - a new `kernel_console_font` would be much cleaner than negative array indexing.
The reason I ended up giving it up was, frankly speaking, these macros are being used at about 30 places, and I am not familiar enough with the framebuffer and newport_con code, so I wasn't confident how to clean them up and plug in `kernel_console_font` properly...
Another reason was that, functions like fbcon_get_font() handle both user fonts and built-in fonts, so I wanted a single solution for both of them. I think we can't really introduce `kernel_console_font` while keeping these macros, that would make the error handling logics etc. very messy.
I'm not very sure what to do now. Should I give it another try cleaning up all the macros?
And thank you for reviewing this!
I think the only way to make this work is that we have one place which takes in the userspace uapi struct, and then converts it once into a kernel_console_font. With all the error checking.
Then all internal code deals in terms of kernel_console_font, with properly typed and named struct members and helper functions and everything. And we might need a gradual conversion for this, so that first we can convert over invidual console drivers, then subsystems, until at the end we've pushed the conversion from uapi array to kernel_console_font all the way to the ioctl entry points.
But that's indeed a huge pile of work, and fair warning: fbcon is semi-orphaned, so by doing this you'll pretty much volunteer for maintainership :-)
But I'd be very happy to help get this done and throw some maintainership credentials at you in the proces ...
Cheers, Daniel
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
I think the only way to make this work is that we have one place which takes in the userspace uapi struct, and then converts it once into a kernel_console_font. With all the error checking.
Ah, I didn't think of that! When trying to introduce `kernel_console_font` I ended up using the uapi version and the kernel version in parallel...
Then all internal code deals in terms of kernel_console_font, with properly typed and named struct members and helper functions and everything. And we might need a gradual conversion for this, so that first we can convert over invidual console drivers, then subsystems, until at the end we've pushed the conversion from uapi array to kernel_console_font all the way to the ioctl entry points.
But that's indeed a huge pile of work, and fair warning: fbcon is semi-orphaned, so by doing this you'll pretty much volunteer for maintainership :-)
But I'd be very happy to help get this done and throw some maintainership credentials at you in the proces ...
Sounds exciting, I will be glad to do this! I'm just a beginner, but I will try to do what I can do.
Thank you, Peilin Ye
On Fri, Sep 25, 2020 at 11:35:09AM -0400, Peilin Ye wrote:
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
I think the only way to make this work is that we have one place which takes in the userspace uapi struct, and then converts it once into a kernel_console_font. With all the error checking.
Ah, I didn't think of that! When trying to introduce `kernel_console_font` I ended up using the uapi version and the kernel version in parallel...
Then all internal code deals in terms of kernel_console_font, with properly typed and named struct members and helper functions and everything. And we might need a gradual conversion for this, so that first we can convert over invidual console drivers, then subsystems, until at the end we've pushed the conversion from uapi array to kernel_console_font all the way to the ioctl entry points.
But that's indeed a huge pile of work, and fair warning: fbcon is semi-orphaned, so by doing this you'll pretty much volunteer for maintainership :-)
But I'd be very happy to help get this done and throw some maintainership credentials at you in the proces ...
Sounds exciting, I will be glad to do this! I'm just a beginner, but I will try to do what I can do.
If you want to follow along a bit I think would be good to subscribe to the dri-devel mailing list. At least for all the fbcon/fbdev/gpu stuff.
I don't think there's a dedicated list for vt/console stuff, aside from Greg's inbox :-) -Daniel
On Tue, Sep 29, 2020 at 11:09:45AM +0200, Daniel Vetter wrote:
If you want to follow along a bit I think would be good to subscribe to the dri-devel mailing list. At least for all the fbcon/fbdev/gpu stuff.
I don't think there's a dedicated list for vt/console stuff, aside from Greg's inbox :-)
Ah, I've been checking lore.kernel.org/dri-devel/ once a while. Sure! I'll subscribe right now :)
Peilin Ye
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
I think the only way to make this work is that we have one place which takes in the userspace uapi struct, and then converts it once into a kernel_console_font. With all the error checking.
Hi Daniel,
It seems that users don't use `console_font` directly, they use `console_font_op`. Then, in TTY:
(drivers/tty/vt/vt.c) int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { case KD_FONT_OP_SET: return con_font_set(vc, op); case KD_FONT_OP_GET: return con_font_get(vc, op); case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: return con_font_copy(vc, op); } return -ENOSYS; }
These 4 functions allocate `console_font`. We can replace them with our `kernel_console_font`. So, ...
$ vgrep ".con_font_set" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1294 .con_font_set = sisusbcon_font_set, 1 drivers/usb/misc/sisusbvga/sisusb_con.c 1378 .con_font_set = sisusbdummycon_font_set, 2 drivers/video/console/dummycon.c 162 .con_font_set = dummycon_font_set, 3 drivers/video/console/newport_con.c 693 .con_font_set = newport_font_set, 4 drivers/video/console/vgacon.c 1226 .con_font_set = vgacon_font_set, 5 drivers/video/fbdev/core/fbcon.c 3120 .con_font_set = fbcon_set_font, $ $ vgrep ".con_font_get" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get, 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get, 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font, $ $ vgrep ".con_font_default" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default, 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default, 2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default, 3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font, $ $ vgrep ".con_font_copy" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy, 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy, 2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font, $ _
... are these all the callbacks we need to take care of? What about other console drivers that don't register these callbacks? ...
$ vgrep ".con_init" Index File Line Content [...] 3 drivers/usb/misc/sisusbvga/sisusb_con.c 1285 .con_init = sisusbcon_init, 4 drivers/usb/misc/sisusbvga/sisusb_con.c 1369 .con_init = sisusbdummycon_init, 5 drivers/video/console/dummycon.c 153 .con_init = dummycon_init, 6 drivers/video/console/mdacon.c 544 .con_init = mdacon_init, 7 drivers/video/console/newport_con.c 684 .con_init = newport_init, 8 drivers/video/console/sticon.c 328 .con_init = sticon_init, 9 drivers/video/console/vgacon.c 1217 .con_init = vgacon_init, 10 drivers/video/fbdev/core/fbcon.c 3111 .con_init = fbcon_init, [...]
... for example, mdacon.c? What font does mdacon.c use? I know that /lib/fonts/ exports two functions, find_font() and get_default_font(), but I don't see them being used in mdacon.c.
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^
This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Then all internal code deals in terms of kernel_console_font, with properly typed and named struct members and helper functions and everything. And we might need a gradual conversion for this, so that first we can convert over invidual console drivers, then subsystems, until at the end we've pushed the conversion from uapi array to kernel_console_font all the way to the ioctl entry points.
Currently `struct vc_data` contains a `struct console_font vc_font`, and I think this is making gradual conversion very hard. As an example, in fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra information we want in `kernel_console_font`, as long as `struct vc_data` still uses `console_font`...
However, if we let `struct vc_data` use `kernel_console_font` instead, we'll have to handle a lot of things in one go:
$ vgrep --no-less --no-header ".vc_font" | wc -l 296 $ echo ":(" :(
The good news is, I've tried cleaning up all the macros in fbcon.c in my playground, and things seem to work. For example, currently we have:
if (userfont) cnt = FNTCHARCNT(data); else cnt = 256;
After introducing `kernel_console_font` (and adding `charcount` to `struct font_desc` etc.), this should look like:
#define to_font(_data) container_of(_data, struct kernel_console_font, data) [...] cnt = to_font(data)->charcount;
No more `if` and `else`, and the framebuffer layer will be able to support new bulit-in fonts that have more than 256 characters. This seems really nice, so I'd like to spend some time working on it.
However before I start working on real patches, do you have suggestions about which console driver I should start with, or how should I split up the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
Thank you! Peilin Ye
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
I think the only way to make this work is that we have one place which takes in the userspace uapi struct, and then converts it once into a kernel_console_font. With all the error checking.
Hi Daniel,
It seems that users don't use `console_font` directly, they use `console_font_op`. Then, in TTY:
Wow, this is a maze :-/
(drivers/tty/vt/vt.c) int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { case KD_FONT_OP_SET: return con_font_set(vc, op); case KD_FONT_OP_GET: return con_font_get(vc, op); case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: return con_font_copy(vc, op); } return -ENOSYS; }
So my gut feeling is that this is just a bit of overenthusiastic common code sharing, and all it results is confuse everyone. I think if we change the conf_font_get/set/default/copy functions to not take the *op struct (which is take pretty arbitrarily from one of the ioctl), but the parameters each needs directly, that would clean up the code a _lot_. Since most callers would then directly call the right operation, instead of this detour through console_font_op. struct console_font_op is an uapi struct, so really shouldn't be used for internal abstractions - we can't change uapi, hence this makes it impossible to refactor anything from the get-go.
I also think that trying to get rid of con_font_op callers as much as possible (everywhere where the op struct is constructed in the kernel and doesn't come from userspace essentially) should be doable as a stand-alone patch series.
These 4 functions allocate `console_font`. We can replace them with our `kernel_console_font`. So, ...
$ vgrep ".con_font_set"
An aside: git grep is awesome, and really fast.
Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1294 .con_font_set = sisusbcon_font_set, 1 drivers/usb/misc/sisusbvga/sisusb_con.c 1378 .con_font_set = sisusbdummycon_font_set, 2 drivers/video/console/dummycon.c 162 .con_font_set = dummycon_font_set, 3 drivers/video/console/newport_con.c 693 .con_font_set = newport_font_set, 4 drivers/video/console/vgacon.c 1226 .con_font_set = vgacon_font_set, 5 drivers/video/fbdev/core/fbcon.c 3120 .con_font_set = fbcon_set_font, $ $ vgrep ".con_font_get" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get, 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get, 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font, $ $ vgrep ".con_font_default" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default, 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default,
The above two return 0 but do nothing, which means width/height are now bogus (or well the same as what userspace set). I don't think that works correctly ...
2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default,
This just seems to release the userspace font. This is already done in other places where it makes a lot more sense to clean up.
3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font,
This actually does something. tbh I would not be surprises if the fb_set utility is the only thing that uses this - with a bit of code search we could perhaps confirm this, and delete all the other implementations.
$ $ vgrep ".con_font_copy" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy, 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy,
Above two do nothing, but return 0. Again this wont work I think.
2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font,
Smells again like something that's only used by fb_set, and we could probably delete the other dummy implementations. Also I'm not even really clear on what this does ...
Removing these dummy functions means that for a dummy console these ioctls would start failing, but then I don't think anyone boots up into a dummy console and expects font changes to work. So again I think we could split this cleanup as prep work.
$ _
... are these all the callbacks we need to take care of? What about other console drivers that don't register these callbacks? ...
$ vgrep ".con_init" Index File Line Content [...] 3 drivers/usb/misc/sisusbvga/sisusb_con.c 1285 .con_init = sisusbcon_init, 4 drivers/usb/misc/sisusbvga/sisusb_con.c 1369 .con_init = sisusbdummycon_init, 5 drivers/video/console/dummycon.c 153 .con_init = dummycon_init, 6 drivers/video/console/mdacon.c 544 .con_init = mdacon_init, 7 drivers/video/console/newport_con.c 684 .con_init = newport_init, 8 drivers/video/console/sticon.c 328 .con_init = sticon_init, 9 drivers/video/console/vgacon.c 1217 .con_init = vgacon_init, 10 drivers/video/fbdev/core/fbcon.c 3111 .con_init = fbcon_init, [...]
... for example, mdacon.c? What font does mdacon.c use? I know that /lib/fonts/ exports two functions, find_font() and get_default_font(), but I don't see them being used in mdacon.c.
I think all other consoles either don't have fonts at all, or only support built-in fonts.
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
Then all internal code deals in terms of kernel_console_font, with properly typed and named struct members and helper functions and everything. And we might need a gradual conversion for this, so that first we can convert over invidual console drivers, then subsystems, until at the end we've pushed the conversion from uapi array to kernel_console_font all the way to the ioctl entry points.
Currently `struct vc_data` contains a `struct console_font vc_font`, and I think this is making gradual conversion very hard. As an example, in fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra information we want in `kernel_console_font`, as long as `struct vc_data` still uses `console_font`...
However, if we let `struct vc_data` use `kernel_console_font` instead, we'll have to handle a lot of things in one go:
$ vgrep --no-less --no-header ".vc_font" | wc -l 296 $ echo ":(" :(
Yes :-/
This is essentially why the entire vc/fbcon layer is such a mess. It's a chaos, it doesn't really have clear abstraction, and very often the uapi structures (see also conf_font_op) leak deeply into the implementation, which means changing anything is nearly impossible ...
I think for vc_date->vc_font we might need a multi-step approach: - first add a new helper function which sets the font for a vc using an uapi console_font struct (and probably hard-coded assumes cnt == 256. - roll that out everwhere - change the type of vc_font to what we want (which should only need a change in the helper function, which will also set charcount hopefully correctly, using the hard-coded assumption - have another functions which sets the vf_font using a kernel_console_font for all the cases where it matters - now you can start using it and assume the charcount is set correctly
It's a journey unfortunately.
The good news is, I've tried cleaning up all the macros in fbcon.c in my playground, and things seem to work. For example, currently we have:
if (userfont) cnt = FNTCHARCNT(data); else cnt = 256;After introducing `kernel_console_font` (and adding `charcount` to `struct font_desc` etc.), this should look like:
#define to_font(_data) container_of(_data, struct kernel_console_font, data) [...] cnt = to_font(data)->charcount;
Hm I guess we can't unify font_desc and the kernel_console_font we're talking about into one? I think that was brough up already somewhere else in this thread ...
No more `if` and `else`, and the framebuffer layer will be able to support new bulit-in fonts that have more than 256 characters. This seems really nice, so I'd like to spend some time working on it.
However before I start working on real patches, do you have suggestions about which console driver I should start with, or how should I split up the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
I think from a "stop security bugs" trying to clean up fbcon is the important part. That's also the most complex (only one that supports the default and copy functions it seems, and also one of the few that supports get). The other ones I think we should just try to not break. vgacon should still be useable (but I think only on systems where you can boot into legacy bios, not into uefi, at least on x86). I have no idea where some of the other consoles are even used.
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
It seems that users don't use `console_font` directly, they use `console_font_op`. Then, in TTY:
Wow, this is a maze :-/
(drivers/tty/vt/vt.c) int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { case KD_FONT_OP_SET: return con_font_set(vc, op); case KD_FONT_OP_GET: return con_font_get(vc, op); case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: return con_font_copy(vc, op); } return -ENOSYS; }
So my gut feeling is that this is just a bit of overenthusiastic common code sharing, and all it results is confuse everyone. I think if we change the conf_font_get/set/default/copy functions to not take the *op struct (which is take pretty arbitrarily from one of the ioctl), but the parameters each needs directly, that would clean up the code a _lot_. Since most callers would then directly call the right operation, instead of this detour through console_font_op. struct console_font_op is an uapi struct, so really shouldn't be used for internal abstractions - we can't change uapi, hence this makes it impossible to refactor anything from the get-go.
I also think that trying to get rid of con_font_op callers as much as possible (everywhere where the op struct is constructed in the kernel and doesn't come from userspace essentially) should be doable as a stand-alone patch series.
I see, I'll do some code searching and try to clean them up.
These 4 functions allocate `console_font`. We can replace them with our `kernel_console_font`. So, ...
$ vgrep ".con_font_set"
An aside: git grep is awesome, and really fast.
Ah, yes, by default vgrep uses git-grep. I use vgrep when I need to see something colorful :)
$ vgrep ".con_font_get" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get, 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get, 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font, $ $ vgrep ".con_font_default" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default, 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default,
The above two return 0 but do nothing, which means width/height are now bogus (or well the same as what userspace set). I don't think that works correctly ...
2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default,This just seems to release the userspace font. This is already done in other places where it makes a lot more sense to clean up.
3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font,This actually does something. tbh I would not be surprises if the fb_set utility is the only thing that uses this - with a bit of code search we could perhaps confirm this, and delete all the other implementations.
$ vgrep ".con_font_copy" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy, 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy,
Above two do nothing, but return 0. Again this wont work I think.
2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font,Smells again like something that's only used by fb_set, and we could probably delete the other dummy implementations. Also I'm not even really clear on what this does ...
Removing these dummy functions means that for a dummy console these ioctls would start failing, but then I don't think anyone boots up into a dummy console and expects font changes to work. So again I think we could split this cleanup as prep work.
Sure, for step two, I'll read, confirm and try to remove these dummy functions.
... are these all the callbacks we need to take care of? What about other console drivers that don't register these callbacks? ...
... for example, mdacon.c? What font does mdacon.c use? I know that /lib/fonts/ exports two functions, find_font() and get_default_font(), but I don't see them being used in mdacon.c.
I think all other consoles either don't have fonts at all, or only support built-in fonts.
Ah, I see. I'll search for find_font() and get_default_font() when dealing with built-in fonts, then. These files are using them, in addition to fbcon.c:
drivers/firmware/efi/earlycon.c: font = get_default_font(xres, yres, -1, -1); drivers/video/console/sticore.c: fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0);
drivers/media/pci/solo6x10/solo6x10-enc.c: const struct font_desc *vga = find_font("VGA8x16"); drivers/media/test-drivers/vimc/vimc-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/media/test-drivers/vivid/vivid-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/usb/misc/sisusbvga/sisusb.c: myfont = find_font("VGA8x16"); drivers/video/console/sticore.c: fbfont = find_font(fbfont_name);
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
Currently `struct vc_data` contains a `struct console_font vc_font`, and I think this is making gradual conversion very hard. As an example, in fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra information we want in `kernel_console_font`, as long as `struct vc_data` still uses `console_font`...
However, if we let `struct vc_data` use `kernel_console_font` instead, we'll have to handle a lot of things in one go:
$ vgrep --no-less --no-header ".vc_font" | wc -l 296 $ echo ":(" :(
Yes :-/
This is essentially why the entire vc/fbcon layer is such a mess. It's a chaos, it doesn't really have clear abstraction, and very often the uapi structures (see also conf_font_op) leak deeply into the implementation, which means changing anything is nearly impossible ...
I think for vc_date->vc_font we might need a multi-step approach:
- first add a new helper function which sets the font for a vc using
an uapi console_font struct (and probably hard-coded assumes cnt == 256.
But user fonts may have a charcount different to 256... But yes I'll try to figure out how.
- roll that out everwhere
- change the type of vc_font to what we want (which should only need a
change in the helper function, which will also set charcount hopefully correctly, using the hard-coded assumption
- have another functions which sets the vf_font using a
kernel_console_font for all the cases where it matters
- now you can start using it and assume the charcount is set correctly
It's a journey unfortunately.
But at least it now sounds manageable! :)
Thank you, I'll look into this (especially the user charcount issue mentioned above) after cleaning up the uAPi structs and dummy functions.
The good news is, I've tried cleaning up all the macros in fbcon.c in my playground, and things seem to work. For example, currently we have:
if (userfont) cnt = FNTCHARCNT(data); else cnt = 256;After introducing `kernel_console_font` (and adding `charcount` to `struct font_desc` etc.), this should look like:
#define to_font(_data) container_of(_data, struct kernel_console_font, data) [...] cnt = to_font(data)->charcount;
Hm I guess we can't unify font_desc and the kernel_console_font we're talking about into one? I think that was brough up already somewhere else in this thread ...
Sure, let us use `font_desc` from now on.
No more `if` and `else`, and the framebuffer layer will be able to support new bulit-in fonts that have more than 256 characters. This seems really nice, so I'd like to spend some time working on it.
However before I start working on real patches, do you have suggestions about which console driver I should start with, or how should I split up the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
I think from a "stop security bugs" trying to clean up fbcon is the important part. That's also the most complex (only one that supports the default and copy functions it seems, and also one of the few that supports get). The other ones I think we should just try to not break. vgacon should still be useable (but I think only on systems where you can boot into legacy bios, not into uefi, at least on x86). I have no idea where some of the other consoles are even used.
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then `vc_data`. Thank you for the insights!
Peilin Ye
On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote:
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
It seems that users don't use `console_font` directly, they use `console_font_op`. Then, in TTY:
Wow, this is a maze :-/
(drivers/tty/vt/vt.c) int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { case KD_FONT_OP_SET: return con_font_set(vc, op); case KD_FONT_OP_GET: return con_font_get(vc, op); case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: return con_font_copy(vc, op); } return -ENOSYS; }
So my gut feeling is that this is just a bit of overenthusiastic common code sharing, and all it results is confuse everyone. I think if we change the conf_font_get/set/default/copy functions to not take the *op struct (which is take pretty arbitrarily from one of the ioctl), but the parameters each needs directly, that would clean up the code a _lot_. Since most callers would then directly call the right operation, instead of this detour through console_font_op. struct console_font_op is an uapi struct, so really shouldn't be used for internal abstractions - we can't change uapi, hence this makes it impossible to refactor anything from the get-go.
I also think that trying to get rid of con_font_op callers as much as possible (everywhere where the op struct is constructed in the kernel and doesn't come from userspace essentially) should be doable as a stand-alone patch series.
I see, I'll do some code searching and try to clean them up.
These 4 functions allocate `console_font`. We can replace them with our `kernel_console_font`. So, ...
$ vgrep ".con_font_set"
An aside: git grep is awesome, and really fast.
Ah, yes, by default vgrep uses git-grep. I use vgrep when I need to see something colorful :)
$ vgrep ".con_font_get" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get, 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get, 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font, $ $ vgrep ".con_font_default" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default, 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default,
The above two return 0 but do nothing, which means width/height are now bogus (or well the same as what userspace set). I don't think that works correctly ...
2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default,This just seems to release the userspace font. This is already done in other places where it makes a lot more sense to clean up.
3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font,This actually does something. tbh I would not be surprises if the fb_set utility is the only thing that uses this - with a bit of code search we could perhaps confirm this, and delete all the other implementations.
$ vgrep ".con_font_copy" Index File Line Content 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy, 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy,
Above two do nothing, but return 0. Again this wont work I think.
2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font,Smells again like something that's only used by fb_set, and we could probably delete the other dummy implementations. Also I'm not even really clear on what this does ...
Removing these dummy functions means that for a dummy console these ioctls would start failing, but then I don't think anyone boots up into a dummy console and expects font changes to work. So again I think we could split this cleanup as prep work.
Sure, for step two, I'll read, confirm and try to remove these dummy functions.
... are these all the callbacks we need to take care of? What about other console drivers that don't register these callbacks? ...
... for example, mdacon.c? What font does mdacon.c use? I know that /lib/fonts/ exports two functions, find_font() and get_default_font(), but I don't see them being used in mdacon.c.
I think all other consoles either don't have fonts at all, or only support built-in fonts.
Ah, I see. I'll search for find_font() and get_default_font() when dealing with built-in fonts, then. These files are using them, in addition to fbcon.c:
drivers/firmware/efi/earlycon.c: font = get_default_font(xres, yres, -1, -1); drivers/video/console/sticore.c: fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0);
drivers/media/pci/solo6x10/solo6x10-enc.c: const struct font_desc *vga = find_font("VGA8x16"); drivers/media/test-drivers/vimc/vimc-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/media/test-drivers/vivid/vivid-core.c: const struct font_desc *font = find_font("VGA8x16"); drivers/usb/misc/sisusbvga/sisusb.c: myfont = find_font("VGA8x16"); drivers/video/console/sticore.c: fbfont = find_font(fbfont_name);
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
I think the only downside with starting with font_desc as the internal font represenation is that there's a few fields we don't need/have for userspace fonts (like the id/name stuff). So any helpers to e.g. print out font information need to make sure they don't trip over that
But otherwise I don't see a problem with this, I think.
Currently `struct vc_data` contains a `struct console_font vc_font`, and I think this is making gradual conversion very hard. As an example, in fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra information we want in `kernel_console_font`, as long as `struct vc_data` still uses `console_font`...
However, if we let `struct vc_data` use `kernel_console_font` instead, we'll have to handle a lot of things in one go:
$ vgrep --no-less --no-header ".vc_font" | wc -l 296 $ echo ":(" :(
Yes :-/
This is essentially why the entire vc/fbcon layer is such a mess. It's a chaos, it doesn't really have clear abstraction, and very often the uapi structures (see also conf_font_op) leak deeply into the implementation, which means changing anything is nearly impossible ...
I think for vc_date->vc_font we might need a multi-step approach:
- first add a new helper function which sets the font for a vc using
an uapi console_font struct (and probably hard-coded assumes cnt == 256.
But user fonts may have a charcount different to 256... But yes I'll try to figure out how.
Hm yeah, maybe we need a helper to give us the charcount then, which by default is using the magic negative offset.
Then once we've converted everything over to explicitly passing charcount around, we can switch that helper. So something like
int kern_font_charcount(struct kern_font *font);
Feel free to bikeshed the struct name however you see fit :-)
- roll that out everwhere
- change the type of vc_font to what we want (which should only need a
change in the helper function, which will also set charcount hopefully correctly, using the hard-coded assumption
- have another functions which sets the vf_font using a
kernel_console_font for all the cases where it matters
- now you can start using it and assume the charcount is set correctly
It's a journey unfortunately.
But at least it now sounds manageable! :)
Thank you, I'll look into this (especially the user charcount issue mentioned above) after cleaning up the uAPi structs and dummy functions.
The good news is, I've tried cleaning up all the macros in fbcon.c in my playground, and things seem to work. For example, currently we have:
if (userfont) cnt = FNTCHARCNT(data); else cnt = 256;After introducing `kernel_console_font` (and adding `charcount` to `struct font_desc` etc.), this should look like:
#define to_font(_data) container_of(_data, struct kernel_console_font, data) [...] cnt = to_font(data)->charcount;
Hm I guess we can't unify font_desc and the kernel_console_font we're talking about into one? I think that was brough up already somewhere else in this thread ...
Sure, let us use `font_desc` from now on.
No more `if` and `else`, and the framebuffer layer will be able to support new bulit-in fonts that have more than 256 characters. This seems really nice, so I'd like to spend some time working on it.
However before I start working on real patches, do you have suggestions about which console driver I should start with, or how should I split up the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
I think from a "stop security bugs" trying to clean up fbcon is the important part. That's also the most complex (only one that supports the default and copy functions it seems, and also one of the few that supports get). The other ones I think we should just try to not break. vgacon should still be useable (but I think only on systems where you can boot into legacy bios, not into uefi, at least on x86). I have no idea where some of the other consoles are even used.
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then `vc_data`. Thank you for the insights!
Please don't take this rough plan as fixed, it's just where I'd start from browsing the code and your analysis a bit. We'll probably have to adapt as we go and more nasty things turn up ...
Cheers, Daniel
On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote:
On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote:
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
I think the only downside with starting with font_desc as the internal font represenation is that there's a few fields we don't need/have for userspace fonts (like the id/name stuff). So any helpers to e.g. print out font information need to make sure they don't trip over that
But otherwise I don't see a problem with this, I think.
Yes, and built-in fonts don't use refcount. Or maybe we can let find_font() and get_default_font() kmalloc() a copy of built-in font data, then keep track of refcount for both user and built-in fonts, but that will waste a few K of memory for each built-in font we use...
I think for vc_date->vc_font we might need a multi-step approach:
- first add a new helper function which sets the font for a vc using
an uapi console_font struct (and probably hard-coded assumes cnt == 256.
But user fonts may have a charcount different to 256... But yes I'll try to figure out how.
Hm yeah, maybe we need a helper to give us the charcount then, which by default is using the magic negative offset.
Ah, I see! :)
Then once we've converted everything over to explicitly passing charcount around, we can switch that helper. So something like
int kern_font_charcount(struct kern_font *font);
Feel free to bikeshed the struct name however you see fit :-)
I think both `kern_font` and `font_desc` makes sense, naming is so hard...
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then `vc_data`. Thank you for the insights!
Please don't take this rough plan as fixed, it's just where I'd start from browsing the code and your analysis a bit. We'll probably have to adapt as we go and more nasty things turn up ...
Sure, I'll first give it a try and see. Thank you!
Peilin Ye
On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye yepeilin.cs@gmail.com wrote:
On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote:
On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote:
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
I think the only downside with starting with font_desc as the internal font represenation is that there's a few fields we don't need/have for userspace fonts (like the id/name stuff). So any helpers to e.g. print out font information need to make sure they don't trip over that
But otherwise I don't see a problem with this, I think.
Yes, and built-in fonts don't use refcount. Or maybe we can let find_font() and get_default_font() kmalloc() a copy of built-in font data, then keep track of refcount for both user and built-in fonts, but that will waste a few K of memory for each built-in font we use...
A possible trick for this would be to make sure built-in fonts start out with a refcount of 1. So never get freed. Plus maybe a check that if the name is set, then it's a built-in font and if we ever underflow the refcount we just WARN, but don't free anything.
Another trick would be kern_font_get/put wrappers (we'd want those anyway if the userspace fonts are refcounted) and if kern_font->name != NULL (i.e. built-in font with name) then we simply don't call kref_get/put. -Daniel
I think for vc_date->vc_font we might need a multi-step approach:
- first add a new helper function which sets the font for a vc using
an uapi console_font struct (and probably hard-coded assumes cnt == 256.
But user fonts may have a charcount different to 256... But yes I'll try to figure out how.
Hm yeah, maybe we need a helper to give us the charcount then, which by default is using the magic negative offset.
Ah, I see! :)
Then once we've converted everything over to explicitly passing charcount around, we can switch that helper. So something like
int kern_font_charcount(struct kern_font *font);
Feel free to bikeshed the struct name however you see fit :-)
I think both `kern_font` and `font_desc` makes sense, naming is so hard...
For first steps I'd start with demidlayering some of the internal users of uapi structs, like the console_font_op really shouldn't be used anywhere in any function, except in the ioctl handler that converts it into the right function call. You'll probably discover a few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then `vc_data`. Thank you for the insights!
Please don't take this rough plan as fixed, it's just where I'd start from browsing the code and your analysis a bit. We'll probably have to adapt as we go and more nasty things turn up ...
Sure, I'll first give it a try and see. Thank you!
Peilin Ye
On Wed, Sep 30, 2020 at 01:25:14PM +0200, Daniel Vetter wrote:
On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye yepeilin.cs@gmail.com wrote:
On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote:
On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote:
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye yepeilin.cs@gmail.com wrote:
Ah, and speaking of built-in fonts, see fbcon_startup():
/* Setup default font */ [...] vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */ ^^^^^^^^^^^^^^^This is because find_font() and get_default_font() return a `struct font_desc *`, but `struct font_desc` doesn't contain `charcount`. I think we also need to add a `charcount` field to `struct font_desc`.
Hm yeah ... I guess maybe struct font_desc should be the starting point for the kernel internal font structure. It's at least there already ...
I see, that will also make handling built-in fonts much easier!
I think the only downside with starting with font_desc as the internal font represenation is that there's a few fields we don't need/have for userspace fonts (like the id/name stuff). So any helpers to e.g. print out font information need to make sure they don't trip over that
But otherwise I don't see a problem with this, I think.
Yes, and built-in fonts don't use refcount. Or maybe we can let find_font() and get_default_font() kmalloc() a copy of built-in font data, then keep track of refcount for both user and built-in fonts, but that will waste a few K of memory for each built-in font we use...
A possible trick for this would be to make sure built-in fonts start out with a refcount of 1. So never get freed. Plus maybe a check that if the name is set, then it's a built-in font and if we ever underflow the refcount we just WARN, but don't free anything.
Another trick would be kern_font_get/put wrappers (we'd want those anyway if the userspace fonts are refcounted) and if kern_font->name != NULL (i.e. built-in font with name) then we simply don't call kref_get/put.
Ick, don't do that, the first trick of having them start out with an increased reference count is the best way here. Makes the code simpler and no special cases for the tear-down path.
thanks,
greg k-h
On Wed, Sep 30, 2020 at 01:52:11PM +0200, Greg Kroah-Hartman wrote:
On Wed, Sep 30, 2020 at 01:25:14PM +0200, Daniel Vetter wrote:
On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye yepeilin.cs@gmail.com wrote:
Yes, and built-in fonts don't use refcount. Or maybe we can let find_font() and get_default_font() kmalloc() a copy of built-in font data, then keep track of refcount for both user and built-in fonts, but that will waste a few K of memory for each built-in font we use...
A possible trick for this would be to make sure built-in fonts start out with a refcount of 1. So never get freed. Plus maybe a check that if the name is set, then it's a built-in font and if we ever underflow the refcount we just WARN, but don't free anything.
Another trick would be kern_font_get/put wrappers (we'd want those anyway if the userspace fonts are refcounted) and if kern_font->name != NULL (i.e. built-in font with name) then we simply don't call kref_get/put.
Ick, don't do that, the first trick of having them start out with an increased reference count is the best way here. Makes the code simpler and no special cases for the tear-down path.
I see, I'll just let them start out with 1, and only check `->name != NULL` in kern_font_put(). Thank you!
Peilin Ye
On 29. 09. 20, 14:34, Peilin Ye wrote:
the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
Hi,
feel free to change struct vc_data's content as you need, of course. Only the UAPI _definitions_ have to be preserved (like struct console_font).
thanks,
On Wed, Sep 30, 2020 at 07:26:52AM +0200, Jiri Slaby wrote:
On 29. 09. 20, 14:34, Peilin Ye wrote:
the work in general? I couldn't think of how do we clean up subsystems one by one, while keeping a `console_font` in `struct vc_data`.
Hi,
feel free to change struct vc_data's content as you need, of course. Only the UAPI _definitions_ have to be preserved (like struct console_font).
Ah, I see. Thank you for the reminder!
Peilin Ye
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Daniel Vetter -
David Laight -
Greg Kroah-Hartman -
Jiri Slaby -
Peilin Ye -
syzbot