Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl
@ 2020-10-16 12:24 Jiri Slaby
  2020-10-16 12:24 ` [PATCH 2/3] vt: keyboard, simplify vt_kdgkbsent Jiri Slaby
  2020-10-16 12:24 ` [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers Jiri Slaby
  0 siblings, 2 replies; 5+ messages in thread
From: Jiri Slaby @ 2020-10-16 12:24 UTC (permalink / raw)
  To: gregkh; +Cc: linux-serial, linux-kernel, Minh Yuan, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] vt: keyboard, simplify vt_kdgkbsent
  2020-10-16 12:24 [PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl Jiri Slaby
@ 2020-10-16 12:24 ` Jiri Slaby
  2020-10-16 12:24 ` [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers Jiri Slaby
  1 sibling, 0 replies; 5+ messages in thread
From: Jiri Slaby @ 2020-10-16 12:24 UTC (permalink / raw)
  To: gregkh; +Cc: linux-serial, linux-kernel, Minh Yuan, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers
  2020-10-16 12:24 [PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl Jiri Slaby
  2020-10-16 12:24 ` [PATCH 2/3] vt: keyboard, simplify vt_kdgkbsent Jiri Slaby
@ 2020-10-16 12:24 ` Jiri Slaby
  2020-10-16 13:20   ` Greg KH
  1 sibling, 1 reply; 5+ messages in thread
From: Jiri Slaby @ 2020-10-16 12:24 UTC (permalink / raw)
  To: gregkh; +Cc: linux-serial, linux-kernel, Minh Yuan, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers
  2020-10-16 12:24 ` [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers Jiri Slaby
@ 2020-10-16 13:20   ` Greg KH
  2020-10-19  8:00     ` Jiri Slaby
  0 siblings, 1 reply; 5+ messages in thread
From: Greg KH @ 2020-10-16 13:20 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: linux-serial, linux-kernel, Minh Yuan

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers
  2020-10-16 13:20   ` Greg KH
@ 2020-10-19  8:00     ` Jiri Slaby
  0 siblings, 0 replies; 5+ messages in thread
From: Jiri Slaby @ 2020-10-19  8:00 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-serial, linux-kernel, Minh Yuan

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 12:24 [PATCH 1/3] vt: keyboard, reorder user buffer handling in vt_do_kdgkb_ioctl Jiri Slaby
2020-10-16 12:24 ` [PATCH 2/3] vt: keyboard, simplify vt_kdgkbsent Jiri Slaby
2020-10-16 12:24 ` [PATCH 3/3] vt: keyboard, extend func_buf_lock to readers Jiri Slaby
2020-10-16 13:20   ` Greg KH
2020-10-19  8:00     ` Jiri Slaby

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git