LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] atm: zatm: Fix potential Spectre v1
@ 2018-06-29 18:28 Gustavo A. R. Silva
  2018-06-30 12:25 ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-06-29 18:28 UTC (permalink / raw)
  To: Chas Williams, David S. Miller
  Cc: linux-atm-general, netdev, linux-kernel, Gustavo A. R. Silva

pool can be indirectly controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/atm/zatm.c:1491 zatm_ioctl() warn: potential spectre issue
'zatm_dev->pool_info' (local cap)

Fix this by sanitizing pool before using it to index
zatm_dev->pool_info

Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/atm/zatm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index a8d2eb0..2c288d1 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -1483,6 +1483,8 @@ static int zatm_ioctl(struct atm_dev *dev,unsigned int cmd,void __user *arg)
 					return -EFAULT;
 				if (pool < 0 || pool > ZATM_LAST_POOL)
 					return -EINVAL;
+				pool = array_index_nospec(pool,
+							  ZATM_LAST_POOL + 1);
 				if (copy_from_user(&info,
 				    &((struct zatm_pool_req __user *) arg)->info,
 				    sizeof(info))) return -EFAULT;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] atm: zatm: Fix potential Spectre v1
  2018-06-29 18:28 [PATCH] atm: zatm: Fix potential Spectre v1 Gustavo A. R. Silva
@ 2018-06-30 12:25 ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-06-30 12:25 UTC (permalink / raw)
  To: gustavo; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel

From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Date: Fri, 29 Jun 2018 13:28:07 -0500

> pool can be indirectly controlled by user-space, hence leading to
> a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This issue was detected with the help of Smatch:
> 
> drivers/atm/zatm.c:1491 zatm_ioctl() warn: potential spectre issue
> 'zatm_dev->pool_info' (local cap)
> 
> Fix this by sanitizing pool before using it to index
> zatm_dev->pool_info
> 
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
> 
> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Applied.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] atm: zatm: Fix potential Spectre v1
  2018-05-03 18:17 Gustavo A. R. Silva
  2018-05-03 19:09 ` Randy Dunlap
@ 2018-05-04 16:54 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2018-05-04 16:54 UTC (permalink / raw)
  To: gustavo; +Cc: 3chas3, linux-atm-general, netdev, linux-kernel

From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Date: Thu, 3 May 2018 13:17:12 -0500

> pool can be indirectly controlled by user-space, hence leading to
> a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This issue was detected with the help of Smatch:
> 
> drivers/atm/zatm.c:1462 zatm_ioctl() warn: potential spectre issue
> 'zatm_dev->pool_info' (local cap)
> 
> Fix this by sanitizing pool before using it to index
> zatm_dev->pool_info
> 
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
> 
> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

Applied and queued up for -stable.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] atm: zatm: Fix potential Spectre v1
  2018-05-03 19:09 ` Randy Dunlap
@ 2018-05-03 19:25   ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-05-03 19:25 UTC (permalink / raw)
  To: rdunlap; +Cc: gustavo, 3chas3, linux-atm-general, netdev, linux-kernel

From: Randy Dunlap <rdunlap@infradead.org>
Date: Thu, 3 May 2018 12:09:40 -0700

> Just for (my) info:  all of these types of patches are to prevent
> what is loaded in cache when the index is out of range, right?
> Not some random pool_info[random], but pool_info[valid, i.e., 0].
> 
> Since the value of pool is already sanity checked and -EINVAL is
> returned when it's out of range.

Well, the whole point is that the cpu can speculate execution past the
range check and execute the indexed read anyways.  So even if the
value is "sanity checked" the cpu can execute ahead and load things
into the cache, just cancelling the register state updates later when
the range check is fully resolved.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] atm: zatm: Fix potential Spectre v1
  2018-05-03 18:17 Gustavo A. R. Silva
