| Message ID | cover.1600953813.git.yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Prevent out-of-bounds access for built-in font data buffers | expand |
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=08b8be45afea11888776f897895aef9ad1c3ecfd > > 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 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,
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=08b8be45afea11888776f897895aef9ad1c3ecfd > > > > 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
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: > 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. 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: > > 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. > > 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 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 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 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
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
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=08b8be45afea11888776f897895aef9ad1c3ecfd 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(-)