* [PATCH] IB/core: Add mitigation for Spectre V1 @ 2019-07-30 20:24 Tony Luck 2019-07-30 23:34 ` Ira Weiny 2019-07-30 23:52 ` Gustavo A. R. Silva 0 siblings, 2 replies; 9+ messages in thread From: Tony Luck @ 2019-07-30 20:24 UTC (permalink / raw) To: Doug Ledford Cc: Tony Luck, Jason Gunthorpe, Leon Romanovsky, Parav Pandit, Ira Weiny, linux-rdma, linux-kernel 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> --- [I don't have h/w, so just compile tested] drivers/infiniband/core/user_mad.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c index 9f8a48016b41..fdce254e4f65 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,6 +889,7 @@ static int ib_umad_unreg_agent(struct ib_umad_file *file, u32 __user *arg) mutex_lock(&file->port->file_mutex); mutex_lock(&file->mutex); + id = array_index_nospec(id, IB_UMAD_MAX_AGENTS); if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) { ret = -EINVAL; goto out; -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] IB/core: Add mitigation for Spectre V1 2019-07-30 20:24 [PATCH] IB/core: Add mitigation for Spectre V1 Tony Luck @ 2019-07-30 23:34 ` Ira Weiny 2019-07-30 23:52 ` Gustavo A. R. Silva 1 sibling, 0 replies; 9+ messages in thread From: Ira Weiny @ 2019-07-30 23:34 UTC (permalink / raw) To: Tony Luck Cc: Doug Ledford, Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel On Tue, Jul 30, 2019 at 01:24:07PM -0700, Tony Luck 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> Reviewed-by: Ira Weiny <ira.weiny@intel.com> Tested-by: Ira Weiny <ira.weiny@intel.com> > --- > > [I don't have h/w, so just compile tested] > > drivers/infiniband/core/user_mad.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c > index 9f8a48016b41..fdce254e4f65 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,6 +889,7 @@ static int ib_umad_unreg_agent(struct ib_umad_file *file, u32 __user *arg) > mutex_lock(&file->port->file_mutex); > mutex_lock(&file->mutex); > > + id = array_index_nospec(id, IB_UMAD_MAX_AGENTS); > if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) { > ret = -EINVAL; > goto out; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] IB/core: Add mitigation for Spectre V1 2019-07-30 20:24 [PATCH] IB/core: Add mitigation for Spectre V1 Tony Luck 2019-07-30 23:34 ` Ira Weiny @ 2019-07-30 23:52 ` Gustavo A. R. Silva 2019-07-31 4:28 ` Ira Weiny 1 sibling, 1 reply; 9+ messages in thread From: Gustavo A. R. Silva @ 2019-07-30 23:52 UTC (permalink / raw) To: Tony Luck, Doug Ledford Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, Ira Weiny, linux-rdma, linux-kernel On 7/30/19 3:24 PM, Tony Luck 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> > --- > > [I don't have h/w, so just compile tested] > > drivers/infiniband/core/user_mad.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c > index 9f8a48016b41..fdce254e4f65 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,6 +889,7 @@ static int ib_umad_unreg_agent(struct ib_umad_file *file, u32 __user *arg) > mutex_lock(&file->port->file_mutex); > mutex_lock(&file->mutex); > > + id = array_index_nospec(id, IB_UMAD_MAX_AGENTS); This is wrong. This prevents the below condition id >= IB_UMAD_MAX_AGENTS from ever being true. And I don't think this is what you want. > if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) { > ret = -EINVAL; > goto out; > -- Gustavo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] IB/core: Add mitigation for Spectre V1 2019-07-30 23:52 ` Gustavo A. R. Silva @ 2019-07-31 4:28 ` Ira Weiny 2019-07-31 4:39 ` [PATCH V2] " Luck, Tony 0 siblings, 1 reply; 9+ messages in thread From: Ira Weiny @ 2019-07-31 4:28 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Tony Luck, Doug Ledford, Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel On Tue, Jul 30, 2019 at 06:52:12PM -0500, Gustavo A. R. Silva wrote: > > > On 7/30/19 3:24 PM, Tony Luck 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> > > --- > > > > [I don't have h/w, so just compile tested] > > > > drivers/infiniband/core/user_mad.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c > > index 9f8a48016b41..fdce254e4f65 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,6 +889,7 @@ static int ib_umad_unreg_agent(struct ib_umad_file *file, u32 __user *arg) > > mutex_lock(&file->port->file_mutex); > > mutex_lock(&file->mutex); > > > > + id = array_index_nospec(id, IB_UMAD_MAX_AGENTS); > > This is wrong. This prevents the below condition id >= IB_UMAD_MAX_AGENTS > from ever being true. And I don't think this is what you want. Ah Yea... FWIW this would probably never be hit. Tony; split the check? 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; } Ira > > > if (id >= IB_UMAD_MAX_AGENTS || !__get_agent(file, id)) { > > ret = -EINVAL; > > goto out; > > > > -- > Gustavo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2] IB/core: Add mitigation for Spectre V1 2019-07-31 4:28 ` Ira Weiny @ 2019-07-31 4:39 ` Luck, Tony 2019-07-31 14:52 ` Doug Ledford 0 siblings, 1 reply; 9+ messages in thread From: Luck, Tony @ 2019-07-31 4:39 UTC (permalink / raw) To: Ira Weiny Cc: Gustavo A. R. Silva, Doug Ledford, Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel 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; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] IB/core: Add mitigation for Spectre V1 2019-07-31 4:39 ` [PATCH V2] " Luck, Tony @ 2019-07-31 14:52 ` Doug Ledford 2019-07-31 17:52 ` Gustavo A. R. Silva 0 siblings, 1 reply; 9+ messages in thread From: Doug Ledford @ 2019-07-31 14:52 UTC (permalink / raw) To: Luck, Tony, Ira Weiny Cc: Gustavo A. R. Silva, Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel [-- 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 --] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] IB/core: Add mitigation for Spectre V1 2019-07-31 14:52 ` Doug Ledford @ 2019-07-31 17:52 ` Gustavo A. R. Silva 2019-07-31 18:52 ` Doug Ledford 0 siblings, 1 reply; 9+ messages in thread From: Gustavo A. R. Silva @ 2019-07-31 17:52 UTC (permalink / raw) To: Doug Ledford, Luck, Tony, Ira Weiny Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel On 7/31/19 9:52 AM, Doug Ledford wrote: > > 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: > > > @@ -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; > } > This is insufficient. The speculation windows are large: "Speculative execution on modern CPUs can run several hundred instructions ahead." [1] [1] https://spectreattack.com/spectre.pdf -- Gustavo -- Gustavo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] IB/core: Add mitigation for Spectre V1 2019-07-31 17:52 ` Gustavo A. R. Silva @ 2019-07-31 18:52 ` Doug Ledford 2019-07-31 19:22 ` Doug Ledford 0 siblings, 1 reply; 9+ messages in thread From: Doug Ledford @ 2019-07-31 18:52 UTC (permalink / raw) To: Gustavo A. R. Silva, Luck, Tony, Ira Weiny Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel [-- Attachment #1: Type: text/plain, Size: 631 bytes --] On Wed, 2019-07-31 at 12:52 -0500, Gustavo A. R. Silva wrote: > This is insufficient. The speculation windows are large: > > "Speculative execution on modern CPUs can run several > hundred instructions ahead." [1] > > [1] https://spectreattack.com/spectre.pdf Thanks, I'll take a look at that. That issue aside, returning without wasting time on two mutexes is still better IMO, so I like my patch more than the proposed one. Tony, would you like to resubmit? -- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2] IB/core: Add mitigation for Spectre V1 2019-07-31 18:52 ` Doug Ledford @ 2019-07-31 19:22 ` Doug Ledford 0 siblings, 0 replies; 9+ messages in thread From: Doug Ledford @ 2019-07-31 19:22 UTC (permalink / raw) To: Gustavo A. R. Silva, Luck, Tony, Ira Weiny Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, linux-rdma, linux-kernel [-- Attachment #1: Type: text/plain, Size: 802 bytes --] On Wed, 2019-07-31 at 14:52 -0400, Doug Ledford wrote: > On Wed, 2019-07-31 at 12:52 -0500, Gustavo A. R. Silva wrote: > > This is insufficient. The speculation windows are large: > > > > "Speculative execution on modern CPUs can run several > > hundred instructions ahead." [1] > > > > [1] https://spectreattack.com/spectre.pdf > > Thanks, I'll take a look at that. That issue aside, returning without > wasting time on two mutexes is still better IMO, so I like my patch > more > than the proposed one. Tony, would you like to resubmit? > Never mind, I took your V2 and fixed it up like I wanted. Patch applied, thanks. -- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-31 19:22 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-30 20:24 [PATCH] IB/core: Add mitigation for Spectre V1 Tony Luck 2019-07-30 23:34 ` Ira Weiny 2019-07-30 23:52 ` Gustavo A. R. Silva 2019-07-31 4:28 ` Ira Weiny 2019-07-31 4:39 ` [PATCH V2] " Luck, Tony 2019-07-31 14:52 ` Doug Ledford 2019-07-31 17:52 ` Gustavo A. R. Silva 2019-07-31 18:52 ` Doug Ledford 2019-07-31 19:22 ` Doug Ledford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).