[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
@ 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; 6+ 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 related	[flat|nested] 6+ messages in thread