ACK+Cmnt: [PATCH 1/2] vt: keyboard, simplify vt_kdgkbsent

[Thadeu Lima de Souza Cascardo]

On Mon, Feb 22, 2021 at 12:39:21PM -0700, Tim Gardner wrote:
> From: Jiri Slaby <jslaby at suse.cz>
> 
> Use 'strlen' of the string, add one for NUL terminator 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 'func_table[i]'. Use
> an empty string in that case.
> 
> The original check for overflow could never trigger as the func_buf
> strings are always shorter or equal to 'struct kbsentry's.
> 
> Cc: <stable at vger.kernel.org>
> Signed-off-by: Jiri Slaby <jslaby at suse.cz>
> Link: https://lore.kernel.org/r/20201019085517.10176-1-jslaby@suse.cz
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry picked from commit 6ca03f90527e499dd5e32d6522909e2ad390896b)

As this is part of a fix for CVE-2020-25656, there needs to be a CVE line,
which we usually put around here:

CVE-2020-25656

So, this commit appears as part of the changelog entry for the CVE.

Aside from that,

Acked-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>

> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> ---
>  drivers/tty/vt/keyboard.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 568b2171f335..e64e3a7aa2f1 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -1994,9 +1994,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)
>  {
>  	struct kbsentry *kbs;
> -	char *p;
>  	u_char *q;
> -	u_char __user *up;
>  	int sz, fnw_sz;
>  	int delta;
>  	char *first_free, *fj, *fnw;
> @@ -2022,23 +2020,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  	i = kbs->kb_func;
>  
>  	switch (cmd) {
> -	case KDGKBSENT:
> -		sz = sizeof(kbs->kb_string) - 1; /* sz should have been
> -						  a struct member */
> -		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);
> -		return ((p && *p) ? -EOVERFLOW : 0);
> +	case KDGKBSENT: {
> +		/* size should have been a struct member */
> +		unsigned char *from = func_table[i] ? : "";
> +
> +		ret = copy_to_user(user_kdgkb->kb_string, from,
> +				strlen(from) + 1) ? -EFAULT : 0;
> +
> +		goto reterr;
> +	}
>  	case KDSKBSENT:
>  		if (!perm) {
>  			ret = -EPERM;
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team