ACK: [PATCH 2/2] vt: keyboard, extend func_buf_lock to readers

[Thadeu Lima de Souza Cascardo]

On Mon, Feb 22, 2021 at 12:39:22PM -0700, Tim Gardner wrote:
> From: Jiri Slaby <jslaby at suse.cz>
> 
> 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. Use already allocated, but unused
> 'kbs->kb_string' for that purpose.
> 
> Note that the program above needs at least CAP_SYS_TTY_CONFIG.
> 
> This depends on the previous patch and on the func_buf_lock lock added
> in commit 46ca3f735f34 (tty/vt: fix write/write race in ioctl(KDSKBSENT)
> handler) in 5.2.
> 
> Likely fixes CVE-2020-25656.
> 
> Cc: <stable at vger.kernel.org>
> Reported-by: Minh Yuan <yuanmingbuaa at gmail.com>
> Signed-off-by: Jiri Slaby <jslaby at suse.cz>
> Link: https://lore.kernel.org/r/20201019085517.10176-2-jslaby@suse.cz
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> (cherry picked from commit 82e61c3909db51d91b9d3e2071557b6435018b80)
> CVE-2020-25656
> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>

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

> ---
>  drivers/tty/vt/keyboard.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index e64e3a7aa2f1..b6e78fdbfdff 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -742,8 +742,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);
>  }
> @@ -1990,7 +1995,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)
>  {
>  	struct kbsentry *kbs;
> @@ -2022,10 +2027,14 @@ 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] ? : "";
> +		ssize_t len = sizeof(user_kdgkb->kb_string);
> +
> +		spin_lock_irqsave(&func_buf_lock, flags);
> +		len = strlcpy(kbs->kb_string, func_table[i] ? : "", len);
> +		spin_unlock_irqrestore(&func_buf_lock, flags);
>  
> -		ret = copy_to_user(user_kdgkb->kb_string, from,
> -				strlen(from) + 1) ? -EFAULT : 0;
> +		ret = copy_to_user(user_kdgkb->kb_string, kbs->kb_string,
> +				len + 1) ? -EFAULT : 0;
>  
>  		goto reterr;
>  	}
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team