* Re: BUG: GPF in non-whitelisted uaccess (non-canonical address?)
From: Eric Biggers @ 2018-11-14 17:14 UTC (permalink / raw)
To: David Herrmann
Cc: Dmitry Vyukov, Dmitry Torokhov, syzbot+72473edc9bf4eb1c6556,
Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel, syzkaller-bugs
In-Reply-To: <CACT4Y+b8801QqLe4axBct-cZ2a-jr2S2=MSRhg7rV20bPvUHxQ@mail.gmail.com>
On Wed, Nov 14, 2018 at 08:52:46AM -0800, 'Dmitry Vyukov' via syzkaller-bugs wrote:
> On Wed, Nov 14, 2018 at 4:20 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> > Hey
> >
> > On Wed, Nov 14, 2018 at 1:25 AM syzbot
> > <syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com> wrote:
> >> syzbot has found a reproducer for the following crash on:
> >>
> >> HEAD commit: ccda4af0f4b9 Linux 4.20-rc2
> >> git tree: upstream
> >> console output: https://syzkaller.appspot.com/x/log.txt?x=13b4e77b400000
> >> kernel config: https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5
> >> dashboard link: https://syzkaller.appspot.com/bug?extid=72473edc9bf4eb1c6556
> >> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
> >> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1646a225400000
> >> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=108a6533400000
> >>
> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >> Reported-by: syzbot+72473edc9bf4eb1c6556@syzkaller.appspotmail.com
> >>
> > [...]
> >> BUG: GPF in non-whitelisted uaccess (non-canonical address?)
> >
> > This uses sendpage(2) to feed data from a file into a uhid chardev.
> > The default behavior of the kernel is to create a temporary pipe, then
> > splice from the file into the pipe, and then splice again from the
> > pipe into uhid.
> >
> > The kernel provides default implementations for splicing between files
> > and any other file. The default implementation of `.splice_write()`
> > uses kmap() to map the page from the pipe and then uses the
> > __kernel_write() (which uses .f_op->write()) to push the data into the
> > target file. The problem is, __kernel_write() sets the address-space
> > to KERNEL_DS `set_fs(get_ds())`, thus granting the UHID request access
> > to kernel memory.
> >
> > I see several ways to fix that, the most simple solution is to simply
> > prevent splice/sendpage on uhid (by setting f_op.splice_write to a
> > dummy). Alternatively, we can implement a proper splice helper that
> > takes the page directly, rather than through the __kernel_write()
> > default implementation.
>
> also +dtor for uhid
>
Well, the problem is that uhid_char_write() reads from a user pointer embedded
in the write() payload. (Which really is abusing write(), but I assume it
cannot be changed at this point...) Thus it's unsafe to be called under
KERNEL_DS. So it needs:
if (uaccess_kernel())
return -EACCES;
See sg_check_file_access(), called from sg_read() and sg_write(), for another
example of this in the kernel.
- Eric
^ permalink raw reply
* [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
From: Eric Biggers @ 2018-11-14 18:02 UTC (permalink / raw)
To: David Herrmann, Jiri Kosina, Benjamin Tissoires, linux-input
Cc: linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
syzbot+72473edc9bf4eb1c6556, stable, Jann Horn
In-Reply-To: <20181114171447.GA87768@gmail.com>
From: Eric Biggers <ebiggers@google.com>
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
sendfile(), this can read from kernel memory. Therefore, UHID_CREATE
must not be allowed in this case.
For consistency and to make sure all current and future uhid commands
are covered, apply the restriction to uhid_char_write() as a whole
rather than to UHID_CREATE specifically.
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: <stable@vger.kernel.org> # v3.6+
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
drivers/hid/uhid.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 3c55073136064..e94c5e248b56e 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
int ret;
size_t len;
+ if (uaccess_kernel()) { /* payload may contain a __user pointer */
+ pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
+ __func__, task_tgid_vnr(current), current->comm);
+ return -EACCES;
+ }
+
/* we need at least the "type" member of uhid_event */
if (count < sizeof(__u32))
return -EINVAL;
--
2.19.1.930.g4563a0d9d0-goog
^ permalink raw reply related
* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
From: Dmitry Torokhov @ 2018-11-14 18:14 UTC (permalink / raw)
To: ebiggers
Cc: dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, jannh
In-Reply-To: <20181114180217.195917-1-ebiggers@kernel.org>
On Wed, Nov 14, 2018 at 10:03 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> 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
> sendfile(), this can read from kernel memory. Therefore, UHID_CREATE
> must not be allowed in this case.
Hmm, instead of disallowing access, can we switch back to USER_DS
before trying to use the user pointer?
>
>
> For consistency and to make sure all current and future uhid commands
> are covered, apply the restriction to uhid_char_write() as a whole
> rather than to UHID_CREATE specifically.
>
> 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: <stable@vger.kernel.org> # v3.6+
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/hid/uhid.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 3c55073136064..e94c5e248b56e 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
> int ret;
> size_t len;
>
> + if (uaccess_kernel()) { /* payload may contain a __user pointer */
> + pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
> + __func__, task_tgid_vnr(current), current->comm);
> + return -EACCES;
> + }
> +
> /* we need at least the "type" member of uhid_event */
> if (count < sizeof(__u32))
> return -EINVAL;
> --
> 2.19.1.930.g4563a0d9d0-goog
>
^ permalink raw reply
* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
From: Jann Horn @ 2018-11-14 18:18 UTC (permalink / raw)
To: ebiggers
Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski
In-Reply-To: <20181114180217.195917-1-ebiggers@kernel.org>
+cc Andy
On Wed, Nov 14, 2018 at 7:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
> 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
> sendfile(), this can read from kernel memory. Therefore, UHID_CREATE
> must not be allowed in this case.
>
> For consistency and to make sure all current and future uhid commands
> are covered, apply the restriction to uhid_char_write() as a whole
> rather than to UHID_CREATE specifically.
>
> 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
Wheeeee, it found something! :)
> Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")
> Cc: <stable@vger.kernel.org> # v3.6+
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> drivers/hid/uhid.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 3c55073136064..e94c5e248b56e 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
> int ret;
> size_t len;
>
> + if (uaccess_kernel()) { /* payload may contain a __user pointer */
> + pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
> + __func__, task_tgid_vnr(current), current->comm);
> + return -EACCES;
> + }
If this file can conceivably be opened by a process that doesn't have
root privileges, this check should be something along the lines of
ib_safe_file_access() or sg_check_file_access().
Checking for uaccess_kernel() prevents the symptom that syzkaller
notices - a user being able to cause a kernel memory access -, but it
doesn't deal with the case where a user opens a file descriptor to
this device and tricks a more privileged process into writing into it
(e.g. by passing it to a suid binary as stdout or stderr).
Looking closer, I wonder whether this kind of behavior is limited to
the UHID_CREATE request, which has a comment on it saying "/*
Obsolete! Use UHID_CREATE2. */". If we could keep this kind of ugly
kludge away from the code paths you're supposed to be using, that
would be nice...
^ permalink raw reply
* Re: [PATCH] HID: uhid: prevent uhid_char_write() under KERNEL_DS
From: Eric Biggers @ 2018-11-14 21:54 UTC (permalink / raw)
To: Jann Horn
Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski
In-Reply-To: <CAG48ez3bPkh+DMPwiebM+r4ozX2CiVY=9=WMBP_xm1qVaSN4sQ@mail.gmail.com>
On Wed, Nov 14, 2018 at 07:18:39PM +0100, 'Jann Horn' via syzkaller-bugs wrote:
> +cc Andy
>
> On Wed, Nov 14, 2018 at 7:03 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > 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
> > sendfile(), this can read from kernel memory. Therefore, UHID_CREATE
> > must not be allowed in this case.
> >
> > For consistency and to make sure all current and future uhid commands
> > are covered, apply the restriction to uhid_char_write() as a whole
> > rather than to UHID_CREATE specifically.
> >
> > 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
>
> Wheeeee, it found something! :)
>
> > Fixes: d365c6cfd337 ("HID: uhid: add UHID_CREATE and UHID_DESTROY events")
> > Cc: <stable@vger.kernel.org> # v3.6+
> > Cc: Jann Horn <jannh@google.com>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> > drivers/hid/uhid.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> > index 3c55073136064..e94c5e248b56e 100644
> > --- a/drivers/hid/uhid.c
> > +++ b/drivers/hid/uhid.c
> > @@ -705,6 +705,12 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
> > int ret;
> > size_t len;
> >
> > + if (uaccess_kernel()) { /* payload may contain a __user pointer */
> > + pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
> > + __func__, task_tgid_vnr(current), current->comm);
> > + return -EACCES;
> > + }
>
> If this file can conceivably be opened by a process that doesn't have
> root privileges, this check should be something along the lines of
> ib_safe_file_access() or sg_check_file_access().
>
> Checking for uaccess_kernel() prevents the symptom that syzkaller
> notices - a user being able to cause a kernel memory access -, but it
> doesn't deal with the case where a user opens a file descriptor to
> this device and tricks a more privileged process into writing into it
> (e.g. by passing it to a suid binary as stdout or stderr).
>
Yep, I'll do that.
> Looking closer, I wonder whether this kind of behavior is limited to
> the UHID_CREATE request, which has a comment on it saying "/*
> Obsolete! Use UHID_CREATE2. */". If we could keep this kind of ugly
> kludge away from the code paths you're supposed to be using, that
> would be nice...
>
I wanted to be careful, but yes AFAICS it can be limited to UHID_CREATE only,
so I'll do that instead.
- Eric
^ permalink raw reply
* [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Eric Biggers @ 2018-11-14 21:55 UTC (permalink / raw)
To: David Herrmann, Jiri Kosina, Benjamin Tissoires, linux-input
Cc: linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
syzbot+72473edc9bf4eb1c6556, stable, Jann Horn, Andy Lutomirski
In-Reply-To: <CAG48ez3bPkh+DMPwiebM+r4ozX2CiVY=9=WMBP_xm1qVaSN4sQ@mail.gmail.com>
From: Eric Biggers <ebiggers@google.com>
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: <stable@vger.kernel.org> # v3.6+
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
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 <linux/atomic.h>
#include <linux/compat.h>
+#include <linux/cred.h>
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/hid.h>
@@ -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().
+ */
+ 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:
--
2.19.1.930.g4563a0d9d0-goog
^ permalink raw reply related
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Jann Horn @ 2018-11-14 22:04 UTC (permalink / raw)
To: ebiggers
Cc: dh.herrmann, Jiri Kosina, benjamin.tissoires, linux-input,
kernel list, syzkaller-bugs, Dmitry Vyukov, dtor,
syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski
In-Reply-To: <20181114215509.163600-1-ebiggers@kernel.org>
On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> From: Eric Biggers <ebiggers@google.com>
>
> 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: <stable@vger.kernel.org> # v3.6+
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
Reviewed-by: Jann Horn <jannh@google.com>
> ---
> 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 <linux/atomic.h>
> #include <linux/compat.h>
> +#include <linux/cred.h>
> #include <linux/device.h>
> #include <linux/fs.h>
> #include <linux/hid.h>
> @@ -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().
> + */
> + 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:
> --
> 2.19.1.930.g4563a0d9d0-goog
>
^ permalink raw reply
* [PATCH AUTOSEL 4.18 02/59] HID: i2c-hid: Add a small delay after sleep command for Raydium touchpanel
From: Sasha Levin @ 2018-11-14 22:22 UTC (permalink / raw)
To: stable, linux-kernel; +Cc: Kai-Heng Feng, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20181114222335.99339-1-sashal@kernel.org>
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
[ Upstream commit 00b790ea545b6ef30221adef6e9c3707e03b82b5 ]
Raydium touchpanel (2386:4B33) sometimes does not work in desktop session
although it works in display manager.
During user logging, the display manager exits, close the HID device,
then the device gets runtime suspended and powered off. The desktop
session begins shortly after, opens the HID device, then the device gets
runtime resumed and powered on.
If the trasition from display manager to desktop sesesion is fast, the
touchpanel cannot switch from powered off to powered on in short
timeframe. So add a small delay to workaround the issue.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-ids.h | 3 +++
drivers/hid/i2c-hid/i2c-hid.c | 19 +++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ae5b72269e27..7e61af2f0976 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -919,6 +919,9 @@
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3003 0x3003
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
+#define I2C_VENDOR_ID_RAYDIUM 0x2386
+#define I2C_PRODUCT_ID_RAYDIUM_4B33 0x4b33
+
#define USB_VENDOR_ID_RAZER 0x1532
#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d17cf6e323b2..d835fd02da4f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -48,6 +48,7 @@
#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV BIT(0)
#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
#define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
+#define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
/* flags */
#define I2C_HID_STARTED 0
@@ -157,6 +158,8 @@ struct i2c_hid {
bool irq_wake_enabled;
struct mutex reset_lock;
+
+ unsigned long sleep_delay;
};
static const struct i2c_hid_quirks {
@@ -171,6 +174,8 @@ static const struct i2c_hid_quirks {
{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET |
I2C_HID_QUIRK_NO_RUNTIME_PM },
+ { I2C_VENDOR_ID_RAYDIUM, I2C_PRODUCT_ID_RAYDIUM_4B33,
+ I2C_HID_QUIRK_DELAY_AFTER_SLEEP },
{ 0, 0 }
};
@@ -386,6 +391,7 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
{
struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret;
+ unsigned long now, delay;
i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -403,9 +409,22 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
goto set_pwr_exit;
}
+ if (ihid->quirks & I2C_HID_QUIRK_DELAY_AFTER_SLEEP &&
+ power_state == I2C_HID_PWR_ON) {
+ now = jiffies;
+ if (time_after(ihid->sleep_delay, now)) {
+ delay = jiffies_to_usecs(ihid->sleep_delay - now);
+ usleep_range(delay, delay + 1);
+ }
+ }
+
ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
0, NULL, 0, NULL, 0);
+ if (ihid->quirks & I2C_HID_QUIRK_DELAY_AFTER_SLEEP &&
+ power_state == I2C_HID_PWR_SLEEP)
+ ihid->sleep_delay = jiffies + msecs_to_jiffies(20);
+
if (ret)
dev_err(&client->dev, "failed to change power setting.\n");
--
2.17.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.18 03/59] Revert "HID: add NOGET quirk for Eaton Ellipse MAX UPS"
From: Sasha Levin @ 2018-11-14 22:22 UTC (permalink / raw)
To: stable, linux-kernel
Cc: Benjamin Tissoires, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20181114222335.99339-1-sashal@kernel.org>
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
[ Upstream commit 6298944d8f57f40ee2a3e6dcea1253e78d7a9969 ]
This reverts commit 67ddbb3e6568fb1820b2cc45b00c50702b114801.
67ddbb3e656 ("HID: add NOGET quirk for Eaton Ellipse MAX UPS") was reported
by Laurent Bigonville. It turns out that a later model Laurent got
doesn't need the quirk after all.
My take is that Eaton upgraded their firmwares, so we don't need it
anymore.
The old model was from 2012, so better make sure the new line works
properly by removing the quirk. This allows upower to actually fetch
the current data.
Reported-by: Laurent Bigonville <bigon@bigon.be>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-quirks.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 249d49b6b16c..caafbea94882 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -106,7 +106,6 @@ static const struct hid_device_id hid_quirks[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C05A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C06A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_GAMEPADBLOCK), HID_QUIRK_MULTI_INPUT },
- { HID_USB_DEVICE(USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS), HID_QUIRK_NOGET },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2), HID_QUIRK_NO_INIT_REPORTS },
--
2.17.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.18 04/59] HID: alps: allow incoming reports when only the trackstick is opened
From: Sasha Levin @ 2018-11-14 22:22 UTC (permalink / raw)
To: stable, linux-kernel
Cc: Benjamin Tissoires, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20181114222335.99339-1-sashal@kernel.org>
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
[ Upstream commit 7dd8db68949a7acc5bd528ee0ecb8f8720f49921 ]
If userspace only reads the trackstick node, and no one is listening to
the touchpad nor the hidraw node then, the device is not powered on.
Add open/close callbacks to allow users to disable the touchpad in Gnome
while keeping the trackstick active.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1559632
Link: https://gitlab.gnome.org/GNOME/mutter/issues/128
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-alps.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
index aec253b44156..3cd7229b6e54 100644
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -660,6 +660,20 @@ static int T4_init(struct hid_device *hdev, struct alps_dev *pri_data)
return ret;
}
+static int alps_sp_open(struct input_dev *dev)
+{
+ struct hid_device *hid = input_get_drvdata(dev);
+
+ return hid_hw_open(hid);
+}
+
+static void alps_sp_close(struct input_dev *dev)
+{
+ struct hid_device *hid = input_get_drvdata(dev);
+
+ hid_hw_close(hid);
+}
+
static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
{
struct alps_dev *data = hid_get_drvdata(hdev);
@@ -733,6 +747,10 @@ static int alps_input_configured(struct hid_device *hdev, struct hid_input *hi)
input2->id.version = input->id.version;
input2->dev.parent = input->dev.parent;
+ input_set_drvdata(input2, hdev);
+ input2->open = alps_sp_open;
+ input2->close = alps_sp_close;
+
__set_bit(EV_KEY, input2->evbit);
data->sp_btn_cnt = (data->sp_btn_info & 0x0F);
for (i = 0; i < data->sp_btn_cnt; i++)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Dmitry Torokhov @ 2018-11-14 22:28 UTC (permalink / raw)
To: jannh
Cc: ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, luto
In-Reply-To: <CAG48ez02ZYZOKRXuZ9yYUijZnaaw=G7-KYu1rB7C99+B45VqjQ@mail.gmail.com>
On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > From: Eric Biggers <ebiggers@google.com>
> >
> > 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: <stable@vger.kernel.org> # v3.6+
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> Reviewed-by: Jann Horn <jannh@google.com>
>
> > ---
> > 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 <linux/atomic.h>
> > #include <linux/compat.h>
> > +#include <linux/cred.h>
> > #include <linux/device.h>
> > #include <linux/fs.h>
> > #include <linux/hid.h>
> > @@ -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?
> > + 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:
> > --
> > 2.19.1.930.g4563a0d9d0-goog
> >
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Jann Horn @ 2018-11-14 22:37 UTC (permalink / raw)
To: dtor
Cc: ebiggers, dh.herrmann, Jiri Kosina, benjamin.tissoires,
linux-input, kernel list, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, Andy Lutomirski
In-Reply-To: <CAE_wzQ_GqjwkhUjoPs3h-s7KSVe8KoH-uu-4mf672JN0X89d6g@mail.gmail.com>
On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov <dtor@google.com> wrote:
> On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
> > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > 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.
[...]
> > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
[...]
> > > @@ -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?
Does that even make sense? You are using some deprecated legacy
interface; you interact with it by splicing a request from something
like a file or a pipe into the uhid device; but the request you're
splicing through contains a pointer into userspace memory? Do you know
of anyone who is actually doing that? If not, anyone who does want to
do this for some reason in the future can just go use UHID_CREATE2
instead.
> > > + 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:
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Dmitry Torokhov @ 2018-11-14 22:46 UTC (permalink / raw)
To: jannh
Cc: ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, luto
In-Reply-To: <CAG48ez1GH5=d2LcSVA=+AoW6zB=BELr1QL34f3jrA8ip_6WMtQ@mail.gmail.com>
On Wed, Nov 14, 2018 at 2:38 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov <dtor@google.com> wrote:
> > On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
> > > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > 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.
> [...]
> > > > diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> [...]
> > > > @@ -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?
>
> Does that even make sense? You are using some deprecated legacy
> interface; you interact with it by splicing a request from something
> like a file or a pipe into the uhid device; but the request you're
> splicing through contains a pointer into userspace memory? Do you know
> of anyone who is actually doing that? If not, anyone who does want to
> do this for some reason in the future can just go use UHID_CREATE2
> instead.
I do not know if anyone is still using UHID_CREATE with sendpage and
neither do you really. It is all about not breaking userspace without
good reason and here ensuring that we switch to USER_DS and then back
to whatever it was does not seem too hard.
>
> > > > + 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:
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Eric Biggers @ 2018-11-14 23:00 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: jannh, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, luto
In-Reply-To: <CAE_wzQ_GqjwkhUjoPs3h-s7KSVe8KoH-uu-4mf672JN0X89d6g@mail.gmail.com>
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 <jannh@google.com> wrote:
> >
> > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > From: Eric Biggers <ebiggers@google.com>
> > >
> > > 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: <stable@vger.kernel.org> # v3.6+
> > > Cc: Jann Horn <jannh@google.com>
> > > Cc: Andy Lutomirski <luto@kernel.org>
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> >
> > Reviewed-by: Jann Horn <jannh@google.com>
> >
> > > ---
> > > 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 <linux/atomic.h>
> > > #include <linux/compat.h>
> > > +#include <linux/cred.h>
> > > #include <linux/device.h>
> > > #include <linux/fs.h>
> > > #include <linux/hid.h>
> > > @@ -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. 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. Temporarily switching
to USER_DS would only avoid one of the two problems.
Do you think the proposed restrictions would actually break anything?
- 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:
> > > --
^ permalink raw reply
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()"
From: Kees Cook @ 2018-11-14 23:09 UTC (permalink / raw)
To: David Herrmann
Cc: Laura Abbott, linux-input, LKML, Jiri Kosina, Benjamin Tissoires,
Xiongfeng Wang
In-Reply-To: <ea944fb3-0cd9-e6e6-5449-faab7c9603c1@redhat.com>
On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 11/14/18 5:16 AM, David Herrmann wrote:
>>
>> This reverts commit 336fd4f5f25157e9e8bd50e898a1bbcd99eaea46.
>>
>> Please note that `strlcpy()` does *NOT* do what you think it does.
>> strlcpy() *ALWAYS* reads the full input string, regardless of the
>> 'length' parameter. That is, if the input is not zero-terminated,
>> strlcpy() will *READ* beyond input boundaries. It does this, because it
>> always returns the size it *would* copy if the target was big enough,
>> not the truncated size it actually copied.
>>
>> The original code was perfectly fine. The hid device is
>> zero-initialized and the strncpy() functions copied up to n-1
>> characters. The result is always zero-terminated this way.
>>
>> This is the third time someone tried to replace strncpy with strlcpy in
>> this function, and gets it wrong. I now added a comment that should at
>> least make people reconsider.
>>
>
> Can we switch to strscpy instead? This will quiet gcc and avoid the
> issues with strlcpy.
Yes please: it looks like these strings are expected to be NUL
terminated, so strscpy() without the "- 1" and min() logic would be
the correct solution here. If @hid is already zero, then this would
just be:
strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
If they are NOT NUL terminated, then keep using strncpy() but mark the
fields in the struct with the __nonstring attribute.
-Kees
>
>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> drivers/hid/uhid.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> index fefedc0b4dc6..0dfdd0ac7120 100644
>> --- a/drivers/hid/uhid.c
>> +++ b/drivers/hid/uhid.c
>> @@ -496,12 +496,13 @@ static int uhid_dev_create2(struct uhid_device
>> *uhid,
>> goto err_free;
>> }
>> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name));
>> - strlcpy(hid->name, ev->u.create2.name, len);
>> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys));
>> - strlcpy(hid->phys, ev->u.create2.phys, len);
>> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq));
>> - strlcpy(hid->uniq, ev->u.create2.uniq, len);
>> + /* @hid is zero-initialized, strncpy() is correct, strlcpy() not
>> */
>> + len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
>> + strncpy(hid->name, ev->u.create2.name, len);
>> + len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
>> + strncpy(hid->phys, ev->u.create2.phys, len);
>> + len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
>> + strncpy(hid->uniq, ev->u.create2.uniq, len);
>> hid->ll_driver = &uhid_hid_driver;
>> hid->bus = ev->u.create2.bus;
>>
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Dmitry Torokhov @ 2018-11-14 23:20 UTC (permalink / raw)
To: ebiggers
Cc: jannh, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, luto
In-Reply-To: <20181114230046.GC87768@gmail.com>
On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers <ebiggers@kernel.org> 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 <jannh@google.com> wrote:
> > >
> > > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > From: Eric Biggers <ebiggers@google.com>
> > > >
> > > > 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: <stable@vger.kernel.org> # v3.6+
> > > > Cc: Jann Horn <jannh@google.com>
> > > > Cc: Andy Lutomirski <luto@kernel.org>
> > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > >
> > > Reviewed-by: Jann Horn <jannh@google.com>
> > >
> > > > ---
> > > > 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 <linux/atomic.h>
> > > > #include <linux/compat.h>
> > > > +#include <linux/cred.h>
> > > > #include <linux/device.h>
> > > > #include <linux/fs.h>
> > > > #include <linux/hid.h>
> > > > @@ -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
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Andy Lutomirski @ 2018-11-15 0:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: jannh, ebiggers, dh.herrmann, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, lkml, syzkaller-bugs, Dmitry Vyukov,
syzbot+72473edc9bf4eb1c6556, stable, luto
In-Reply-To: <CAE_wzQ91=FQ1HYYvbOi5R5VSXjJibLwYWsAPFtvUemPa9+Turg@mail.gmail.com>
> On Nov 14, 2018, at 2:46 PM, Dmitry Torokhov <dtor@google.com> wrote:
>
>> On Wed, Nov 14, 2018 at 2:38 PM Jann Horn <jannh@google.com> wrote:
>>
>>> On Wed, Nov 14, 2018 at 11:29 PM Dmitry Torokhov <dtor@google.com> wrote:
>>>> On Wed, Nov 14, 2018 at 2:05 PM Jann Horn <jannh@google.com> wrote:
>>>>> On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>>>>> 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.
>> [...]
>>>>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> [...]
>>>>> @@ -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?
>>
>> Does that even make sense? You are using some deprecated legacy
>> interface; you interact with it by splicing a request from something
>> like a file or a pipe into the uhid device; but the request you're
>> splicing through contains a pointer into userspace memory? Do you know
>> of anyone who is actually doing that? If not, anyone who does want to
>> do this for some reason in the future can just go use UHID_CREATE2
>> instead.
>
> I do not know if anyone is still using UHID_CREATE with sendpage and
> neither do you really. It is all about not breaking userspace without
> good reason and here ensuring that we switch to USER_DS and then back
> to whatever it was does not seem too hard.
It’s about not breaking userspace *except as needed for security fixes*. User pointers in a write() payload is a big no-no.
Also, that f_cred hack is only barely enough. This isn’t just about attacking suid things — this bug allows poking at the address space of an unsuspecting process. So, if a privileged program opens a uhid fd and is then tricked into writing untrusted data to the same fd (which is supposed to be safe), then we have a problem. Fortunately, identically privileged programs usually still don’t share a cred pointer unless they came from the right place.
The real right fix is to remove UHID_CREATE outright. This is terminally broken.
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Benjamin Tissoires @ 2018-11-15 8:14 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: ebiggers, jannh, dh.herrmann, Jiri Kosina,
open list:HID CORE LAYER, lkml, syzkaller-bugs, dvyukov,
syzbot+72473edc9bf4eb1c6556, 3.8+, luto
In-Reply-To: <CAE_wzQ_OS-BnRrXjKFhNBeRFRYsQkX2tNO3DEUQRFR7g7A7-2g@mail.gmail.com>
On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov <dtor@google.com> wrote:
>
> On Wed, Nov 14, 2018 at 3:00 PM Eric Biggers <ebiggers@kernel.org> 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 <jannh@google.com> wrote:
> > > >
> > > > On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > >
> > > > > From: Eric Biggers <ebiggers@google.com>
> > > > >
> > > > > 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: <stable@vger.kernel.org> # v3.6+
> > > > > Cc: Jann Horn <jannh@google.com>
> > > > > Cc: Andy Lutomirski <luto@kernel.org>
> > > > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > >
> > > > Reviewed-by: Jann Horn <jannh@google.com>
> > > >
> > > > > ---
> > > > > 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 <linux/atomic.h>
> > > > > #include <linux/compat.h>
> > > > > +#include <linux/cred.h>
> > > > > #include <linux/device.h>
> > > > > #include <linux/fs.h>
> > > > > #include <linux/hid.h>
> > > > > @@ -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.
Yep, any sane distribution would restrict the permissions of
uhid/userio/uinput to only be accessed by root. If that ever changes,
there is already an issue with the system and it was compromised
either by a terribly dizzy sysadmin.
>
> > 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.
AFAICT, there are 2 users of uhid:
- bluez for BLE devices (using HID over GATT)
- hid-replay for debugging.
There might be a few other users that are making some user space
driver out of opencv, but I wouldn't expect those to be really
widespread.
IIRC, bluez uses UHID_CREATE2 and hid-replay should also (or ought to
be, but this can be easily fixed as I maintain the code and I am the
almost sole user).
Regarding the sendpage fix, doesn't David's patch fixes the issue
already (https://patchwork.kernel.org/patch/10682549/).
I am fine applying whatever patch that fixes the security issues, as
long as it doesn't break bluez nor the hid-replay uses I have for
debugging and regression testing.
Cheers,
Benjamin
>
> >
> > - 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
^ permalink raw reply
* Re: [PATCH] Revert "HID: uhid: use strlcpy() instead of strncpy()"
From: David Herrmann @ 2018-11-15 11:55 UTC (permalink / raw)
To: Kees Cook
Cc: labbott, open list:HID CORE LAYER, linux-kernel, Jiri Kosina,
Benjamin Tissoires, xiongfeng.wang
In-Reply-To: <CAGXu5jJJGg5_crJ3v+AG5JZWVGLpDvZGr0UVyurzFxcv9Fwa5A@mail.gmail.com>
Hi
On Thu, Nov 15, 2018 at 12:09 AM Kees Cook <keescook@chromium.org> wrote:
> On Wed, Nov 14, 2018 at 9:40 AM, Laura Abbott <labbott@redhat.com> wrote:
[...]
> > Can we switch to strscpy instead? This will quiet gcc and avoid the
> > issues with strlcpy.
>
> Yes please: it looks like these strings are expected to be NUL
> terminated, so strscpy() without the "- 1" and min() logic would be
> the correct solution here.
"the correct solution"? To my knowledge the original code was correct
as well. Am I missing something?
> If @hid is already zero, then this would
> just be:
>
> strscpy(hid->name, ev->u.create2.name, sizeof(hid->name));
> strscpy(hid->phys, ev->u.create2.phys, sizeof(hid->phys));
> strscpy(hid->uniq, ev->u.create2.uniq, sizeof(hid->uniq));
>
> If they are NOT NUL terminated, then keep using strncpy() but mark the
> fields in the struct with the __nonstring attribute.
They are supposed to be NUL terminated, but for compatibility reasons
we allow them to be not. So I don't think your proposal is safe.
Thanks
David
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: David Herrmann @ 2018-11-15 12:06 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, ebiggers, jannh, Jiri Kosina,
open list:HID CORE LAYER, linux-kernel, syzkaller-bugs,
Dmitry Vyukov, syzbot+72473edc9bf4eb1c6556, stable,
Andy Lutomirski
In-Reply-To: <CAO-hwJJ3jkQFyf7oByERd98nqEUTjJRu_1tZpFK9sWvgT921iQ@mail.gmail.com>
Hey
On Thu, Nov 15, 2018 at 9:14 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov <dtor@google.com> wrote:
> > > 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.
>
> Yep, any sane distribution would restrict the permissions of
> uhid/userio/uinput to only be accessed by root. If that ever changes,
> there is already an issue with the system and it was compromised
> either by a terribly dizzy sysadmin.
UHID is safe to be used by a non-root user. This does not imply that
you should open up access to the world, but you are free to have a
dedicated group or user with access to uhid. I agree that in most
common desktop-scenarios you should not grant world-access to it,
though.
> >
> > > 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.
>
> AFAICT, there are 2 users of uhid:
> - bluez for BLE devices (using HID over GATT)
> - hid-replay for debugging.
There are several more (eg., android bt-broadcom), and UHID_CREATE is
actively used. Dropping support for it will break these use-cases.
Thanks
David
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: David Herrmann @ 2018-11-15 12:09 UTC (permalink / raw)
To: ebiggers
Cc: Jiri Kosina, Benjamin Tissoires, open list:HID CORE LAYER,
linux-kernel, syzkaller-bugs, Dmitry Vyukov, Dmitry Torokhov,
syzbot+72473edc9bf4eb1c6556, stable, jannh, Andy Lutomirski
In-Reply-To: <20181114215509.163600-1-ebiggers@kernel.org>
Hi
On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> 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: <stable@vger.kernel.org> # v3.6+
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> 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 <linux/atomic.h>
> #include <linux/compat.h>
> +#include <linux/cred.h>
> #include <linux/device.h>
> #include <linux/fs.h>
> #include <linux/hid.h>
> @@ -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().
> + */
> + if (file->f_cred != current_cred() || uaccess_kernel()) {
I think `uaccess_kernel()` would be enough. UHID does not check any
credentials. If you believe this should be there nevertheless, feel
free to keep it. Either way:
Acked-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> + 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:
> --
> 2.19.1.930.g4563a0d9d0-goog
>
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Andy Lutomirski @ 2018-11-15 14:49 UTC (permalink / raw)
To: David Herrmann
Cc: ebiggers, Jiri Kosina, Benjamin Tissoires,
open list:HID CORE LAYER, linux-kernel, syzkaller-bugs,
Dmitry Vyukov, Dmitry Torokhov, syzbot+72473edc9bf4eb1c6556,
stable, jannh, Andy Lutomirski
In-Reply-To: <CANq1E4Qr+k5hWbxKBQQTO0dSP0QFk4V+hJ9Fmmqz6=cOmfD1KA@mail.gmail.com>
> On Nov 15, 2018, at 4:09 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>
> Hi
>
>> On Wed, Nov 14, 2018 at 10:55 PM Eric Biggers <ebiggers@kernel.org> wrote:
>> From: Eric Biggers <ebiggers@google.com>
>>
>> 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: <stable@vger.kernel.org> # v3.6+
>> Cc: Jann Horn <jannh@google.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Signed-off-by: Eric Biggers <ebiggers@google.com>
>> ---
>> 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 <linux/atomic.h>
>> #include <linux/compat.h>
>> +#include <linux/cred.h>
>> #include <linux/device.h>
>> #include <linux/fs.h>
>> #include <linux/hid.h>
>> @@ -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().
>> + */
>> + if (file->f_cred != current_cred() || uaccess_kernel()) {
>
> I think `uaccess_kernel()` would be enough. UHID does not check any
> credentials. If you believe this should be there nevertheless, feel
> free to keep it.
The free check is needed. Without it, consider what sudo >uhid_fd does. It doesn’t use sudo’s credentials, but it does read its address space.
Can this patch get a comment added?
> Either way:
>
> Acked-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: forbid UHID_CREATE under KERNEL_DS or elevated privileges
From: Andy Lutomirski @ 2018-11-15 14:50 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Dmitry Torokhov, ebiggers, jannh, Jiri Kosina,
open list:HID CORE LAYER, linux-kernel, syzkaller-bugs,
Dmitry Vyukov, syzbot+72473edc9bf4eb1c6556, stable,
Andy Lutomirski
In-Reply-To: <CANq1E4ThFYS8=QqnyUVMAafy3BHb8CiDqm9LmT22XZzm9wJSeg@mail.gmail.com>
> On Nov 15, 2018, at 4:06 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
>
> Hey
>
> On Thu, Nov 15, 2018 at 9:14 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>>
>> On Thu, Nov 15, 2018 at 12:20 AM Dmitry Torokhov <dtor@google.com> wrote:
>>>> 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.
>>
>> Yep, any sane distribution would restrict the permissions of
>> uhid/userio/uinput to only be accessed by root. If that ever changes,
>> there is already an issue with the system and it was compromised
>> either by a terribly dizzy sysadmin.
>
> UHID is safe to be used by a non-root user. This does not imply that
> you should open up access to the world, but you are free to have a
> dedicated group or user with access to uhid. I agree that in most
> common desktop-scenarios you should not grant world-access to it,
> though.
>
>>>
>>>> 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.
>>
>> AFAICT, there are 2 users of uhid:
>> - bluez for BLE devices (using HID over GATT)
>> - hid-replay for debugging.
>
> There are several more (eg., android bt-broadcom), and UHID_CREATE is
> actively used. Dropping support for it will break these use-cases.
>
>
Is the support story for these programs such that we could add a big scary warning and remove UHID_CREATE in a couple months?
^ permalink raw reply
* [PATCH RESEND] hyper-v: Fix wakeup from suspend-to-idle
From: Vitaly Kuznetsov @ 2018-11-15 18:09 UTC (permalink / raw)
To: Linux PM
Cc: Rafael J. Wysocki, kys, haiyangz, sthemmin, Jiri Kosina,
Dmitry Torokhov, linux-input, Linux Kernel Mailing List
It makes little sense but still possible to put Hyper-V guests into
suspend-to-idle state. To wake them up two wakeup sources were registered
in the past: hyperv-keyboard and hid-hyperv. However, since
commit eed4d47efe95 ("ACPI / sleep: Ignore spurious SCI wakeups from
suspend-to-idle") pm_wakeup_event() from these devices is ignored. Switch
to pm_wakeup_hard_event() API as these devices are actually the only
possible way to wakeup Hyper-V guests.
Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: K. Y. Srinivasan <kys@microsoft.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- It seems the patch slipped through the cracks, resending.
---
drivers/hid/hid-hyperv.c | 2 +-
drivers/input/serio/hyperv-keyboard.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index b372854cf38d..704049e62d58 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -309,7 +309,7 @@ static void mousevsc_on_receive(struct hv_device *device,
hid_input_report(input_dev->hid_device, HID_INPUT_REPORT,
input_dev->input_buf, len, 1);
- pm_wakeup_event(&input_dev->device->device, 0);
+ pm_wakeup_hard_event(&input_dev->device->device);
break;
default:
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
index 47a0e81a2989..a8b9be3e28db 100644
--- a/drivers/input/serio/hyperv-keyboard.c
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -177,7 +177,7 @@ static void hv_kbd_on_receive(struct hv_device *hv_dev,
* state because the Enter-UP can trigger a wakeup at once.
*/
if (!(info & IS_BREAK))
- pm_wakeup_event(&hv_dev->device, 0);
+ pm_wakeup_hard_event(&hv_dev->device);
break;
--
2.17.2
^ permalink raw reply related
* Re: [PATCH RESEND] hyper-v: Fix wakeup from suspend-to-idle
From: Dmitry Torokhov @ 2018-11-15 19:07 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Linux PM, Rafael J. Wysocki, kys, haiyangz, sthemmin, Jiri Kosina,
linux-input, Linux Kernel Mailing List
In-Reply-To: <20181115180933.396-1-vkuznets@redhat.com>
On Thu, Nov 15, 2018 at 07:09:33PM +0100, Vitaly Kuznetsov wrote:
> It makes little sense but still possible to put Hyper-V guests into
> suspend-to-idle state. To wake them up two wakeup sources were registered
> in the past: hyperv-keyboard and hid-hyperv. However, since
> commit eed4d47efe95 ("ACPI / sleep: Ignore spurious SCI wakeups from
> suspend-to-idle") pm_wakeup_event() from these devices is ignored. Switch
> to pm_wakeup_hard_event() API as these devices are actually the only
> possible way to wakeup Hyper-V guests.
>
> Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: K. Y. Srinivasan <kys@microsoft.com>
> Acked-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> - It seems the patch slipped through the cracks, resending.
OK, so since nobody else took it I'll push it through my tree.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox