All of lore.kernel.org
 help / color / mirror / Atom feed
* [v3] usbip: vhci_sysfs: fix potential Spectre v1
@ 2018-05-19  1:13 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-19  1:13 UTC (permalink / raw)
  To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Gustavo A. R. Silva

pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'

Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.

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>
---
Changes in v3:
 - Pass the addresses of pdev_nr and rhport into valid_port and
   valid_args.

Changes in v2:
 - Place the barriers into valid_port.

 drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..be37aec 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,9 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+/* Hardening for Spectre-v1 */
+#include <linux/nospec.h>
+
 #include "usbip_common.h"
 #include "vhci.h"
 
@@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
 	return 0;
 }
 
-static int valid_port(__u32 pdev_nr, __u32 rhport)
+static int valid_port(__u32 *pdev_nr, __u32 *rhport)
 {
-	if (pdev_nr >= vhci_num_controllers) {
-		pr_err("pdev %u\n", pdev_nr);
+	if (*pdev_nr >= vhci_num_controllers) {
+		pr_err("pdev %u\n", *pdev_nr);
 		return 0;
 	}
-	if (rhport >= VHCI_HC_PORTS) {
-		pr_err("rhport %u\n", rhport);
+	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
+
+	if (*rhport >= VHCI_HC_PORTS) {
+		pr_err("rhport %u\n", *rhport);
 		return 0;
 	}
+	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
+
 	return 1;
 }
 
@@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
 	pdev_nr = port_to_pdev_nr(port);
 	rhport = port_to_rhport(port);
 
-	if (!valid_port(pdev_nr, rhport))
+	if (!valid_port(&pdev_nr, &rhport))
 		return -EINVAL;
 
 	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
@@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(detach);
 
-static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
+static int valid_args(__u32 *pdev_nr, __u32 *rhport,
+		      enum usb_device_speed speed)
 {
 	if (!valid_port(pdev_nr, rhport)) {
 		return 0;
@@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 			     sockfd, devid, speed);
 
 	/* check received parameters */
-	if (!valid_args(pdev_nr, rhport, speed))
+	if (!valid_args(&pdev_nr, &rhport, speed))
 		return -EINVAL;
 
 	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1
@ 2018-05-19  1:13 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-19  1:13 UTC (permalink / raw)
  To: Valentina Manea, Shuah Khan, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Gustavo A. R. Silva

pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'

Fix this by sanitizing pdev_nr and rhport before using them to index
vhcis and vhci->vhci_hcd_ss->vdev respectively.

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>
Acked-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>
---
Changes in v3:
 - Pass the addresses of pdev_nr and rhport into valid_port and
   valid_args.

Changes in v2:
 - Place the barriers into valid_port.

 drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..be37aec 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,9 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+/* Hardening for Spectre-v1 */
+#include <linux/nospec.h>
+
 #include "usbip_common.h"
 #include "vhci.h"
 
@@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
 	return 0;
 }
 
-static int valid_port(__u32 pdev_nr, __u32 rhport)
+static int valid_port(__u32 *pdev_nr, __u32 *rhport)
 {
-	if (pdev_nr >= vhci_num_controllers) {
-		pr_err("pdev %u\n", pdev_nr);
+	if (*pdev_nr >= vhci_num_controllers) {
+		pr_err("pdev %u\n", *pdev_nr);
 		return 0;
 	}
-	if (rhport >= VHCI_HC_PORTS) {
-		pr_err("rhport %u\n", rhport);
+	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
+
+	if (*rhport >= VHCI_HC_PORTS) {
+		pr_err("rhport %u\n", *rhport);
 		return 0;
 	}
+	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
+
 	return 1;
 }
 
@@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
 	pdev_nr = port_to_pdev_nr(port);
 	rhport = port_to_rhport(port);
 
-	if (!valid_port(pdev_nr, rhport))
+	if (!valid_port(&pdev_nr, &rhport))
 		return -EINVAL;
 
 	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
@@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(detach);
 
-static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
+static int valid_args(__u32 *pdev_nr, __u32 *rhport,
+		      enum usb_device_speed speed)
 {
 	if (!valid_port(pdev_nr, rhport)) {
 		return 0;
@@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
 			     sockfd, devid, speed);
 
 	/* check received parameters */
-	if (!valid_args(pdev_nr, rhport, speed))
+	if (!valid_args(&pdev_nr, &rhport, speed))
 		return -EINVAL;
 
 	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [v3] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-19  1:13 ` [PATCH v3] " Gustavo A. R. Silva
@ 2018-05-22 15:56 ` Shuah Khan
  -1 siblings, 0 replies; 6+ messages in thread
From: Shuah Khan @ 2018-05-22 15:56 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Valentina Manea, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Shuah Khan

On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
> pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'
> 
> Fix this by sanitizing pdev_nr and rhport before using them to index
> vhcis and vhci->vhci_hcd_ss->vdev respectively.
> 
> 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>
> ---
> Changes in v3:
>  - Pass the addresses of pdev_nr and rhport into valid_port and
>    valid_args.
> 
> Changes in v2:
>  - Place the barriers into valid_port.
> 
>  drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index 4880838..be37aec 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -10,6 +10,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> +/* Hardening for Spectre-v1 */
> +#include <linux/nospec.h>
> +
>  #include "usbip_common.h"
>  #include "vhci.h"
>  
> @@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
>  	return 0;
>  }
>  
> -static int valid_port(__u32 pdev_nr, __u32 rhport)
> +static int valid_port(__u32 *pdev_nr, __u32 *rhport)
>  {
> -	if (pdev_nr >= vhci_num_controllers) {
> -		pr_err("pdev %u\n", pdev_nr);
> +	if (*pdev_nr >= vhci_num_controllers) {
> +		pr_err("pdev %u\n", *pdev_nr);
>  		return 0;
>  	}
> -	if (rhport >= VHCI_HC_PORTS) {
> -		pr_err("rhport %u\n", rhport);
> +	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
> +
> +	if (*rhport >= VHCI_HC_PORTS) {
> +		pr_err("rhport %u\n", *rhport);
>  		return 0;
>  	}
> +	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
> +
>  	return 1;
>  }
>  
> @@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>  	pdev_nr = port_to_pdev_nr(port);
>  	rhport = port_to_rhport(port);
>  
> -	if (!valid_port(pdev_nr, rhport))
> +	if (!valid_port(&pdev_nr, &rhport))
>  		return -EINVAL;
>  
>  	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> @@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(detach);
>  
> -static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
> +static int valid_args(__u32 *pdev_nr, __u32 *rhport,
> +		      enum usb_device_speed speed)
>  {
>  	if (!valid_port(pdev_nr, rhport)) {
>  		return 0;
> @@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  			     sockfd, devid, speed);
>  
>  	/* check received parameters */
> -	if (!valid_args(pdev_nr, rhport, speed))
> +	if (!valid_args(&pdev_nr, &rhport, speed))
>  		return -EINVAL;
>  
>  	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> 

Looks good to me. Thanks for taking care of this.

Acked-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>

-- Shuah
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1
@ 2018-05-22 15:56 ` Shuah Khan
  0 siblings, 0 replies; 6+ messages in thread
From: Shuah Khan @ 2018-05-22 15:56 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Valentina Manea, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Shuah Khan

On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
> pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'
> 
> Fix this by sanitizing pdev_nr and rhport before using them to index
> vhcis and vhci->vhci_hcd_ss->vdev respectively.
> 
> 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>
> ---
> Changes in v3:
>  - Pass the addresses of pdev_nr and rhport into valid_port and
>    valid_args.
> 
> Changes in v2:
>  - Place the barriers into valid_port.
> 
>  drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index 4880838..be37aec 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -10,6 +10,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> +/* Hardening for Spectre-v1 */
> +#include <linux/nospec.h>
> +
>  #include "usbip_common.h"
>  #include "vhci.h"
>  
> @@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
>  	return 0;
>  }
>  
> -static int valid_port(__u32 pdev_nr, __u32 rhport)
> +static int valid_port(__u32 *pdev_nr, __u32 *rhport)
>  {
> -	if (pdev_nr >= vhci_num_controllers) {
> -		pr_err("pdev %u\n", pdev_nr);
> +	if (*pdev_nr >= vhci_num_controllers) {
> +		pr_err("pdev %u\n", *pdev_nr);
>  		return 0;
>  	}
> -	if (rhport >= VHCI_HC_PORTS) {
> -		pr_err("rhport %u\n", rhport);
> +	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
> +
> +	if (*rhport >= VHCI_HC_PORTS) {
> +		pr_err("rhport %u\n", *rhport);
>  		return 0;
>  	}
> +	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
> +
>  	return 1;
>  }
>  
> @@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>  	pdev_nr = port_to_pdev_nr(port);
>  	rhport = port_to_rhport(port);
>  
> -	if (!valid_port(pdev_nr, rhport))
> +	if (!valid_port(&pdev_nr, &rhport))
>  		return -EINVAL;
>  
>  	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> @@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(detach);
>  
> -static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
> +static int valid_args(__u32 *pdev_nr, __u32 *rhport,
> +		      enum usb_device_speed speed)
>  {
>  	if (!valid_port(pdev_nr, rhport)) {
>  		return 0;
> @@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>  			     sockfd, devid, speed);
>  
>  	/* check received parameters */
> -	if (!valid_args(pdev_nr, rhport, speed))
> +	if (!valid_args(&pdev_nr, &rhport, speed))
>  		return -EINVAL;
>  
>  	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> 

Looks good to me. Thanks for taking care of this.

Acked-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>

-- Shuah

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [v3] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-22 15:56 ` [PATCH v3] " Shuah Khan
@ 2018-05-22 16:07 ` Gustavo A. R. Silva
  -1 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-22 16:07 UTC (permalink / raw)
  To: Shuah Khan, Valentina Manea, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel

On 05/22/2018 10:56 AM, Shuah Khan wrote:
> On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
>> pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
>> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'
>>
>> Fix this by sanitizing pdev_nr and rhport before using them to index
>> vhcis and vhci->vhci_hcd_ss->vdev respectively.
>>
>> 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>
>> ---
>> Changes in v3:
>>   - Pass the addresses of pdev_nr and rhport into valid_port and
>>     valid_args.
>>
>> Changes in v2:
>>   - Place the barriers into valid_port.
>>
>>   drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
>> index 4880838..be37aec 100644
>> --- a/drivers/usb/usbip/vhci_sysfs.c
>> +++ b/drivers/usb/usbip/vhci_sysfs.c
>> @@ -10,6 +10,9 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   
>> +/* Hardening for Spectre-v1 */
>> +#include <linux/nospec.h>
>> +
>>   #include "usbip_common.h"
>>   #include "vhci.h"
>>   
>> @@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
>>   	return 0;
>>   }
>>   
>> -static int valid_port(__u32 pdev_nr, __u32 rhport)
>> +static int valid_port(__u32 *pdev_nr, __u32 *rhport)
>>   {
>> -	if (pdev_nr >= vhci_num_controllers) {
>> -		pr_err("pdev %u\n", pdev_nr);
>> +	if (*pdev_nr >= vhci_num_controllers) {
>> +		pr_err("pdev %u\n", *pdev_nr);
>>   		return 0;
>>   	}
>> -	if (rhport >= VHCI_HC_PORTS) {
>> -		pr_err("rhport %u\n", rhport);
>> +	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
>> +
>> +	if (*rhport >= VHCI_HC_PORTS) {
>> +		pr_err("rhport %u\n", *rhport);
>>   		return 0;
>>   	}
>> +	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
>> +
>>   	return 1;
>>   }
>>   
>> @@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>   	pdev_nr = port_to_pdev_nr(port);
>>   	rhport = port_to_rhport(port);
>>   
>> -	if (!valid_port(pdev_nr, rhport))
>> +	if (!valid_port(&pdev_nr, &rhport))
>>   		return -EINVAL;
>>   
>>   	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
>> @@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>   }
>>   static DEVICE_ATTR_WO(detach);
>>   
>> -static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
>> +static int valid_args(__u32 *pdev_nr, __u32 *rhport,
>> +		      enum usb_device_speed speed)
>>   {
>>   	if (!valid_port(pdev_nr, rhport)) {
>>   		return 0;
>> @@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>   			     sockfd, devid, speed);
>>   
>>   	/* check received parameters */
>> -	if (!valid_args(pdev_nr, rhport, speed))
>> +	if (!valid_args(&pdev_nr, &rhport, speed))
>>   		return -EINVAL;
>>   
>>   	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
>>
> 
> Looks good to me. Thanks for taking care of this.
> 

Glad to help. :)

> Acked-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>
> 

Thanks
---
Gustavo
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1
@ 2018-05-22 16:07 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-22 16:07 UTC (permalink / raw)
  To: Shuah Khan, Valentina Manea, Greg Kroah-Hartman; +Cc: linux-usb, linux-kernel



On 05/22/2018 10:56 AM, Shuah Khan wrote:
> On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
>> pdev_nr and rhport can be 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/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_ss->vdev'
>> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre issue 'vhci->vhci_hcd_hs->vdev'
>>
>> Fix this by sanitizing pdev_nr and rhport before using them to index
>> vhcis and vhci->vhci_hcd_ss->vdev respectively.
>>
>> 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>
>> ---
>> Changes in v3:
>>   - Pass the addresses of pdev_nr and rhport into valid_port and
>>     valid_args.
>>
>> Changes in v2:
>>   - Place the barriers into valid_port.
>>
>>   drivers/usb/usbip/vhci_sysfs.c | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
>> index 4880838..be37aec 100644
>> --- a/drivers/usb/usbip/vhci_sysfs.c
>> +++ b/drivers/usb/usbip/vhci_sysfs.c
>> @@ -10,6 +10,9 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   
>> +/* Hardening for Spectre-v1 */
>> +#include <linux/nospec.h>
>> +
>>   #include "usbip_common.h"
>>   #include "vhci.h"
>>   
>> @@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd *vhci_hcd, __u32 rhport)
>>   	return 0;
>>   }
>>   
>> -static int valid_port(__u32 pdev_nr, __u32 rhport)
>> +static int valid_port(__u32 *pdev_nr, __u32 *rhport)
>>   {
>> -	if (pdev_nr >= vhci_num_controllers) {
>> -		pr_err("pdev %u\n", pdev_nr);
>> +	if (*pdev_nr >= vhci_num_controllers) {
>> +		pr_err("pdev %u\n", *pdev_nr);
>>   		return 0;
>>   	}
>> -	if (rhport >= VHCI_HC_PORTS) {
>> -		pr_err("rhport %u\n", rhport);
>> +	*pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
>> +
>> +	if (*rhport >= VHCI_HC_PORTS) {
>> +		pr_err("rhport %u\n", *rhport);
>>   		return 0;
>>   	}
>> +	*rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
>> +
>>   	return 1;
>>   }
>>   
>> @@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>   	pdev_nr = port_to_pdev_nr(port);
>>   	rhport = port_to_rhport(port);
>>   
>> -	if (!valid_port(pdev_nr, rhport))
>> +	if (!valid_port(&pdev_nr, &rhport))
>>   		return -EINVAL;
>>   
>>   	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
>> @@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>   }
>>   static DEVICE_ATTR_WO(detach);
>>   
>> -static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed)
>> +static int valid_args(__u32 *pdev_nr, __u32 *rhport,
>> +		      enum usb_device_speed speed)
>>   {
>>   	if (!valid_port(pdev_nr, rhport)) {
>>   		return 0;
>> @@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>   			     sockfd, devid, speed);
>>   
>>   	/* check received parameters */
>> -	if (!valid_args(pdev_nr, rhport, speed))
>> +	if (!valid_args(&pdev_nr, &rhport, speed))
>>   		return -EINVAL;
>>   
>>   	hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
>>
> 
> Looks good to me. Thanks for taking care of this.
> 

Glad to help. :)

> Acked-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>
> 

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-22 16:07 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-19  1:13 [v3] usbip: vhci_sysfs: fix potential Spectre v1 Gustavo A. R. Silva
2018-05-19  1:13 ` [PATCH v3] " Gustavo A. R. Silva
  -- strict thread matches above, loose matches on Subject: below --
2018-05-22 15:56 [v3] " Shuah Khan
2018-05-22 15:56 ` [PATCH v3] " Shuah Khan
2018-05-22 16:07 [v3] " Gustavo A. R. Silva
2018-05-22 16:07 ` [PATCH v3] " Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.