[PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

[PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

[Meng Xu]

In con_font_set(), when we need to guess font height (for
compat reasons?), the current approach uses multiple userspace
fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive
the height. This has two drawbacks:

1. performance: accessing userspace memory is less efficient than
directly de-reference the byte

2. security: a more critical problem is that the height derived
might not match with the actual font.data. This is because a user
thread might race condition to change the memory of op->data after
the op->height guessing but before the second fetch: font.data =
memdup_user(op->data, size). Leaving font.height = 32 while the
actual height is 1 or vice-versa.

This patch tries to resolve both issues by re-locating the height
guessing part after the font.data is fetched in. In this way, the
userspace data is fetched in one shot and we directly dereference
the font.data in kernel space to probe for the height.

Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
---
 drivers/tty/vt/vt.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 2ebaba1..a43cecb 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4121,37 +4121,45 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
 		return -EINVAL;
 	if (op->charcount > 512)
 		return -EINVAL;
+	if (op->width <= 0 || op->width > 32 || op->height > 32)
+		return -EINVAL;
+	size = (op->width+7)/8 * 32 * op->charcount;
+	if (size > max_font_size)
+		return -ENOSPC;
+
+	font.data = memdup_user(op->data, size);
+	if (IS_ERR(font.data))
+		return PTR_ERR(font.data);
+
 	if (!op->height) {		/* Need to guess font height [compat] */
 		int h, i;
-		u8 __user *charmap = op->data;
-		u8 tmp;
-		
-		/* If from KDFONTOP ioctl, don't allow things which can be done in userland,
-		   so that we can get rid of this soon */
-		if (!(op->flags & KD_FONT_FLAG_OLD))
+		u8 *charmap = font.data;
+
+		/* 
+		 * If from KDFONTOP ioctl, don't allow things which can be done in userland,
+		 * so that we can get rid of this soon 
+		 */
+		if (!(op->flags & KD_FONT_FLAG_OLD)) {
+			kfree(font.data);
 			return -EINVAL;
+		}
+
 		for (h = 32; h > 0; h--)
-			for (i = 0; i < op->charcount; i++) {
-				if (get_user(tmp, &charmap[32*i+h-1]))
-					return -EFAULT;
-				if (tmp)
+			for (i = 0; i < op->charcount; i++)
+				if (charmap[32*i+h-1])
 					goto nonzero;
-			}
+
+		kfree(font.data);
 		return -EINVAL;
+
 	nonzero:
 		op->height = h;
 	}
-	if (op->width <= 0 || op->width > 32 || op->height > 32)
-		return -EINVAL;
-	size = (op->width+7)/8 * 32 * op->charcount;
-	if (size > max_font_size)
-		return -ENOSPC;
+
 	font.charcount = op->charcount;
-	font.height = op->height;
 	font.width = op->width;
-	font.data = memdup_user(op->data, size);
-	if (IS_ERR(font.data))
-		return PTR_ERR(font.data);
+	font.height = op->height;
+
 	console_lock();
 	if (vc->vc_mode != KD_TEXT)
 		rc = -EINVAL;
-- 
2.7.4

Re: [PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

[Greg KH]

On Sun, Sep 24, 2017 at 12:59:42PM -0400, Meng Xu wrote:
> In con_font_set(), when we need to guess font height (for
> compat reasons?), the current approach uses multiple userspace
> fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive
> the height. This has two drawbacks:
> 
> 1. performance: accessing userspace memory is less efficient than
> directly de-reference the byte
> 
> 2. security: a more critical problem is that the height derived
> might not match with the actual font.data. This is because a user
> thread might race condition to change the memory of op->data after
> the op->height guessing but before the second fetch: font.data =
> memdup_user(op->data, size). Leaving font.height = 32 while the
> actual height is 1 or vice-versa.
> 
> This patch tries to resolve both issues by re-locating the height
> guessing part after the font.data is fetched in. In this way, the
> userspace data is fetched in one shot and we directly dereference
> the font.data in kernel space to probe for the height.
> 
> Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
> ---
>  drivers/tty/vt/vt.c | 48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Please always run your patches through checkpatch.pl so that you don't
get a grumpy maintainer telling you to use checkpatch.pl :(

Can you fix the obvious issues up and resend?

thanks,

greg k-h

[PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

[Meng Xu]

In con_font_set(), when we need to guess font height (for
compat reasons?), the current approach uses multiple userspace
fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive
the height. This has two drawbacks:

1. performance: accessing userspace memory is less efficient than
directly de-reference the byte

2. security: a more critical problem is that the height derived
might not match with the actual font.data. This is because a user
thread might race condition to change the memory of op->data after
the op->height guessing but before the second fetch: font.data =
memdup_user(op->data, size). Leaving font.height = 32 while the
actual height is 1 or vice-versa.

This patch tries to resolve both issues by re-locating the height
guessing part after the font.data is fetched in. In this way, the
userspace data is fetched in one shot and we directly dereference
the font.data in kernel space to probe for the height.

Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
---
 drivers/tty/vt/vt.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 2ebaba1..291e2b0 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4121,37 +4121,45 @@ static int con_font_set(struct vc_data *vc, struct console_font_op *op)
 		return -EINVAL;
 	if (op->charcount > 512)
 		return -EINVAL;
+	if (op->width <= 0 || op->width > 32 || op->height > 32)
+		return -EINVAL;
+	size = (op->width+7)/8 * 32 * op->charcount;
+	if (size > max_font_size)
+		return -ENOSPC;
+
+	font.data = memdup_user(op->data, size);
+	if (IS_ERR(font.data))
+		return PTR_ERR(font.data);
+
 	if (!op->height) {		/* Need to guess font height [compat] */
 		int h, i;
-		u8 __user *charmap = op->data;
-		u8 tmp;
-		
-		/* If from KDFONTOP ioctl, don't allow things which can be done in userland,
-		   so that we can get rid of this soon */
-		if (!(op->flags & KD_FONT_FLAG_OLD))
+		u8 *charmap = font.data;
+
+		/*
+		 * If from KDFONTOP ioctl, don't allow things which can be done
+		 * in userland,so that we can get rid of this soon
+		 */
+		if (!(op->flags & KD_FONT_FLAG_OLD)) {
+			kfree(font.data);
 			return -EINVAL;
+		}
+
 		for (h = 32; h > 0; h--)
-			for (i = 0; i < op->charcount; i++) {
-				if (get_user(tmp, &charmap[32*i+h-1]))
-					return -EFAULT;
-				if (tmp)
+			for (i = 0; i < op->charcount; i++)
+				if (charmap[32*i+h-1])
 					goto nonzero;
-			}
+
+		kfree(font.data);
 		return -EINVAL;
+
 	nonzero:
 		op->height = h;
 	}
-	if (op->width <= 0 || op->width > 32 || op->height > 32)
-		return -EINVAL;
-	size = (op->width+7)/8 * 32 * op->charcount;
-	if (size > max_font_size)
-		return -ENOSPC;
+
 	font.charcount = op->charcount;
-	font.height = op->height;
 	font.width = op->width;
-	font.data = memdup_user(op->data, size);
-	if (IS_ERR(font.data))
-		return PTR_ERR(font.data);
+	font.height = op->height;
+
 	console_lock();
 	if (vc->vc_mode != KD_TEXT)
 		rc = -EINVAL;
-- 
2.7.4

Re: [PATCH] tty: vt: remove multi-fetch, derive font.height from font.data

[Greg KH]

On Wed, Oct 04, 2017 at 10:12:38AM -0400, Meng Xu wrote:
> In con_font_set(), when we need to guess font height (for
> compat reasons?), the current approach uses multiple userspace
> fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive
> the height. This has two drawbacks:
> 
> 1. performance: accessing userspace memory is less efficient than
> directly de-reference the byte
> 
> 2. security: a more critical problem is that the height derived
> might not match with the actual font.data. This is because a user
> thread might race condition to change the memory of op->data after
> the op->height guessing but before the second fetch: font.data =
> memdup_user(op->data, size). Leaving font.height = 32 while the
> actual height is 1 or vice-versa.
> 
> This patch tries to resolve both issues by re-locating the height
> guessing part after the font.data is fetched in. In this way, the
> userspace data is fetched in one shot and we directly dereference
> the font.data in kernel space to probe for the height.
> 
> Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
> ---
>  drivers/tty/vt/vt.c | 48 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Is this a v2 patch?  If so, please mark it as such and say what changed
below the --- line.  The kernel documenation says how to do this
correctly.

thanks,

greg k-h