Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp7311896imu; Wed, 14 Nov 2018 15:22:03 -0800 (PST) X-Google-Smtp-Source: AJdET5fPGzpjJKMzZjU2gJFBmU88rqkzn9Vc8sY5e0fWwJ4u1jOvYfkPKqrHlrswgPzDF2TCgkrU X-Received: by 2002:a62:2545:: with SMTP id l66-v6mr3969907pfl.207.1542237722948; Wed, 14 Nov 2018 15:22:02 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1542237722; cv=none; d=google.com; s=arc-20160816; b=0U8BUTYKcCg6K18IBba0cx+YhLFfzSTWTDjru2qh76vAAWffwoJ1kD8AO3AYyvbmw7 LyA5gmrX/RVq1YHQoLspPmEJjtvB5TLR2fJm0+f+WyhkWzP0VdOh+aL+KmB9JYOCEgcs vnTaw2C77M6SjxA3qqRHtqstt0KdZdg9bTGoTevjl7/VZgBMNOQj4EqQOtXJaBwU7DKP FN4G7/u3sKUS+02HRiYIzoXXhksCDBWvoGsdNPNDwqZD6JJITw57KyYLYpMEhaxRRU8h HQX8O4z9e4i2qipODRxLHUS2V81nL4LjLEWi5jglPvvqX/8Jo8ITZJFe1HEluoyXZBfa tJew== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=isuOJddTU+/EUoXIvjRigMvt+8ae+uIRq8fzkQnez88=; b=LU3m9Yy+2plCpEVRPdLAxivV7H3FYqlmTovspmXQMlTLdOXsZBP9d2Fsg6rO1BOdag mZvYbo4ifpCdRRXI24V6whvMrlYhgQ3fDusOKcTkkD020VfRGCVzuOEKwiDQiSZX10cz mNG43mQ1f7UfS3+ZoibzAlAnGUWcjen/tMhEKZqBM+mBjzw2Gk5r7Gn3UVMEWskr7Cx7 WJ1p13R1AmoomGyZSpJa+qIBNAUlQ8G8vrPNRND7DgAXbVCt7QiuJ4eYxieaecAKtxGY gQOJD2It+j9rKmP91igNgqg1xaY7tH0oZ7syv2JODzV9F5UOEb+L+g6UD85n1yAyzkx4 HBHA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=pTti4wn3; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id i13si25173616pgc.41.2018.11.14.15.21.48; Wed, 14 Nov 2018 15:22:02 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=pTti4wn3; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727325AbeKOJZp (ORCPT + 99 others); Thu, 15 Nov 2018 04:25:45 -0500 Received: from mail-lj1-f193.google.com ([209.85.208.193]:34129 "EHLO mail-lj1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726371AbeKOJZp (ORCPT ); Thu, 15 Nov 2018 04:25:45 -0500 Received: by mail-lj1-f193.google.com with SMTP id u6-v6so15663574ljd.1 for ; Wed, 14 Nov 2018 15:20:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=isuOJddTU+/EUoXIvjRigMvt+8ae+uIRq8fzkQnez88=; b=pTti4wn3S9xD3u/+eGTjizU/rB/YJQFrGqIdtDjl65+ZVRsPpysWMB5mh+Wp7xTWOZ iXwJecq626WNM36H0lyzW7joCwu3Xy2wIH/I70bhO/kuuaUXuSbci6H0MvbRglFBd12Y rAY1nL8kd87Z+GNuhWjU2CGAb1kI0j6rEdqWdiYl+3FhK6lFCF1Okn/Vr/Co+DNxKx9q Cm43Maak7QRxc3+glA884cyQFHPMR9jhIA5e6Xwbivq8FEM1MTGmb+cekGrgo4az6ZQ/ 40DTkE9wjC+Su9N0dOTYLiHV+Kw32dCImeNrUQpX+pvKGA9Qm2qOJM8chI2xOYKsVk38 12mw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=isuOJddTU+/EUoXIvjRigMvt+8ae+uIRq8fzkQnez88=; b=hWbD7KGtL52ICe6eoNyccqgAz6ka7VfGgNlZmoHgs6jMv+xksUy9ikqAiKzCs+TxAK 2iHE8LVnB09HKZsYR95pTRDKyad2CFFAp1eQwmRBQ5nIIAWI53p/r1EGPlY9+Kg+w/RI MN5cFIZ+2ngPSnYG5qLwLkSbJ/p0/LVE3GEGVXZz06fTf5lBLVYSwKWI76F9dPiWQefH iMVri6cwRGfp3P4YBv0IBUuRZE7RZ1AHRlTt65y09tXjV9qXb+A4m9IKoPoulmw4BIvX /tzwHXIcAklUKyBAqIWtAj5+JetXsyouXEfo5qY+zGTI2TPOZQkVdg/mxr5ZmweJmxXy aMNQ== X-Gm-Message-State: AGRZ1gKOF+nl/ogVWpMpcjt7kpQyvvfnhRYb6xSOWsF2g8icftZui0i2 o66I4dB7qtJMtchg3N8tfGKgfMn1nEYxbgosiaayKA== X-Received: by 2002:a2e:9983:: with SMTP id w3-v6mr2464720lji.133.1542237622416; Wed, 14 Nov 2018 15:20:22 -0800 (PST) MIME-Version: 1.0 References: <20181114215509.163600-1-ebiggers@kernel.org> <20181114230046.GC87768@gmail.com> In-Reply-To: <20181114230046.GC87768@gmail.com> From: Dmitry Torokhov Date: Wed, 14 Nov 2018 15:20:11 -0800 Message-ID: Subject: Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges To: ebiggers@kernel.org Cc: jannh@google.com, dh.herrmann@googlemail.com, Jiri Kosina , Benjamin Tissoires , "open list:HID CORE LAYER" , lkml , syzkaller-bugs@googlegroups.com, Dmitry Vyukov , syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com, stable@vger.kernel.org, luto@kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers wrote: > > Hi Dmitry, > > On Wed, Nov 14, 2018 at 02:28:56PM -0800, 'Dmitry Torokhov' via syzkaller-bugs wrote: > > On Wed, Nov 14, 2018 at 2:05 PM Jann Horn wrote: > > > > > > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers wrote: > > > > > > > > From: Eric Biggers > > > > > > > > When a UHID_CREATE command is written to the uhid char device, a > > > > copy_from_user() is done from a user pointer embedded in the command. > > > > When the address limit is KERNEL_DS, e.g. as is the case during > > > > sys_sendfile(), this can read from kernel memory. Alternatively, > > > > information can be leaked from a setuid binary that is tricked to write > > > > to the file descriptor. Therefore, forbid UHID_CREATE in these cases. > > > > > > > > No other commands in uhid_char_write() are affected by this bug and > > > > UHID_CREATE is marked as "obsolete", so apply the restriction to > > > > UHID_CREATE only rather than to uhid_char_write() entirely. > > > > > > > > Thanks to Dmitry Vyukov for adding uhid definitions to syzkaller and to > > > > Jann Horn for commit 9da3f2b740544 ("x86/fault: BUG() when uaccess > > > > helpers fault on kernel addresses"), allowing this bug to be found. > > > > > > > > Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com > > > > Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events") > > > > Cc: # v3.6+ > > > > Cc: Jann Horn > > > > Cc: Andy Lutomirski > > > > Signed-off-by: Eric Biggers > > > > > > Reviewed-by: Jann Horn > > > > > > > --- > > > > drivers/hid/uhid.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c > > > > index 3c55073136064..051639c09f728 100644 > > > > --- a/drivers/hid/uhid.c > > > > +++ b/drivers/hid/uhid.c > > > > @@ -12,6 +12,7 @@ > > > > > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -722,6 +723,17 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer, > > > > > > > > switch (uhid->input_buf.type) { > > > > case UHID_CREATE: > > > > + /* > > > > + * 'struct uhid_create_req' contains a __user pointer which is > > > > + * copied from, so it's unsafe to allow this with elevated > > > > + * privileges (e.g. from a setuid binary) or via kernel_write(). > > > > + */ > > > > uhid is a privileged interface so we would go from root to less > > privileged (if at all). If non-privileged process can open uhid it can > > construct virtual keyboard and inject whatever keystrokes it wants. > > > > Also, instead of disallowing access, can we ensure that we switch back > > to USER_DS before trying to load data from the user pointer? > > > > Actually uhid doesn't have any capability checks, so it's up to userspace to > assign permissions to the device node. Yes. There are quite a few such instances where kernel does not bother implementing superfluous checks and instead relies on userspace to provide sane environment. IIRC nobody in the kernel enforces root filesystem not be accessible to ordinary users, it is done with generic permission checks. > I think it's best not to make > assumptions about how the interface will be used and to be consistent with how > other ->write() methods in the kernel handle the misfeature where a __user > pointer in the write() or read() payload is dereferenced. I can see that you might want to check credentials, etc, if interface can be accessed by unprivileged process, however is it a big no no for uhid/userio/uinput devices. > Temporarily switching > to USER_DS would only avoid one of the two problems. So because of the above there is only one problem. If your system opened access to uhid to random processes you have much bigger problems than exposing some data from a suid binary. You can spam "rm -rf .; rm -rf /" though uhid if there is interactive session somewhere. > > Do you think the proposed restrictions would actually break anything? It would break if someone uses UHID_CREATE with sendpage. I do not know if anyone does. If we were certain there are no users we'd simply removed UHID_CREATE altogether. > > - Eric > > > > > + if (file->f_cred != current_cred() || uaccess_kernel()) { > > > > + pr_err_once("UHID_CREATE from different security context by process %d (%s), this is not allowed.\n", > > > > + task_tgid_vnr(current), current->comm); > > > > + ret = -EACCES; > > > > + goto unlock; > > > > + } > > > > ret = uhid_dev_create(uhid, &uhid->input_buf); > > > > break; > > > > case UHID_CREATE2: > > > > -- Thanks. -- Dmitry