[PATCH 0/1] nospec: Add documentation for array_index_nospec

[PATCH 0/1] nospec: Add documentation for array_index_nospec

[Jordy Zomer]

array_index_nospec() should only be used if the upper boundary is a built
time constant. Otherwise the boundary could be speculated on as well.
While it might be more difficult to control two loads, it doesn't rule
out the problem. Adding this to the documentation so people won't mis-use
it instead of barrier_nospec().

Jordy Zomer (1):
  nospec: Add documentation for array_index_nospec

 Documentation/staging/speculation.rst | 5 +++++
 include/linux/nospec.h                | 5 +++++
 2 files changed, 10 insertions(+)

-- 
2.41.0.162.gfafddb0af9-goog

[PATCH 1/1] nospec: Add documentation for array_index_nospec

[Jordy Zomer]

array_index_nospec() should only be used if the upper boundary is a built
time constant. Otherwise the boundary could be speculated on as well.
While it might be more difficult to control two loads, it doesn't rule
out the problem. Adding this to the documentation so people won't mis-use
it instead of barrier_nospec().

Signed-off-by: Jordy Zomer <jordyzomer@google.com>
---
 Documentation/staging/speculation.rst | 5 +++++
 include/linux/nospec.h                | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/staging/speculation.rst b/Documentation/staging/speculation.rst
index 8045d99bcf12..efc0ab32263b 100644
--- a/Documentation/staging/speculation.rst
+++ b/Documentation/staging/speculation.rst
@@ -79,6 +79,11 @@ A call to array_index_nospec(index, size) returns a sanitized index
 value that is bounded to [0, size) even under cpu speculation
 conditions.
 
+Please note that this function should only be used if the upper
+boundary is a built-time constant, otherwise this could be
+speculated on as well. If this is not the case please refer to
+barrier_nospec().
+
 This can be used to protect the earlier load_array() example::
 
 	int load_array(int *array, unsigned int index)
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index 9f0af4f116d9..1d72c40595f4 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -51,6 +51,11 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
  * ...if the CPU speculates past the bounds check then
  * array_index_nospec() will clamp the index within the range of [0,
  * size).
+ *
+ * Please note that this function should only be used if the upper
+ * boundary is a built-time constant, otherwise this could be
+ * speculated on as well. If this is not the case please refer to
+ * barrier_nospec().
  */
 #define array_index_nospec(index, size)					\
 ({									\
-- 
2.41.0.162.gfafddb0af9-goog

Re: [PATCH 1/1] nospec: Add documentation for array_index_nospec

[Dave Hansen]

On 6/16/23 06:37, Jordy Zomer wrote:
> array_index_nospec() should only be used if the upper boundary is a built
> time constant. Otherwise the boundary could be speculated on as well.
> While it might be more difficult to control two loads, it doesn't rule
> out the problem. Adding this to the documentation so people won't mis-use
> it instead of barrier_nospec().

Then shouldn't we be using __builtin_constant_p() to enforce this?

Re: [PATCH 1/1] nospec: Add documentation for array_index_nospec

[Matthew Wilcox]

On Fri, Jun 16, 2023 at 01:37:35PM +0000, Jordy Zomer wrote:
> +Please note that this function should only be used if the upper
> +boundary is a built-time constant, otherwise this could be
> +speculated on as well. If this is not the case please refer to
> +barrier_nospec().

"build time", not "built time".  Also, "Please note that" doesn't
really add any value.  You can just write:

This function should only be used if the upper boundary is a build-time
constant, otherwise this could be speculated on as well.  If it is not
a constant, use barrier_nospec() instead.

Re: [PATCH 1/1] nospec: Add documentation for array_index_nospec

[Jordy Zomer]

Thanks both, I was planning on doing some plumbing next week to fix the
already affected calls and then add a BUILD_BUG_ON() in combination with
__builtin_constant_p() to prevent misuse from happening in the future. In
addition I'll send a V2 next week to fix the spelling/wording issue.

Cheers,

Jordy

On Fri, Jun 16, 2023 at 8:05 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Jun 16, 2023 at 01:37:35PM +0000, Jordy Zomer wrote:
> > +Please note that this function should only be used if the upper
> > +boundary is a built-time constant, otherwise this could be
> > +speculated on as well. If this is not the case please refer to
> > +barrier_nospec().
>
> "build time", not "built time".  Also, "Please note that" doesn't
> really add any value.  You can just write:
>
> This function should only be used if the upper boundary is a build-time
> constant, otherwise this could be speculated on as well.  If it is not
> a constant, use barrier_nospec() instead.
>