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 <[email protected]> --- 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
