LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
@ 2018-05-17 20:16 Gustavo A. R. Silva
  2018-05-18 13:47 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-17 20:16 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 v2:
 - Place the barriers into valid_port.

 drivers/usb/usbip/vhci_sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 4880838..69db0c9 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -10,6 +10,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#include <linux/nospec.h>
+
 #include "usbip_common.h"
 #include "vhci.h"
 
@@ -211,10 +213,14 @@ static int valid_port(__u32 pdev_nr, __u32 rhport)
 		pr_err("pdev %u\n", pdev_nr);
 		return 0;
 	}
+	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;
 }
 
-- 
2.7.4

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

* Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-17 20:16 [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1 Gustavo A. R. Silva
@ 2018-05-18 13:47 ` Greg Kroah-Hartman
  2018-05-18 16:06   ` Shuah Khan
  0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2018-05-18 13:47 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Valentina Manea, Shuah Khan, linux-usb, linux-kernel

On Thu, May 17, 2018 at 03:16:28PM -0500, 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 v2:
>  - Place the barriers into valid_port.

Thanks for the change.  I'll wait for Shuah's ack/review before queueing
this up just as she knows that codebase much better than anyone else.

thanks,

greg k-h

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

* Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-18 13:47 ` Greg Kroah-Hartman
@ 2018-05-18 16:06   ` Shuah Khan
  2018-05-18 22:27     ` Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2018-05-18 16:06 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Greg Kroah-Hartman, Valentina Manea, linux-usb, linux-kernel, Shuah Khan

On 05/18/2018 07:47 AM, Greg Kroah-Hartman wrote:
> On Thu, May 17, 2018 at 03:16:28PM -0500, 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 v2:
>>  - Place the barriers into valid_port.

attach_store() doesn't call valid_port() - can you make the change to
have attach_store() call valid_port() to protect that code path.

> 
> Thanks for the change.  I'll wait for Shuah's ack/review before queueing
> this up just as she knows that codebase much better than anyone else.
> > 

thanks,
-- Shuah

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

* Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-18 16:06   ` Shuah Khan
@ 2018-05-18 22:27     ` Gustavo A. R. Silva
  2018-05-19  7:04       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2018-05-18 22:27 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Shuah Khan, Valentina Manea, linux-usb, linux-kernel



On 05/18/2018 11:06 AM, Shuah Khan wrote:
> On 05/18/2018 07:47 AM, Greg Kroah-Hartman wrote:
>> On Thu, May 17, 2018 at 03:16:28PM -0500, 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 v2:
>>>   - Place the barriers into valid_port.
>  
> attach_store() doesn't call valid_port() - can you make the change to
> have attach_store() call valid_port() to protect that code path.
> 
>>
>> Thanks for the change.  I'll wait for Shuah's ack/review before queueing
>> this up just as she knows that codebase much better than anyone else.
>>>
> 

Greg,

I've been talking with Dan Williams (intel) about this kind of issues 
[1] and it seems my original assumptions are correct. Hence, this patch 
is not useful and, in order to actually prevent speculation here we 
would need to pass the address of pdev_nr and rhport into valid_port, 
otherwise there may be speculation at drivers/usb/usbip/vhci_sysfs.c:235:

         if (!valid_port(pdev_nr, rhport))
                 return -EINVAL;

         hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);


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

Thanks
--
Gustavo

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

* Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1
  2018-05-18 22:27     ` Gustavo A. R. Silva
@ 2018-05-19  7:04       ` Greg Kroah-Hartman
  2018-05-19 16:53         ` Gustavo A. R. Silva
  0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2018-05-19  7:04 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Shuah Khan, Valentina Manea, linux-usb, linux-kernel

On Fri, May 18, 2018 at 05:27:22PM -0500, Gustavo A. R. Silva wrote:
> 
> 
> On 05/18/2018 11:06 AM, Shuah Khan wrote:
> > On 05/18/2018 07:47 AM, Greg Kroah-Hartman wrote:
> > > On Thu, May 17, 2018 at 03:16:28PM -0500, 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 v2:
> > > >   - Place the barriers into valid_port.
> > attach_store() doesn't call valid_port() - can you make the change to
> > have attach_store() call valid_port() to protect that code path.
> > 
> > > 
> > > Thanks for the change.  I'll wait for Shuah's ack/review before queueing
> > > this up just as she knows that codebase much better than anyone else.
> > > > 
> > 
> 
> Greg,
> 
> I've been talking with Dan Williams (intel) about this kind of issues [1]
> and it seems my original assumptions are correct. Hence, this patch is not
> useful and, in order to actually prevent speculation here we would need to
> pass the address of pdev_nr and rhport into valid_port, otherwise there may
> be speculation at drivers/usb/usbip/vhci_sysfs.c:235:
> 
>         if (!valid_port(pdev_nr, rhport))
>                 return -EINVAL;
> 
>         hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);

Ah, yes, sorry, you do need to pass the address through, my mistake
completely.  But the location for the checking is still the right place
to do it, so I was half-right :)

thanks

greg k-h

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

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



On 05/19/2018 02:04 AM, Greg Kroah-Hartman wrote:
>>
>> Greg,
>>
>> I've been talking with Dan Williams (intel) about this kind of issues [1]
>> and it seems my original assumptions are correct. Hence, this patch is not
>> useful and, in order to actually prevent speculation here we would need to
>> pass the address of pdev_nr and rhport into valid_port, otherwise there may
>> be speculation at drivers/usb/usbip/vhci_sysfs.c:235:
>>
>>          if (!valid_port(pdev_nr, rhport))
>>                  return -EINVAL;
>>
>>          hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> 
> Ah, yes, sorry, you do need to pass the address through, my mistake
> completely.  But the location for the checking is still the right place
> to do it, so I was half-right :)
> 

Yep. And that totally make sense.

I already sent v3:

https://marc.info/?l=linux-kernel&m=152669243313887&w=2

Thanks!
--
Gustavo

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

end of thread, other threads:[~2018-05-19 16:53 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-17 20:16 [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1 Gustavo A. R. Silva
2018-05-18 13:47 ` Greg Kroah-Hartman
2018-05-18 16:06   ` Shuah Khan
2018-05-18 22:27     ` Gustavo A. R. Silva
2018-05-19  7:04       ` Greg Kroah-Hartman
2018-05-19 16:53         ` Gustavo A. R. Silva

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