On 05/15/2018 05:57 PM, Dan Williams wrote:
On Tue, May 15, 2018 at 3:29 PM, Thomas Gleixner <t...@linutronix.de> wrote:
On Tue, 15 May 2018, Andrew Morton wrote:
On Mon, 14 May 2018 22:00:38 -0500 "Gustavo A. R. Silva"
<gust...@embeddedor.com> wrote:
resource can be 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:
kernel/sys.c:1474 __do_compat_sys_old_getrlimit() warn: potential
spectre issue 'get_current()->signal->rlim' (local cap)
kernel/sys.c:1455 __do_sys_old_getrlimit() warn: potential spectre issue
'get_current()->signal->rlim' (local cap)
Fix this by sanitizing *resource* before using it to index
current->signal->rlim
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].
hm. Not my area, but I'm always willing to learn ;)
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -69,6 +69,9 @@
#include <asm/io.h>
#include <asm/unistd.h>
+/* Hardening for Spectre-v1 */
+#include <linux/nospec.h>
+
#include "uid16.h"
#ifndef SET_UNALIGN_CTL
@@ -1451,6 +1454,7 @@ SYSCALL_DEFINE2(old_getrlimit, unsigned int, resource,
if (resource >= RLIM_NLIMITS)
return -EINVAL;
+ resource = array_index_nospec(resource, RLIM_NLIMITS);
task_lock(current->group_leader);
x = current->signal->rlim[resource];
Can the speculation proceed past the task_lock()? Or is the policy to
ignore such happy happenstances even if they are available?
Locks are not in the way of speculation. Speculation has almost no limits
except serializing instructions. At least they respect the magic AND
limitation in array_index_nospec().
I'd say it another way, because they don't respect the magic AND, we
just make the result in the speculation path safe. So, it's controlled
speculation.
Dan,
What do you think about adding the following function to the nospec API:
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index e791ebc..81e9a77 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -55,4 +55,17 @@ static inline unsigned long
array_index_mask_nospec(unsigned long index,
\
(typeof(_i)) (_i & _mask); \
})
+
+
+#ifndef sanitize_index_nospec
+inline bool sanitize_index_nospec(unsigned long index,
+ unsigned long size)
+{
+ if (index >= size)
+ return false;
+ index = array_index_nospec(index, size);
+
+ return true;
+}
+#endif
#endif /* _LINUX_NOSPEC_H */
And here is an example of its use:
diff --git a/drivers/media/i2c/tvp7002.c b/drivers/media/i2c/tvp7002.c
index 4599b7e..27b39c0 100644
--- a/drivers/media/i2c/tvp7002.c
+++ b/drivers/media/i2c/tvp7002.c
@@ -35,6 +35,8 @@
#include <media/v4l2-ctrls.h>
#include <media/v4l2-fwnode.h>
+#include <linux/nospec.h>
+
#include "tvp7002_reg.h"
MODULE_DESCRIPTION("TI TVP7002 Video and Graphics Digitizer driver");
@@ -784,7 +786,7 @@ static int tvp7002_enum_dv_timings(struct
v4l2_subdev *sd,
return -EINVAL;
/* Check requested format index is within range */
- if (timings->index >= NUM_TIMINGS)
+ if (!sanitize_index_nospec(timings->index, NUM_TIMINGS))
return -EINVAL;
timings->timings = tvp7002_timings[timings->index].timings;
This patter is very common. So, it may be a good idea to unify both
bounds checking and the serialization of instructions into a single
function.
What do you think?
Thanks
--
Gustavo
