[Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[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(-)

-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 1/3] fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h

[Peilin Ye]

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 */
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] [PATCH 2/3] Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts

[Peilin Ye]

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=08b8be45afea11888776f897895aef9ad1c3ecfd
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
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Peilin Ye]

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=08b8be45afea11888776f897895aef9ad1c3ecfd
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);
-- 
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Greg Kroah-Hartman]

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[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:
> > 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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[David Laight]

> 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)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Dan Carpenter]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Jiri Slaby]

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,
-- 
js
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[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.

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Jiri Slaby]

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,
-- 
js
suse labs
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Daniel Vetter]

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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Greg Kroah-Hartman]

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers

[Peilin Ye]

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

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees