[PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl

[PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl

[Jiri Slaby]

KDGKBSENT (the getter) needs only user_kdgkb->kb_func from the
userspace, i.e. the index. So do the complete copy only in KDSKBSENT
(the setter). That means, we obtain the index before the switch-case
and use it in both paths and copy the string only in the setter case.
And we do it by strndup_user helper now which was not available when
this function was written.

Given we copy the two members of struct kbsentry separately, we no
longer need a local definition. Hence we need to change all the sizeofs
here too.

And also the getter now returns in all fail paths, not freeing the
setter's string.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/keyboard.c | 51 ++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 0db53b5b3acf..d8e2452da1bd 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1994,7 +1994,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
 /* FIXME: This one needs untangling and locking */
 int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 {
-	struct kbsentry *kbs;
+	char *kbs;
 	char *p;
 	u_char *q;
 	u_char __user *up;
@@ -2008,43 +2008,34 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 	if (!capable(CAP_SYS_TTY_CONFIG))
 		perm = 0;
 
-	kbs = kmalloc(sizeof(*kbs), GFP_KERNEL);
-	if (!kbs) {
-		ret = -ENOMEM;
-		goto reterr;
-	}
+	if (get_user(i, &user_kdgkb->kb_func))
+		return -EFAULT;
 
-	/* we mostly copy too much here (512bytes), but who cares ;) */
-	if (copy_from_user(kbs, user_kdgkb, sizeof(struct kbsentry))) {
-		ret = -EFAULT;
-		goto reterr;
-	}
-	kbs->kb_string[sizeof(kbs->kb_string)-1] = '\0';
-	i = array_index_nospec(kbs->kb_func, MAX_NR_FUNC);
+	i = array_index_nospec(i, MAX_NR_FUNC);
 
 	switch (cmd) {
 	case KDGKBSENT:
-		sz = sizeof(kbs->kb_string) - 1; /* sz should have been
-						  a struct member */
+		/* sz should have been a struct member */
+		sz = sizeof_field(struct kbsentry, kb_string) - 1;
 		up = user_kdgkb->kb_string;
 		p = func_table[i];
 		if(p)
 			for ( ; *p && sz; p++, sz--)
-				if (put_user(*p, up++)) {
-					ret = -EFAULT;
-					goto reterr;
-				}
-		if (put_user('\0', up)) {
-			ret = -EFAULT;
-			goto reterr;
-		}
-		kfree(kbs);
+				if (put_user(*p, up++))
+					return -EFAULT;
+
+		if (put_user('\0', up))
+			return -EFAULT;
+
 		return ((p && *p) ? -EOVERFLOW : 0);
 	case KDSKBSENT:
-		if (!perm) {
-			ret = -EPERM;
-			goto reterr;
-		}
+		if (!perm)
+			return -EPERM;
+
+		kbs = strndup_user(user_kdgkb->kb_string,
+				sizeof(user_kdgkb->kb_string));
+		if (IS_ERR(kbs))
+			return PTR_ERR(kbs);
 
 		fnw = NULL;
 		fnw_sz = 0;
@@ -2062,7 +2053,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 		else
 			fj = first_free;
 		/* buffer usage increase by new entry */
-		delta = (q ? -strlen(q) : 1) + strlen(kbs->kb_string);
+		delta = (q ? -strlen(q) : 1) + strlen(kbs);
 
 		if (delta <= funcbufleft) { 	/* it fits in current buf */
 		    if (j < MAX_NR_FUNC) {
@@ -2114,7 +2105,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 		    funcbufsize = sz;
 		}
 		/* finally insert item itself */
-		strcpy(func_table[i], kbs->kb_string);
+		strcpy(func_table[i], kbs);
 		spin_unlock_irqrestore(&func_buf_lock, flags);
 		break;
 	}
-- 
2.28.0

[PATCH 2/3] vt: keyboard, simplify vt_kdgkbsent

[Jiri Slaby]

Use 'strlen' of the string, add one for NUL and simply do 'copy_to_user'
instead of the explicit 'for' loop. This makes the KDGKBSENT case more
compact.

The only thing we need to take care about is NULL 'from'.

The original check for overflow could never trigger as the func_buf
(called 'from' here) strings are always shorter or equal to struct
kbsentry's.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/vt/keyboard.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index d8e2452da1bd..68f9f6a62d02 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -1995,9 +1995,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
 int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 {
 	char *kbs;
-	char *p;
 	u_char *q;
-	u_char __user *up;
 	int sz, fnw_sz;
 	int delta;
 	char *first_free, *fj, *fnw;
@@ -2014,20 +2012,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 	i = array_index_nospec(i, MAX_NR_FUNC);
 
 	switch (cmd) {
-	case KDGKBSENT:
-		/* sz should have been a struct member */
-		sz = sizeof_field(struct kbsentry, kb_string) - 1;
-		up = user_kdgkb->kb_string;
-		p = func_table[i];
-		if(p)
-			for ( ; *p && sz; p++, sz--)
-				if (put_user(*p, up++))
-					return -EFAULT;
-
-		if (put_user('\0', up))
+	case KDGKBSENT: {
+		/* size should have been a struct member */
+		unsigned char *from = func_table[i] ? : "";
+
+		if (copy_to_user(user_kdgkb->kb_string, from, strlen(from) + 1))
 			return -EFAULT;
 
-		return ((p && *p) ? -EOVERFLOW : 0);
+		return 0;
+	}
 	case KDSKBSENT:
 		if (!perm)
 			return -EPERM;
-- 
2.28.0

[PATCH 3/3] vt: keyboard, extend func_buf_lock to readers

[Jiri Slaby]

Both read-side users of func_table/func_buf need locking. Without that,
one can easily confuse the code by repeatedly setting altering strings
like:
while (1)
	for (a = 0; a < 2; a++) {
		struct kbsentry kbs = {};
		strcpy((char *)kbs.kb_string, a ? ".\n" : "88888\n");
		ioctl(fd, KDSKBSENT, &kbs);
	}

When that program runs, one can get unexpected output by holding F1
(note the unxpected period on the last line):
.
88888
.8888

So protect all accesses to 'func_table' (and func_buf) by preexisting
'func_buf_lock'.

It is easy in 'k_fn' handler as 'puts_queue' is expected not to sleep.
On the other hand, KDGKBSENT needs a local (atomic) copy of the string
because copy_to_user can sleep.

Likely fixes CVE-2020-25656.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: Minh Yuan <yuanmingbuaa@gmail.com>
---
 drivers/tty/vt/keyboard.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 68f9f6a62d02..68b1acc0074c 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -743,8 +743,13 @@ static void k_fn(struct vc_data *vc, unsigned char value, char up_flag)
 		return;
 
 	if ((unsigned)value < ARRAY_SIZE(func_table)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&func_buf_lock, flags);
 		if (func_table[value])
 			puts_queue(vc, func_table[value]);
+		spin_unlock_irqrestore(&func_buf_lock, flags);
+
 	} else
 		pr_err("k_fn called with value=%d\n", value);
 }
@@ -1991,7 +1996,7 @@ int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm,
 #undef s
 #undef v
 
-/* FIXME: This one needs untangling and locking */
+/* FIXME: This one needs untangling */
 int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 {
 	char *kbs;
@@ -2014,12 +2019,23 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 	switch (cmd) {
 	case KDGKBSENT: {
 		/* size should have been a struct member */
-		unsigned char *from = func_table[i] ? : "";
+		char *func_copy;
+		ssize_t len = sizeof(user_kdgkb->kb_string);
 
-		if (copy_to_user(user_kdgkb->kb_string, from, strlen(from) + 1))
-			return -EFAULT;
+		func_copy = kmalloc(len, GFP_KERNEL);
+		if (!func_copy)
+			return -ENOMEM;
 
-		return 0;
+		spin_lock_irqsave(&func_buf_lock, flags);
+		len = strlcpy(func_copy, func_table[i] ? : "", len);
+		spin_unlock_irqrestore(&func_buf_lock, flags);
+
+		ret = copy_to_user(user_kdgkb->kb_string, func_copy, len + 1) ?
+			-EFAULT : 0;
+
+		kfree(func_copy);
+
+		return ret;
 	}
 	case KDSKBSENT:
 		if (!perm)
-- 
2.28.0

Re: [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers

[Greg KH]

On Fri, Oct 16, 2020 at 02:24:12PM +0200, Jiri Slaby wrote:
> Both read-side users of func_table/func_buf need locking. Without that,
> one can easily confuse the code by repeatedly setting altering strings
> like:
> while (1)
> 	for (a = 0; a < 2; a++) {
> 		struct kbsentry kbs = {};
> 		strcpy((char *)kbs.kb_string, a ? ".\n" : "88888\n");
> 		ioctl(fd, KDSKBSENT, &kbs);
> 	}
> 
> When that program runs, one can get unexpected output by holding F1
> (note the unxpected period on the last line):
> .
> 88888
> .8888
> 
> So protect all accesses to 'func_table' (and func_buf) by preexisting
> 'func_buf_lock'.
> 
> It is easy in 'k_fn' handler as 'puts_queue' is expected not to sleep.
> On the other hand, KDGKBSENT needs a local (atomic) copy of the string
> because copy_to_user can sleep.
> 
> Likely fixes CVE-2020-25656.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: Minh Yuan <yuanmingbuaa@gmail.com>
> ---
>  drivers/tty/vt/keyboard.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)

So all 3 of these should go to 5.10-final?

thanks,

greg k-h

Re: [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers

[Jiri Slaby]

On 16. 10. 20, 15:20, Greg KH wrote:
> On Fri, Oct 16, 2020 at 02:24:12PM +0200, Jiri Slaby wrote:
>> Both read-side users of func_table/func_buf need locking. Without that,
>> one can easily confuse the code by repeatedly setting altering strings
>> like:
>> while (1)
>> 	for (a = 0; a < 2; a++) {
>> 		struct kbsentry kbs = {};
>> 		strcpy((char *)kbs.kb_string, a ? ".\n" : "88888\n");
>> 		ioctl(fd, KDSKBSENT, &kbs);
>> 	}
>>
>> When that program runs, one can get unexpected output by holding F1
>> (note the unxpected period on the last line):
>> .
>> 88888
>> .8888
>>
>> So protect all accesses to 'func_table' (and func_buf) by preexisting
>> 'func_buf_lock'.
>>
>> It is easy in 'k_fn' handler as 'puts_queue' is expected not to sleep.
>> On the other hand, KDGKBSENT needs a local (atomic) copy of the string
>> because copy_to_user can sleep.
>>
>> Likely fixes CVE-2020-25656.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Reported-by: Minh Yuan <yuanmingbuaa@gmail.com>
>> ---
>>   drivers/tty/vt/keyboard.c | 26 +++++++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> So all 3 of these should go to 5.10-final?

Let me try to eliminate also patch 1/3 which I now think is possible.

-- 
js
suse labs