Re: [PATCH V2] IB/core: Add mitigation for Spectre V1

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3337 bytes --]

On Tue, 2019-07-30 at 21:39 -0700, Luck, Tony wrote:
> Some processors may mispredict an array bounds check and
> speculatively access memory that they should not. With
> a user supplied array index we like to play things safe
> by masking the value with the array size before it is
> used as an index.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> ---
> V2: Mask the index *AFTER* the bounds check. Issue pointed
>     out by Gustavo. Fix suggested by Ira.
> 
>  drivers/infiniband/core/user_mad.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/user_mad.c
> b/drivers/infiniband/core/user_mad.c
> index 9f8a48016b41..32cea5ed9ce1 100644
> --- a/drivers/infiniband/core/user_mad.c
> +++ b/drivers/infiniband/core/user_mad.c
> @@ -49,6 +49,7 @@
>  #include <linux/sched.h>
>  #include <linux/semaphore.h>
>  #include <linux/slab.h>
> +#include <linux/nospec.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -888,7 +889,12 @@ static int ib_umad_unreg_agent(struct
> ib_umad_file *file, u32 __user *arg)
>  	mutex_lock(&file->port->file_mutex);
>  	mutex_lock(&file->mutex);
>  
> -	if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) {
> +	if (id >= IB_UMAD_MAX_AGENTS) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	id = array_index_nospec(id, IB_UMAD_MAX_AGENTS);
> +	if (!__get_agent(file, id)) {
>  		ret = -EINVAL;
>  		goto out;
>  	}

I'm not sure this is the best fix for this.  However, here is where I
get to admit that I largely ignored the whole Spectre V1 thing, so I'm
not sure I completely understand the vulnerability and the limits to
that.  But, looking at the function, it seems we can do an early return
without ever taking any of the mutexes in the function in the case of id
>= IB_UMAD_MAX_AGENTS, so if we did that, would that separate the
checking of id far enough from the usage of it as an array index that we
wouldn't need the clamp to prevent speculative prefetch?  About how far,
in code terms, does this speculative prefetch occur?

This is the patch I was thinking of:

diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 9f8a48016b41..1e507dd257ff 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -49,6 +49,7 @@
 #include <linux/sched.h>
 #include <linux/semaphore.h>
 #include <linux/slab.h>
+#include <linux/nospec.h>
 
 #include <linux/uaccess.h>
 
@@ -884,11 +885,18 @@ static int ib_umad_unreg_agent(struct ib_umad_file *file, u32 __user *arg)
 
        if (get_user(id, arg))
                return -EFAULT;
+       if (id >= IB_UMAD_MAX_AGENTS)
+               return -EINVAL;
 
        mutex_lock(&file->port->file_mutex);
        mutex_lock(&file->mutex);
 
-       if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) {
+       /*
+        * Is our check of id far enough away, code wise, to prevent
+        * speculative prefetch?
+        */
+       id = array_index_nospec(id, IB_UMAD_MAX_AGENTS);
+       if (!__get_agent(file, id)) {
                ret = -EINVAL;
                goto out;
        }

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
    Fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]