Messages in this thread | | | Subject | Re: [PATCH] posix-timers: Protect posix clock array access against speculation | From | Rasmus Villemoes <> | Date | Thu, 15 Feb 2018 15:05:08 +0100 |
| |
On 2018-02-15 14:27, Thomas Gleixner wrote: > The (clock) id argument of clockid_to_kclock() comes straight from user > space via various syscalls and is used as index into the posix_clocks > array. > > Protect it against spectre v1 array out of bounds speculation. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: stable@vger.kernel.org > --- > kernel/time/posix-timers.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > --- a/kernel/time/posix-timers.c > +++ b/kernel/time/posix-timers.c > @@ -50,6 +50,7 @@ > #include <linux/export.h> > #include <linux/hashtable.h> > #include <linux/compat.h> > +#include <linux/nospec.h> > > #include "timekeeping.h" > #include "posix-timers.h" > @@ -1346,11 +1347,14 @@ static const struct k_clock * const posi > > static const struct k_clock *clockid_to_kclock(const clockid_t id) > { > + clockid_t idx = id; > + > if (id < 0) > return (id & CLOCKFD_MASK) == CLOCKFD ? > &clock_posix_dynamic : &clock_posix_cpu; > > if (id >= ARRAY_SIZE(posix_clocks) || !posix_clocks[id]) > return NULL; > - return posix_clocks[id]; > + > + return posix_clocks[array_index_nospec(idx, ARRAY_SIZE(posix_clocks))]; > } >
Stupid questions from someone trying to learn what the rules for when and how to apply these _nospec macros:
(1) why introduce the idx var? There's no assignment to it other than the initialization. Is it some magic in array_index_nospec that prevents the use of a const-qualified expression?
(2) The line "if (id >= ARRAY_SIZE(posix_clocks) || !posix_clocks[id])" still seems to allow speculatively accessing posix_clocks[id]. Is that ok, and even if so, wouldn't it be cleaner to elide the !posix_clocks[id] check and just return the NULL safely fetched from the array in the following line?
Rasmus
| |