Skip to contentSkip to conversation

Input: uinput - fix Spectre v1 vulnerability

PATCH In review Gustavo A. R. Silva 4 participants 8 messages Oct 16, 2018 – Oct 22, 2018

Subsystems (2)
HID Input drivers
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 Input: uinput - fix Spectre v1 vulnerability

setup.code can be indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential spectre issue 'dev->absinfo' [w] (local cap)

Fix this by sanitizing setup.code before using it to index dev->absinfo.

Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/input/misc/uinput.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/input/misc/uinput.c +3 -0
index 8ec483e..97b0809 100644
@@ -42,6 +42,8 @@
 #include <linux/input/mt.h>
 #include "../input-compat.h"
 
+#include <linux/nospec.h>
+
 #define UINPUT_NAME		"uinput"
 #define UINPUT_BUFFER_SIZE	16
 #define UINPUT_NUM_REQUESTS	16

@@ -497,6 +499,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
 
 	if (setup.code > ABS_MAX)
 		return -ERANGE;
+	setup.code = array_index_nospec(setup.code, ABS_MAX + 1);
 
 	dev = udev->dev;
Signature · 1 line
2.7.4
maintainer
#1 lore raw

Hi Gustavo,

Quoted 9 lines · Gustavo A. R. Silva

setup.code can be indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential spectre issue 'dev->absinfo' [w] (local cap)

Fix this by sanitizing setup.code before using it to index dev->absinfo.

So we are saying that attacker, by repeatedly calling ioctl(..., UI_ABS_SETUP, ...) will be able to poison branch predictor and discover another program or kernel secrets? But uinput is a privileged interface open to root only, as it allows injecting arbitrary keystrokes into the kernel. And since only root can use uinput, meh?

Thanks.

Signature · 1 line
Dmitry
author
#2 lore raw

Hi Dmitry,

Quoted 11 lines · Dmitry Torokhov

Hi Gustavo,

Quoted 8 lines · Gustavo A. R. Silva

setup.code can be indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential spectre issue 'dev->absinfo' [w] (local cap)

Quoted 8 lines
Quoted 1 line

Fix this by sanitizing setup.code before using it to index dev->absinfo.

So we are saying that attacker, by repeatedly calling ioctl(..., UI_ABS_SETUP, ...) will be able to poison branch predictor and discover another program or kernel secrets? But uinput is a privileged interface open to root only, as it allows injecting arbitrary keystrokes into the kernel. And since only root can use uinput, meh?

Oh I see... in that case this is a false positive.

Although, I wonder if all these operations are only accessible to root:

static const struct file_operations uinput_fops = {

        .owner          = THIS_MODULE,
        .open           = uinput_open,
        .release        = uinput_release,
        .read           = uinput_read,
        .write          = uinput_write,
        .poll           = uinput_poll,

.unlocked_ioctl = uinput_ioctl,
#ifdef CONFIG_COMPAT
.compat_ioctl = uinput_compat_ioctl,
#endif
.llseek = no_llseek,
};

Thanks for the feedback.
--
Gustavo

maintainer
#3 lore raw
Quoted 39 lines · Gustavo A. R. Silva

Hi Dmitry,

Quoted 4 lines · Dmitry Torokhov

Hi Gustavo,

Quoted 1 line · Gustavo A. R. Silva

setup.code can be indirectly controlled by user-space, hence leading

to

Quoted 8 lines
Quoted 8 lines

a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential spectre issue 'dev->absinfo' [w] (local cap)

Fix this by sanitizing setup.code before using it to index

dev->absinfo.

Quoted 3 lines

So we are saying that attacker, by repeatedly calling ioctl(..., UI_ABS_SETUP, ...) will be able to poison branch predictor and

discover

Quoted 1 line

another program or kernel secrets? But uinput is a privileged

interface

Quoted 1 line

open to root only, as it allows injecting arbitrary keystrokes into

the

Quoted 2 lines

kernel. And since only root can use uinput, meh?

Oh I see... in that case this is a false positive.

Although, I wonder if all these operations are only accessible to root:

static const struct file_operations uinput_fops = {

       .owner          = THIS_MODULE,
       .open           = uinput_open,
       .release        = uinput_release,
       .read           = uinput_read,
       .write          = uinput_write,
       .poll           = uinput_poll,
Quoted 6 lines

.unlocked_ioctl = uinput_ioctl,
#ifdef CONFIG_COMPAT
.compat_ioctl = uinput_compat_ioctl,
#endif
.llseek = no_llseek,
};

/dev/uinput must be 0600, or accessible to equally privileged user, or you'll be opening your system to much mischief.

Thanks.

Signature · 1 line
Dmitry
author
#4 lore raw
Quoted 2 lines · Dmitry Torokhov

/dev/uinput

I've got it. This explains it all. :)

Quoted 2 lines

must be 0600, or accessible to equally privileged user, or you'll be opening your system to much mischief.

Thanks, Dmitry.
--
Gustavo

#5 lore raw
Quoted 8 lines · On Tue, 16 Oct 2018 20:12:43 +0200 "Gustavo A. R. Silva"…
Quoted 2 lines · Dmitry Torokhov

/dev/uinput

I've got it. This explains it all. :)

Quoted 1 line

must be 0600, or accessible to equally privileged user, or you'll be opening your system to much mischief.

Still a correct change.

CAP_SYS_RAWIO is not the same as being root, especially in a container.

Alan

maintainer
#6 lore raw
Quoted 9 lines · Alan Cox
Quoted 5 lines · On Tue, 16 Oct 2018 20:12:43 +0200 "Gustavo A. R. Silva"…
Quoted 2 lines · Dmitry Torokhov

/dev/uinput

Quoted 7 lines
Quoted 3 lines

I've got it. This explains it all. :)

Quoted 1 line

must be 0600, or accessible to equally privileged user, or you'll be opening your system to much mischief.

Still a correct change.

CAP_SYS_RAWIO is not the same as being root, especially in a container.

Giving access to uinput in an unprivileged container is nutty as well.

Thanks.

Signature · 1 line
Dmitry
#7 lore raw
In reply to Gustavo A. R. Silva
Quoted 16 lines · Gustavo A. R. Silva

Hi Dmitry,

Quoted 13 lines · Dmitry Torokhov

Hi Gustavo,

Quoted 9 lines · Gustavo A. R. Silva

setup.code can be indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/input/misc/uinput.c:512 uinput_abs_setup() warn: potential spectre issue 'dev->absinfo' [w] (local cap)

Fix this by sanitizing setup.code before using it to index dev->absinfo.

Quoted 8 lines
Quoted 6 lines

So we are saying that attacker, by repeatedly calling ioctl(..., UI_ABS_SETUP, ...) will be able to poison branch predictor and discover another program or kernel secrets? But uinput is a privileged interface open to root only, as it allows injecting arbitrary keystrokes into the kernel. And since only root can use uinput, meh?

Oh I see... in that case this is a false positive.

No, please lets fix it.

Some people are trying to make sure kernel is "more priviledged" than root -- its called secure boot etc.

And "if you givean attacker possibility to generate keystrokes, he can read kernel memory as well"... is very unexpected.

Unexpected is bad when talking about security.
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html