@ 2018-05-03 19:09 ` Randy Dunlap
  2018-05-03 19:25   ` David Miller
  2018-05-04 16:54 ` David Miller
  1 sibling, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2018-05-03 19:09 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Chas Williams, David S. Miller
  Cc: linux-atm-general, netdev, linux-kernel

On 05/03/2018 11:17 AM, Gustavo A. R. Silva wrote:
> pool can be indirectly controlled by user-space, hence leading to
> a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This issue was detected with the help of Smatch:
> 
> drivers/atm/zatm.c:1462 zatm_ioctl() warn: potential spectre issue
> 'zatm_dev->pool_info' (local cap)
> 
> Fix this by sanitizing pool before using it to index
> zatm_dev->pool_info
> 
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
> 
> [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Hi,

Just for (my) info:  all of these types of patches are to prevent
what is loaded in cache when the index is out of range, right?
Not some random pool_info[random], but pool_info[valid, i.e., 0].

Since the value of pool is already sanity checked and -EINVAL is
returned when it's out of range.

Thanks.

> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/atm/zatm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
> index 1ef67db..9c9a229 100644
> --- a/drivers/atm/zatm.c
> +++ b/drivers/atm/zatm.c
> @@ -28,6 +28,7 @@
>  #include <asm/io.h>
>  #include <linux/atomic.h>
>  #include <linux/uaccess.h>
> +#include <linux/nospec.h>
>  
>  #include "uPD98401.h"
>  #include "uPD98402.h"
> @@ -1458,6 +1459,8 @@ static int zatm_ioctl(struct atm_dev *dev,unsigned int cmd,void __user *arg)
>  					return -EFAULT;
>  				if (pool < 0 || pool > ZATM_LAST_POOL)
>  					return -EINVAL;
> +				pool = array_index_nospec(pool,
> +							  ZATM_LAST_POOL + 1);
>  				spin_lock_irqsave(&zatm_dev->lock, flags);
>  				info = zatm_dev->pool_info[pool];
>  				if (cmd == ZATM_GETPOOLZ) {
> 


-- 
~Randy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] atm: zatm: Fix potential Spectre v1
@ 2018-05-03 18:17 Gustavo A. R. Silva
  2018-05-03 19:09 ` Randy Dunlap
  2018-05-04 16:54 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-03 18:17 UTC (permalink / raw)
  To: Chas Williams, David S. Miller
  Cc: linux-atm-general, netdev, linux-kernel, Gustavo A. R. Silva

pool can be indirectly controlled by user-space, hence leading to
a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

drivers/atm/zatm.c:1462 zatm_ioctl() warn: potential spectre issue
'zatm_dev->pool_info' (local cap)

Fix this by sanitizing pool before using it to index
zatm_dev->pool_info

Notice that given that speculation windows are large, the policy is
to kill the speculation on the first load and not worry if it can be
completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/atm/zatm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index 1ef67db..9c9a229 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -28,6 +28,7 @@
 #include <asm/io.h>
 #include <linux/atomic.h>
 #include <linux/uaccess.h>
+#include <linux/nospec.h>
 
 #include "uPD98401.h"
 #include "uPD98402.h"
@@ -1458,6 +1459,8 @@ static int zatm_ioctl(struct atm_dev *dev,unsigned int cmd,void __user *arg)
 					return -EFAULT;
 				if (pool < 0 || pool > ZATM_LAST_POOL)
 					return -EINVAL;
+				pool = array_index_nospec(pool,
+							  ZATM_LAST_POOL + 1);
 				spin_lock_irqsave(&zatm_dev->lock, flags);
 				info = zatm_dev->pool_info[pool];
 				if (cmd == ZATM_GETPOOLZ) {
-- 
2.7.4

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-06-30 12:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-29 18:28 [PATCH] atm: zatm: Fix potential Spectre v1 Gustavo A. R. Silva
2018-06-30 12:25 ` David Miller
  -- strict thread matches above, loose matches on Subject: below --
2018-05-03 18:17 Gustavo A. R. Silva
2018-05-03 19:09 ` Randy Dunlap
2018-05-03 19:25   ` David Miller
2018-05-04 16:54 ` David Miller

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).