Skip to contentSkip to conversation

drm/i915/kvmgt: Fix potential Spectre v1

PATCH In review Gustavo A. R. Silva 3 participants 6 messages Aug 3, 2018 – Aug 7, 2018

Subsystems (3)
DRM drivers Intel i915 Intel GVT-g drivers (Intel GPU Virtualization)
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 drm/i915/kvmgt: Fix potential Spectre v1

info.index 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/gpu/drm/i915/gvt/kvmgt.c:1232 intel_vgpu_ioctl() warn:
potential spectre issue 'vgpu->vdev.region' [r]

Fix this by sanitizing info.index before indirectly using it to index vgpu->vdev.region

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/gpu/drm/i915/gvt/kvmgt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/gvt/kvmgt.c +8 -1
index 4d2f53a..b703f20 100644
@@ -43,6 +43,8 @@
 #include <linux/mdev.h>
 #include <linux/debugfs.h>
 
+#include <linux/nospec.h>
+
 #include "i915_drv.h"
 #include "gvt.h"
 

@@ -1139,7 +1141,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
 		struct vfio_region_info info;
 		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
-		int i, ret;
+		unsigned int i;
+		int ret;
 		struct vfio_region_info_cap_sparse_mmap *sparse = NULL;
 		size_t size;
 		int nr_areas = 1;

@@ -1224,6 +1227,10 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 				if (info.index >= VFIO_PCI_NUM_REGIONS +
 						vgpu->vdev.num_regions)
 					return -EINVAL;
+				info.index =
+					array_index_nospec(info.index,
+							VFIO_PCI_NUM_REGIONS +
+							vgpu->vdev.num_regions);
 
 				i = info.index - VFIO_PCI_NUM_REGIONS;
Signature · 6 lines
2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#1 lore raw

✗ Fi.CI.CHECKPATCH: warning for drm/i915/kvmgt: Fix potential Spectre v1

== Series Details ==

== Series Details ==

