Skip to contentSkip to conversation

atm: zatm: Fix potential Spectre v1

PATCH In review Gustavo A. R. Silva 3 participants 4 messages May 3, 2018 – May 4, 2018

Subsystems (3)
Networking Network drivers ATM
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 atm: zatm: Fix potential Spectre v1

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 [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/atm/zatm.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/atm/zatm.c +3 -0
index 1ef67db..9c9a229 100644
@@ -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) {
Signature · 1 line
2.7.4
#1 lore raw
Quoted 8 lines · 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)

Quoted 8 lines

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.

Quoted 28 lines
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/atm/zatm.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/atm/zatm.c +3 -0
index 1ef67db..9c9a229 100644
@@ -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) {
Signature · 1 line
~Randy
maintainer
#2 lore raw

From: Randy Dunlap <[email protected]>
Date: Thu, 3 May 2018 12:09:40 -0700

Quoted 6 lines

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.

maintainer
#3 lore raw
In reply to Gustavo A. R. Silva

From: "Gustavo A. R. Silva" <[email protected]>
Date: Thu, 3 May 2018 13:17:12 -0500

Quoted 11 lines

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

Quoted 8 lines

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 [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>

Applied and queued up for -stable.