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

From: Jiri Slaby
Date: Fri Oct 16 2020 - 08:24:20 EST


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@xxxxxxx>
---
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