Series: drm/i915/kvmgt: Fix potential Spectre v1
URL : https://patchwork.freedesktop.org/series/47640/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
0ea70a869370 drm/i915/kvmgt: Fix potential Spectre v1
-:55: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#55: FILE: drivers/gpu/drm/i915/gvt/kvmgt.c:1232:
+					array_index_nospec(info.index,
+							VFIO_PCI_NUM_REGIONS +

total: 0 errors, 0 warnings, 1 checks, 27 lines checked

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#2 lore raw

✓ Fi.CI.BAT: success for drm/i915/kvmgt: Fix potential Spectre v1

In reply to Gustavo A. R. Silva
== Series Details ==

== Series Details ==

Series: drm/i915/kvmgt: Fix potential Spectre v1
URL : https://patchwork.freedesktop.org/series/47640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4608 -> Patchwork_9845 =

== Summary - SUCCESS ==

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/47640/revisions/1/mbox/

== Known issues ==

Here are the changes found in Patchwork_9845 that come from known issues:

=== IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload-inject:
      fi-hsw-4770r:       PASS -> DMESG-WARN (fdo#107425)

    igt@drv_selftest@live_workarounds:
      {fi-bsw-kefka}:     PASS -> DMESG-FAIL (fdo#107292)
      fi-skl-6700k2:      PASS -> DMESG-FAIL (fdo#107292)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#102614, fdo#106103)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-cnl-psr:         PASS -> DMESG-WARN (fdo#104951)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-bdw-5557u:       DMESG-FAIL (fdo#106560) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
fdo#104951 https://bugs.freedesktop.org/show_bug.cgi?id=104951
fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372
fdo#107425 https://bugs.freedesktop.org/show_bug.cgi?id=107425

== Participating hosts (52 -> 46) ==

Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-u

== Build changes ==

* Linux: CI_DRM_4608 -> Patchwork_9845

CI_DRM_4608: 9d129b43738d0b604a787e54e041973ac7a7c922 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4586: 57caaf440520e397403d898e1d3f1d65ef7b79e2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9845: 0ea70a8693707d7c8195c331511c4d01e5500153 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

0ea70a869370 drm/i915/kvmgt: Fix potential Spectre v1

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9845/issues.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

CI
#3 lore raw

✓ Fi.CI.IGT: success for drm/i915/kvmgt: Fix potential Spectre v1

In reply to Gustavo A. R. Silva
== Series Details ==

== Series Details ==

Series: drm/i915/kvmgt: Fix potential Spectre v1
URL : https://patchwork.freedesktop.org/series/47640/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4608_full -> Patchwork_9845_full =

== Summary - SUCCESS ==

No regressions found.

== Known issues ==

Here are the changes found in Patchwork_9845_full that come from known issues:

=== IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-kbl:          PASS -> FAIL (fdo#106886)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-kbl:          DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@drv_suspend@shrink:
      shard-snb:          INCOMPLETE (fdo#105411, fdo#106886) -> PASS
      shard-hsw:          INCOMPLETE (fdo#103540, fdo#106886) -> PASS

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          INCOMPLETE (fdo#106023, fdo#103665) -> PASS

    igt@kms_flip@flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363, fdo#102887) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

== Participating hosts (5 -> 5) ==

No changes in participating hosts

== Build changes ==

* Linux: CI_DRM_4608 -> Patchwork_9845

CI_DRM_4608: 9d129b43738d0b604a787e54e041973ac7a7c922 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4586: 57caaf440520e397403d898e1d3f1d65ef7b79e2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_9845: 0ea70a8693707d7c8195c331511c4d01e5500153 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9845/shards.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#4 lore raw
In reply to Gustavo A. R. Silva
Quoted 10 lines · Gustavo A. R. Silva

info.index 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/gpu/drm/i915/gvt/kvmgt.c:1232 intel_vgpu_ioctl() warn:
potential spectre issue 'vgpu->vdev.region' [r]

Fix this by sanitizing info.index before indirectly using it to index vgpu->vdev.region

Thanks for catching this! Applied.

Quoted 54 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]>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/gvt/kvmgt.c +8 -1
index 4d2f53a..b703f20 100644
@@ -43,6 +43,8 @@
 #include <linux/mdev.h>
 #include <linux/debugfs.h>
 
+#include <linux/nospec.h>
+
 #include "i915_drv.h"
 #include "gvt.h"
 

@@ -1139,7 +1141,8 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
 		struct vfio_region_info info;
 		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
-		int i, ret;
+		unsigned int i;
+		int ret;
 		struct vfio_region_info_cap_sparse_mmap *sparse = NULL;
 		size_t size;
 		int nr_areas = 1;

@@ -1224,6 +1227,10 @@ static long intel_vgpu_ioctl(struct mdev_device *mdev, unsigned int cmd,
 				if (info.index >= VFIO_PCI_NUM_REGIONS +
 						vgpu->vdev.num_regions)
 					return -EINVAL;
+				info.index =
+					array_index_nospec(info.index,
+							VFIO_PCI_NUM_REGIONS +
+							vgpu->vdev.num_regions);
 
 				i = info.index - VFIO_PCI_NUM_REGIONS;
Signature · 6 lines
2.7.4

_______________________________________________
intel-gvt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
Signature · 3 lines
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#5 lore raw

Hi Zhenyu,

Quoted 6 lines · Zhenyu Wang
Quoted 5 lines · Gustavo A. R. Silva

info.index 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:

Quoted 8 lines
Quoted 5 lines

drivers/gpu/drm/i915/gvt/kvmgt.c:1232 intel_vgpu_ioctl() warn:
potential spectre issue 'vgpu->vdev.region' [r]

Fix this by sanitizing info.index before indirectly using it to index vgpu->vdev.region

Thanks for catching this! Applied.

Glad to help. :)

Thanks
--
Gustavo