usbip: vhci_sysfs: fix potential Spectre v1

PATCH Superseded Gustavo A. R. Silva 3 participants 6 messages May 17, 2018 – May 19, 2018

Subsystems (2)

USB USB over IP driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-05-17 20:16 UTC #0 lore raw

PATCH v2 1/1 usbip: vhci_sysfs: fix potential Spectre v1

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 [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Place the barriers into valid_port.

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

drivers/usb/usbip/vhci_sysfs.c +6 -0

index 4880838..69db0c9 100644
@@ -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;
 }

Greg Kroah-Hartman

maintainer

2018-05-18 13:47 UTC #1 lore raw

Quoted 16 lines · 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].

Quoted 7 lines

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Shuah Khan

maintainer

2018-05-18 16:06 UTC #2 lore raw

Quoted 17 lines · Greg Kroah-Hartman

Quoted 16 lines · 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].

Quoted 7 lines

Quoted 7 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
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.

Quoted 4 lines

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.
Quoted 1 line

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

Gustavo A. R. Silva

author

2018-05-18 22:27 UTC #3 lore raw

Quoted 20 lines · Shuah Khan

Quoted 19 lines · Greg Kroah-Hartman
Quoted 18 lines · 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

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
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.
Quoted 4 lines
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.
Quoted 1 line

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Greg Kroah-Hartman

maintainer

2018-05-19 07:04 UTC #4 lore raw

Quoted 45 lines · Gustavo A. R. Silva

Quoted 20 lines · Shuah Khan
Quoted 19 lines · Greg Kroah-Hartman
Quoted 18 lines · 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
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
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.
Quoted 4 lines
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.
Quoted 1 line
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:

Quoted 4 lines

        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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gustavo A. R. Silva

author

2018-05-19 16:53 UTC #5 lore raw

Quoted 9 lines · Greg Kroah-Hartman

Quoted 9 lines
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:

Quoted 9 lines

Quoted 4 lines
         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

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