Re: [PATCH] posix-timers: Protect posix clock array access against speculation
On Thu, 15 Feb 2018, Dan Williams wrote: > On Thu, Feb 15, 2018 at 6:05 AM, Rasmus Villemoes >wrote: > > (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? > > Right, this looks broken. I would expect: Indeed. Missed that. > if (id >= ARRAY_SIZE(posix_clocks)) > return NULL; > idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); > if (!posix_clocks[idx]) > return NULL; > return posix_clocks[idx]; The !posix_clocks[idx] check is pointless and always was. if (id >= ARRAY_SIZE(posix_clocks)) return NULL; idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); return posix_clocks[idx]; is sufficient. It returns NULL for !posix_clocks[idx] anyway. Thanks, tglx
Re: [PATCH] posix-timers: Protect posix clock array access against speculation
On Thu, 15 Feb 2018, Dan Williams wrote: > On Thu, Feb 15, 2018 at 6:05 AM, Rasmus Villemoes > wrote: > > (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? > > Right, this looks broken. I would expect: Indeed. Missed that. > if (id >= ARRAY_SIZE(posix_clocks)) > return NULL; > idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); > if (!posix_clocks[idx]) > return NULL; > return posix_clocks[idx]; The !posix_clocks[idx] check is pointless and always was. if (id >= ARRAY_SIZE(posix_clocks)) return NULL; idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); return posix_clocks[idx]; is sufficient. It returns NULL for !posix_clocks[idx] anyway. Thanks, tglx
Re: [PATCH] posix-timers: Protect posix clock array access against speculation
On Thu, Feb 15, 2018 at 6:05 AM, Rasmus Villemoeswrote: > 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 >> Cc: sta...@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 >> #include >> #include >> +#include >> >> #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 ? >> _posix_dynamic : _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? It does currently, but perhaps it can be fixed. > > (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? Right, this looks broken. I would expect: if (id >= ARRAY_SIZE(posix_clocks)) return NULL; idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); if (!posix_clocks[idx]) return NULL; return posix_clocks[idx];
Re: [PATCH] posix-timers: Protect posix clock array access against speculation
On Thu, Feb 15, 2018 at 6:05 AM, Rasmus Villemoes wrote: > 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 >> Cc: sta...@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 >> #include >> #include >> +#include >> >> #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 ? >> _posix_dynamic : _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? It does currently, but perhaps it can be fixed. > > (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? Right, this looks broken. I would expect: if (id >= ARRAY_SIZE(posix_clocks)) return NULL; idx = array_index_nospec(idx, ARRAY_SIZE(posix_clocks)); if (!posix_clocks[idx]) return NULL; return posix_clocks[idx];
Re: [PATCH] posix-timers: Protect posix clock array access against speculation
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> Cc: sta...@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 > #include > #include > +#include > > #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 ? > _posix_dynamic : _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
Re: [PATCH] posix-timers: Protect posix clock array access against speculation
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 > Cc: sta...@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 > #include > #include > +#include > > #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 ? > _posix_dynamic : _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