From patchwork Mon Feb 22 19:39:21 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Gardner X-Patchwork-Id: 1443277 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Dksvd0jhqz9sW2; Tue, 23 Feb 2021 06:39:40 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1lEH3S-0003Lu-1l; Mon, 22 Feb 2021 19:39:38 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lEH3O-0003KV-2T for kernel-team@lists.ubuntu.com; Mon, 22 Feb 2021 19:39:34 +0000 Received: from mail-pl1-f197.google.com ([209.85.214.197]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lEH3N-0000Mw-LY for kernel-team@lists.ubuntu.com; Mon, 22 Feb 2021 19:39:33 +0000 Received: by mail-pl1-f197.google.com with SMTP id k13so3786095plc.3 for ; Mon, 22 Feb 2021 11:39:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=OvjAWyfyMeRuUH8RL2dQNdcxQ8i95vp3aB3Wp6e5e8o=; b=bM1tZBkzGd4g3x5BvtE0h4ea619wI1hEw25g+81mXRieT9ijk9Bg+HEjMVkMe3vxk8 /Ck/ATC/mKRSglTurec/bdqm55Y5pnKTtLBuNgOZCvkNU1KoZwViOTqzf8qd7zFN9OZj Z7up1tDrj4iqEkcpTWvTIbkTh/FV0lYbAAjnQpPmtRfb9vOjGvv5ytPbQ4OSqn/3vMf2 uJOGOJehtmgAQ4yhHFAibSukAy4D3nGieeVPbBRo30CEjmsXiK3cg6EfPzW52oq1aF21 olPwNimt6pwooZwiGuR7WVfysdkKsMxm76pCFBcOVzPiY7Dv9OyMLviJiHYVmRJkdoNw JphQ== X-Gm-Message-State: AOAM533UcgSrePQhZ3sL6kuAUL5IqMc0vSVcdp5sKEI4rgGeBxyZAiBK ykhBOIuk2W4WrnUpGfmKAtAbZuNFfyh1asyxJV3Y/gWPIx+++UVQEt8RFDPqWoVn/MZak6SwGPZ 9PeLP2n5LEDa963lD2sy+SwfYfM5WdO+knf12Q5eJpA== X-Received: by 2002:a63:5301:: with SMTP id h1mr21786110pgb.180.1614022771996; Mon, 22 Feb 2021 11:39:31 -0800 (PST) X-Google-Smtp-Source: ABdhPJxerN2ZogglhGZ5jkXSseuWMD0Mn63cnMZqZoyUEOQ4l7ITAWXNVa1CMAPMAIQ7WwOBIvGeeA== X-Received: by 2002:a63:5301:: with SMTP id h1mr21786094pgb.180.1614022771711; Mon, 22 Feb 2021 11:39:31 -0800 (PST) Received: from localhost.localdomain ([69.163.84.166]) by smtp.gmail.com with ESMTPSA id c12sm206940pjq.48.2021.02.22.11.39.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Feb 2021 11:39:31 -0800 (PST) From: Tim Gardner To: kernel-team@lists.ubuntu.com Subject: [PATCH 1/2] vt: keyboard, simplify vt_kdgkbsent Date: Mon, 22 Feb 2021 12:39:21 -0700 Message-Id: <20210222193922.11502-2-tim.gardner@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20210222193922.11502-1-tim.gardner@canonical.com> References: <20210222193922.11502-1-tim.gardner@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Jiri Slaby 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: Signed-off-by: Jiri Slaby Link: https://lore.kernel.org/r/20201019085517.10176-1-jslaby@suse.cz Signed-off-by: Greg Kroah-Hartman (cherry picked from commit 6ca03f90527e499dd5e32d6522909e2ad390896b) Signed-off-by: Tim Gardner Acked-by: Thadeu Lima de Souza Cascardo --- 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; From patchwork Mon Feb 22 19:39:22 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Gardner X-Patchwork-Id: 1443275 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Dksvc1rVGz9sVR; Tue, 23 Feb 2021 06:39:40 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1lEH3Q-0003L2-Rj; Mon, 22 Feb 2021 19:39:36 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lEH3P-0003Ko-3X for kernel-team@lists.ubuntu.com; Mon, 22 Feb 2021 19:39:35 +0000 Received: from mail-pg1-f197.google.com ([209.85.215.197]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1lEH3O-0000N0-MV for kernel-team@lists.ubuntu.com; Mon, 22 Feb 2021 19:39:34 +0000 Received: by mail-pg1-f197.google.com with SMTP id f7so8641462pgp.19 for ; Mon, 22 Feb 2021 11:39:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=TMoQNj5G4hhEizEHsO9/5n9WouzVYG1zFKQPYhUJnTc=; b=UE9jyUekJgM7BI53WP5dKZ9m0kOy3JUUzY0m1fWXWzPlMnbWw0FycJHRcsbgKdhGRV DSHmTkTseq+PxtTLjzhxeM9x+l66CywGKt8+zbNF+7wk1V81ZUKL4c0vCr+7lHr1SacG 7DB6xmhuspFsOu8RCTWcyHSqg1RPAc/6loeg0fvEZixs4gRV4b+j1cQZESkvB0yWlyVJ 6tqAzE7/MQhpz075RKnvoGkTdif3RBymSUmZwPIVULhDFW2KybM/y8iSWd9eOvG13hHi EOWTHyqgaiEmp7JHIAkWhFs8k+UNPun//oAsLazwg4mR5AFRt0pD2Iy/aSH98iB2O/h/ QSqQ== X-Gm-Message-State: AOAM531Q/vBEM0D69qEbeO2g1VRrVh9FWFWzYBQjEn+PJDoDJv7jpw7+ zzvfcmI+hMJj/YZzQbb1t+NceLadlXrPJxGW4ptBxIlq17pAechQQUhEwrVZgh5QznXrWDncXL9 d3Pd5zWWrSekW8V99kzGu5QBmGhUqzJ88mUR67a+23g== X-Received: by 2002:a63:4246:: with SMTP id p67mr20757253pga.414.1614022773047; Mon, 22 Feb 2021 11:39:33 -0800 (PST) X-Google-Smtp-Source: ABdhPJymqTQ/cUcpI59LEf2rABnjxv9icLClEqdMgOyzWf4GA9GWqblBPkWkRqnLF+Hqa57v+vwb+Q== X-Received: by 2002:a63:4246:: with SMTP id p67mr20757244pga.414.1614022772851; Mon, 22 Feb 2021 11:39:32 -0800 (PST) Received: from localhost.localdomain ([69.163.84.166]) by smtp.gmail.com with ESMTPSA id c12sm206940pjq.48.2021.02.22.11.39.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 22 Feb 2021 11:39:32 -0800 (PST) From: Tim Gardner To: kernel-team@lists.ubuntu.com Subject: [PATCH 2/2] vt: keyboard, extend func_buf_lock to readers Date: Mon, 22 Feb 2021 12:39:22 -0700 Message-Id: <20210222193922.11502-3-tim.gardner@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20210222193922.11502-1-tim.gardner@canonical.com> References: <20210222193922.11502-1-tim.gardner@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Jiri Slaby 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: Reported-by: Minh Yuan Signed-off-by: Jiri Slaby Link: https://lore.kernel.org/r/20201019085517.10176-2-jslaby@suse.cz Signed-off-by: Greg Kroah-Hartman (cherry picked from commit 82e61c3909db51d91b9d3e2071557b6435018b80) CVE-2020-25656 Signed-off-by: Tim Gardner Acked-by: Thadeu Lima de Souza Cascardo --- 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; }