[PATCH] gpiolib: Fix potential Spectre v1 vulnerabilities

[PATCH] gpiolib: Fix potential Spectre v1 vulnerabilities

[Gustavo A. R. Silva]

offset and lineinfo.line_offset are 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/gpio/gpiolib.c:580 linehandle_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
drivers/gpio/gpiolib.c:927 lineevent_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
drivers/gpio/gpiolib.c:1053 gpio_ioctl() warn: potential spectre issue 'gdev->descs' [r] (local cap)

Fix this by sanitizing both offset and lineinfo.line_offset before
using them to index gdev->descs.

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: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/gpio/gpiolib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fc36dc358716..73a0a550162e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -29,6 +29,8 @@
 #include <linux/timekeeping.h>
 #include <uapi/linux/gpio.h>
 
+#include <linux/nospec.h>
+
 #include "gpiolib.h"
 
 #define CREATE_TRACE_POINTS
@@ -576,6 +578,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
 			ret = -EINVAL;
 			goto out_free_descs;
 		}
+		offset = array_index_nospec(offset, gdev->ngpio);
 
 		desc = &gdev->descs[offset];
 		ret = gpiod_request(desc, lh->label);
@@ -910,6 +913,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 		ret = -EINVAL;
 		goto out_free_label;
 	}
+	offset = array_index_nospec(offset, gdev->ngpio);
 
 	/* Return an error if a unknown flag is set */
 	if ((lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS) ||
@@ -1049,6 +1053,8 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			return -EFAULT;
 		if (lineinfo.line_offset >= gdev->ngpio)
 			return -EINVAL;
+		lineinfo.line_offset = array_index_nospec(lineinfo.line_offset,
+							  gdev->ngpio);
 
 		desc = &gdev->descs[lineinfo.line_offset];
 		if (desc->name) {
-- 
2.19.2

Re: [PATCH] gpiolib: Fix potential Spectre v1 vulnerabilities

[Linus Walleij]

On Mon, Dec 17, 2018 at 10:34 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:

> offset and lineinfo.line_offset are indirectly controlled by user-space,
> hence leading to a potential exploitation of the Spectre variant 1
> vulnerability.

Goodness gracious me!

> This issue was detected with the help of Smatch:
>
> drivers/gpio/gpiolib.c:580 linehandle_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
> drivers/gpio/gpiolib.c:927 lineevent_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
> drivers/gpio/gpiolib.c:1053 gpio_ioctl() warn: potential spectre issue 'gdev->descs' [r] (local cap)
>
> Fix this by sanitizing both offset and lineinfo.line_offset before
> using them to index gdev->descs.
>
> 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: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Bartosz can you take this for a ride with libgpiod and see if we get
performance regressions from this speculation killing?

If you get some data on that I would like to include it with
the changelog.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Fix potential Spectre v1 vulnerabilities

[Linus Walleij]

On Mon, Dec 17, 2018 at 10:34 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:

> offset and lineinfo.line_offset are 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/gpio/gpiolib.c:580 linehandle_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
> drivers/gpio/gpiolib.c:927 lineevent_create() warn: potential spectre issue 'gdev->descs' [r] (local cap)
> drivers/gpio/gpiolib.c:1053 gpio_ioctl() warn: potential spectre issue 'gdev->descs' [r] (local cap)
>
> Fix this by sanitizing both offset and lineinfo.line_offset before
> using them to index gdev->descs.
>
> 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: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

I don't know what to do with this patch and I am also worried that
it is dangerous and irresponsible to drop it.

I am worried because I simply don't understand the conditions under which
this can be exploited, to what extent, and I am worried that I am sacrificing
performance for security that is not needed on this
performance-critical path, which is why I am asking for performance
effects: I really would like it if someone could take this for a ride
on some x86 system with the mockdev and libgpiod.

It's not like gdev->descs is a secret or contain anything sensitive,
the typical info is just electronic set-up. I get the idea from the commit
message that this is all that may be exposed?

If this can be exploited to expose the whole of kernel memory on the
other hand ("speculation windows are large"), I would be an idiot if I
don't apply it. But then the commit message would say that the entire
memory is at risk?

Yours,
Linus Walleij