fix potential Spectre variant 1 issues

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
 drivers/media/usb/tm6000/tm6000-video.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
 #include <linux/kthread.h>
 #include <linux/highmem.h>
 #include <linux/freezer.h>
+#include <linux/nospec.h>
 
 #include "tm6000-regs.h"
 #include "tm6000.h"

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 35 lines

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
  1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
  #include <linux/kthread.h>
  #include <linux/highmem.h>
  #include <linux/freezer.h>
+#include <linux/nospec.h>
 
  #include "tm6000-regs.h"
  #include "tm6000.h"

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 4 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
  	if (f->index >= ARRAY_SIZE(format))
  		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));

Please enlighten me: how do you think this could be exploited?

When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device, it will just enumerate a hardware functionality, with is constant for a given hardware piece.

The way it works is that userspace do something like:

	int ret = 0;

	for (i = 0; ret == 0; i++) {
		ret = ioctl(VIDIOC_ENUM_FMT, ...);
	}

in order to read an entire const table.

Usually, it doesn't require any special privilege to call this ioctl, but, even if someone changes its permission to 0x400, a simple lsusb output is enough to know what hardware model is there. A lsmod or cat /proc/modules) also tells that the tm6000 module was loaded, with is a very good hint that the tm6000 is there or was there in the past.

In the specific case of tm6000, all hardware supports exactly the same formats, as this is usually defined per-driver. So, a quick look at the driver is enough to know exactly what the ioctl would answer. Also, the net is full of other resources that would allow anyone to get the supported formats for a piece of hardware.

Even assuming that the OS doesn't have lsusb, that /proc is not mounted, that /dev/video0 require special permissions, that the potential attacker doesn't have physical access to the equipment (in order to see if an USB board is plugged), etc... What possible harm he could do by identifying a hardware feature?

Similar notes for the other patches to drivers/media in this
series: let's not just start adding bloatware where not needed.

Please notice that I'm fine if you want to submit potential Spectre variant 1 fixups, but if you're willing to do so, please provide an explanation about the potential threat scenarios that you're identifying at the code.

Dan,

It probably makes sense to have somewhere at smatch a place where we could explicitly mark the false-positives, in order to avoid use to receive patches that would just add an extra delay where it is not needed.

This patchset aims to fix various media platform and media usb cases where we have user controlled array dereferences that could be exploited due to the Spectre variant 1 vulnerability. All were reported by Dan Carpenter.

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

Thanks

Gustavo A. R. Silva (11):
media: tm6000: fix potential Spectre variant 1
exynos4-is: mipi-csis: fix potential Spectre variant 1
fsl-viu: fix potential Spectre variant 1
marvell-ccic: mcam-core: fix potential Spectre variant 1
omap_vout: fix potential Spectre variant 1
rcar-v4l2: fix potential Spectre variant 1
rcar_drif: fix potential Spectre variant 1
sh_vou: fix potential Spectre variant 1
vimc-debayer: fix potential Spectre variant 1
vivid-sdr-cap: fix potential Spectre variant 1
vsp1_rwpf: fix potential Spectre variant 1

  drivers/media/platform/exynos4-is/mipi-csis.c   | 5 ++++-
  drivers/media/platform/fsl-viu.c                | 8 ++++----
  drivers/media/platform/marvell-ccic/mcam-core.c | 3 +++
  drivers/media/platform/omap/omap_vout.c         | 3 +++
  drivers/media/platform/rcar-vin/rcar-v4l2.c     | 4 +++-
  drivers/media/platform/rcar_drif.c              | 4 +++-
  drivers/media/platform/sh_vou.c                 | 3 +++
  drivers/media/platform/vimc/vimc-debayer.c      | 5 ++++-
  drivers/media/platform/vivid/vivid-sdr-cap.c    | 6 ++++++
  drivers/media/platform/vsp1/vsp1_rwpf.c         | 3 +++
  drivers/media/usb/tm6000/tm6000-video.c         | 2 ++
  11 files changed, 38 insertions(+), 8 deletions(-)

Quoted 88 lines

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 35 lines

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
  1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
  #include <linux/kthread.h>
  #include <linux/highmem.h>
  #include <linux/freezer.h>
+#include <linux/nospec.h>
 
  #include "tm6000-regs.h"
  #include "tm6000.h"

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 4 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
  	if (f->index >= ARRAY_SIZE(format))
  		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));  

Please enlighten me: how do you think this could be exploited?

When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device, it will just enumerate a hardware functionality, with is constant for a given hardware piece.

The way it works is that userspace do something like:

	int ret = 0;

	for (i = 0; ret == 0; i++) {
		ret = ioctl(VIDIOC_ENUM_FMT, ...);
	}

in order to read an entire const table.

Usually, it doesn't require any special privilege to call this ioctl, but, even if someone changes its permission to 0x400, a simple lsusb output is enough to know what hardware model is there. A lsmod or cat /proc/modules) also tells that the tm6000 module was loaded, with is a very good hint that the tm6000 is there or was there in the past.

In the specific case of tm6000, all hardware supports exactly the same formats, as this is usually defined per-driver. So, a quick look at the driver is enough to know exactly what the ioctl would answer. Also, the net is full of other resources that would allow anyone to get the supported formats for a piece of hardware.

Even assuming that the OS doesn't have lsusb, that /proc is not mounted, that /dev/video0 require special permissions, that the potential attacker doesn't have physical access to the equipment (in order to see if an USB board is plugged), etc... What possible harm he could do by identifying a hardware feature?

Similar notes for the other patches to drivers/media in this
series: let's not just start adding bloatware where not needed.

Please notice that I'm fine if you want to submit potential Spectre variant 1 fixups, but if you're willing to do so, please provide an explanation about the potential threat scenarios that you're identifying at the code.

Dan,

Quoted 5 lines

It probably makes sense to have somewhere at smatch a place where we could explicitly mark the false-positives, in order to avoid use to receive patches that would just add an extra delay where it is not needed.

I see I've missed some obvious things that you've pointed out here. I'll mark these warnings as False Positives and take your points into account for the analysis of the rest of the Spectre issues reported by Smatch.

Sorry for the noise and thanks for the feedback.

Em Mon, 23 Apr 2018 14:11:02 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 94 lines · Mauro Carvalho Chehab

Quoted 93 lines

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 35 lines

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
   drivers/media/usb/tm6000/tm6000-video.c | 2 ++
   1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
   #include <linux/kthread.h>
   #include <linux/highmem.h>
   #include <linux/freezer.h>
+#include <linux/nospec.h>
 
   #include "tm6000-regs.h"
   #include "tm6000.h"

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 4 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
   	if (f->index >= ARRAY_SIZE(format))
   		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));

Please enlighten me: how do you think this could be exploited?

When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device, it will just enumerate a hardware functionality, with is constant for a given hardware piece.

The way it works is that userspace do something like:

	int ret = 0;

	for (i = 0; ret == 0; i++) {
		ret = ioctl(VIDIOC_ENUM_FMT, ...);
	}

in order to read an entire const table.

Usually, it doesn't require any special privilege to call this ioctl, but, even if someone changes its permission to 0x400, a simple lsusb output is enough to know what hardware model is there. A lsmod or cat /proc/modules) also tells that the tm6000 module was loaded, with is a very good hint that the tm6000 is there or was there in the past.

In the specific case of tm6000, all hardware supports exactly the same formats, as this is usually defined per-driver. So, a quick look at the driver is enough to know exactly what the ioctl would answer. Also, the net is full of other resources that would allow anyone to get the supported formats for a piece of hardware.

Even assuming that the OS doesn't have lsusb, that /proc is not mounted, that /dev/video0 require special permissions, that the potential attacker doesn't have physical access to the equipment (in order to see if an USB board is plugged), etc... What possible harm he could do by identifying a hardware feature?

Similar notes for the other patches to drivers/media in this
series: let's not just start adding bloatware where not needed.

Please notice that I'm fine if you want to submit potential Spectre variant 1 fixups, but if you're willing to do so, please provide an explanation about the potential threat scenarios that you're identifying at the code.

Dan,

It probably makes sense to have somewhere at smatch a place where we could explicitly mark the false-positives, in order to avoid use to receive patches that would just add an extra delay where it is not needed.

Quoted 3 lines

I see I've missed some obvious things that you've pointed out here. I'll mark these warnings as False Positives and take your points into account for the analysis of the rest of the Spectre issues reported by Smatch.

Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org. Please feel free to resubmit any patch if they represent a real threat, adding a corresponding description about the threat scenario at the body of the e-mail.

Quoted 1 line

Sorry for the noise and thanks for the feedback.

Anytime.

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 35 lines

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
 drivers/media/usb/tm6000/tm6000-video.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
 #include <linux/kthread.h>
 #include <linux/highmem.h>
 #include <linux/freezer.h>
+#include <linux/nospec.h>
 
 #include "tm6000-regs.h"
 #include "tm6000.h"

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 4 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 	if (f->index >= ARRAY_SIZE(format))
 		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));

Please enlighten me: how do you think this could be exploited?

When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device, it will just enumerate a hardware functionality, with is constant for a given hardware piece.

The way it works is that userspace do something like:

	int ret = 0;

	for (i = 0; ret == 0; i++) {
		ret = ioctl(VIDIOC_ENUM_FMT, ...);
	}

in order to read an entire const table.

Usually, it doesn't require any special privilege to call this ioctl, but, even if someone changes its permission to 0x400, a simple lsusb output is enough to know what hardware model is there. A lsmod or cat /proc/modules) also tells that the tm6000 module was loaded, with is a very good hint that the tm6000 is there or was there in the past.

In the specific case of tm6000, all hardware supports exactly the same formats, as this is usually defined per-driver. So, a quick look at the driver is enough to know exactly what the ioctl would answer. Also, the net is full of other resources that would allow anyone to get the supported formats for a piece of hardware.

Even assuming that the OS doesn't have lsusb, that /proc is not mounted, that /dev/video0 require special permissions, that the potential attacker doesn't have physical access to the equipment (in order to see if an USB board is plugged), etc... What possible harm he could do by identifying a hardware feature?

Similar notes for the other patches to drivers/media in this
series: let's not just start adding bloatware where not needed.

Please notice that I'm fine if you want to submit potential Spectre variant 1 fixups, but if you're willing to do so, please provide an explanation about the potential threat scenarios that you're identifying at the code.

Dan,

It probably makes sense to have somewhere at smatch a place where we could explicitly mark the false-positives, in order to avoid use to receive patches that would just add an extra delay where it is not needed.

Regards,
Mauro

Hi Mauro,

I saw your comment on LWN. You argue on LWN that since the format array is static the CPU won't speculatively read past the L1 cache?

I don't know if that's true. It should be easy enough to filter out the reads into static arrays. Peter do you know the answer here?

regards,
dan carpenter

Quoted 95 lines · Mauro Carvalho Chehab

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 35 lines

f->index can be controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

Smatch warning:
drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'format'

Fix this by sanitizing f->index before using it to index array _format_

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]

Reported-by Dan Carpenter <[email protected]>

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

---
 drivers/media/usb/tm6000/tm6000-video.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/media/usb/tm6000/tm6000-video.c +1 -0

index b2399d4..d701027 100644
@@ -26,6 +26,7 @@
 #include <linux/kthread.h>
 #include <linux/highmem.h>
 #include <linux/freezer.h>
+#include <linux/nospec.h>
 
 #include "tm6000-regs.h"
 #include "tm6000.h"

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 4 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 	if (f->index >= ARRAY_SIZE(format))
 		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));  

Please enlighten me: how do you think this could be exploited?

When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device, it will just enumerate a hardware functionality, with is constant for a given hardware piece.

The way it works is that userspace do something like:

	int ret = 0;

	for (i = 0; ret == 0; i++) {
		ret = ioctl(VIDIOC_ENUM_FMT, ...);
	}

in order to read an entire const table.

Usually, it doesn't require any special privilege to call this ioctl, but, even if someone changes its permission to 0x400, a simple lsusb output is enough to know what hardware model is there. A lsmod or cat /proc/modules) also tells that the tm6000 module was loaded, with is a very good hint that the tm6000 is there or was there in the past.

In the specific case of tm6000, all hardware supports exactly the same formats, as this is usually defined per-driver. So, a quick look at the driver is enough to know exactly what the ioctl would answer. Also, the net is full of other resources that would allow anyone to get the supported formats for a piece of hardware.

Even assuming that the OS doesn't have lsusb, that /proc is not mounted, that /dev/video0 require special permissions, that the potential attacker doesn't have physical access to the equipment (in order to see if an USB board is plugged), etc... What possible harm he could do by identifying a hardware feature?

Similar notes for the other patches to drivers/media in this
series: let's not just start adding bloatware where not needed.

Please notice that I'm fine if you want to submit potential Spectre variant 1 fixups, but if you're willing to do so, please provide an explanation about the potential threat scenarios that you're identifying at the code.

Dan,

It probably makes sense to have somewhere at smatch a place where we could explicitly mark the false-positives, in order to avoid use to receive patches that would just add an extra delay where it is not needed.

Regards,
Mauro

Quoted 10 lines · Mauro Carvalho Chehab

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 5 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 	if (f->index >= ARRAY_SIZE(format))
 		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));

Please enlighten me: how do you think this could be exploited?

Then usespace probes which part of the descr[] array is now in cache and from that it can infer the initial out-of-bound value.

Quoted 11 lines · Mauro Carvalho Chehab

Quoted 10 lines

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

drivers/media/usb/tm6000/tm6000-video.c static int vidioc_enum_fmt_vid_cap(struct file *file, void *priv, · line 879

Quoted 5 lines

drivers/media/usb/tm6000/tm6000-video.c +1 -0

@@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 	if (f->index >= ARRAY_SIZE(format))
 		return -EINVAL;
 
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(format));  

Please enlighten me: how do you think this could be exploited?

TL;DR: read the papers [1] & [2]

I suspect you didn't get the gist of Spectre V1 [1], let me explain:

Suppose userspace provides f->index > ARRAY_SIZE(format), and we predict the branch to -EINVAL to not be taken.

Then the CPU _WILL_ load (out of bounds) format[f->index] into f->pixelformat and continue onwards to use this bogus value, all the way until it figures out the branch was mis-predicted.

Once it figures out the mispredict, it will throw away the state and start over at the condition site. So far, so basic.

The thing is, is will not (and cannot) throw away all state. Suppose our speculation continues into v4l_fill_fmtdesc() and that switch there is compiled as another array lookup, it will then feed our f->pixelformat (which contains random kernel memory) into that array to find the requested descr pointer.

Now, imagine userspace having flushed cache on the descr pointer array, having trained the branch predictor to mis-predict the branch (see branchscope paper [2]) and doing that out-of-bounds ioctl().

It can then speculative do the out-of-bounds array access, followed by the desc array load, then figure out it was wrong and redo.

Then usespace probes which part of the descr[] array is now in cache and from that it can infer the initial out-of-bound value.

So while format[] is static and bound, it can read random kernel memory up to format+4g, including your crypto keys.

As far as V1 goes, this is actually a fairly solid exploit candidate. No false positive about it.

Now kernel policy is to kill any and all speculation on user controlled array indexing such that we don't have to go look for subsequent side channels (the above cache side channel is the one described in the Spectre paper and by far the easiest, but there are other possible side channels) and we simply don't want to worry about it.

So even from that pov, the proposed patch is good.

[1] https://spectreattack.com/spectre.pdf
[2] www.cs.ucr.edu/~nael/pubs/asplos18.pdf

Quoted 3 lines · Peter Zijlstra

Then usespace probes which part of the descr[] array is now in cache and from that it can infer the initial out-of-bound value.

Just had a better look at v4l_fill_fmtdesc() and actually read the comment. The code cannot be compiled as a array because it is big and sparse. But the log(n) condition tree is a prime candidate for the branchscope side-channel, which would be able to reconstruct a significant number of bits of the original value. A denser tree gives more bits etc.

So, I'm wondering if are there any way to mitigate it inside the core itself, instead of doing it on every driver, e. g. changing v4l_enum_fmt() implementation at v4l2-ioctl.

Ok, a "poor man" approach would be to pass the array directly to the core and let the implementation there to implement the array fetch logic, calling array_index_nospec() there, but I wonder if are there any other way that won't require too much code churn.

Em Mon, 23 Apr 2018 14:11:02 -0500

Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org. Please feel free to resubmit any patch if they represent a real threat, adding a corresponding description about the threat scenario at the body of the e-mail.

Quoted 1 line

Sorry for the noise and thanks for the feedback.

Anytime.

Hi Mauro,

Quoted 7 lines · Mauro Carvalho Chehab

Em Mon, 23 Apr 2018 14:11:02 -0500

Thanks, I 'll mark this series as rejected at patchwork.linuxtv.org. Please feel free to resubmit any patch if they represent a real threat, adding a corresponding description about the threat scenario at the body of the e-mail.

Quoted 4 lines

Quoted 1 line

Sorry for the noise and thanks for the feedback.

Anytime.

I noticed you changed the status of this series from rejected to new.

Also, there are other similar issues in media/pci/

I can write proper patches for all of them if you agree those are not False Positives:

drivers/media/pci/cx18/cx18-ioctl.c +4 -1

index 80b902b..63f4388 100644
@@ -36,6 +36,8 @@
  #include <media/tveeprom.h>
  #include <media/v4l2-event.h>
 
+#include <linux/nospec.h>
+
  u16 cx18_service2vbi(int type)
  {
         switch (type) {

@@ -488,8 +490,9 @@ static int cx18_enum_fmt_vid_cap(struct file *file, 
 void *fh,
                 },
         };
 
-       if (fmt->index > ARRAY_SIZE(formats) - 1)
+       if (fmt->index >= ARRAY_SIZE(formats))
                 return -EINVAL;
+       fmt->index = array_index_nospec(fmt->index, ARRAY_SIZE(formats));
         *fmt = formats[fmt->index];
         return 0;

  }

drivers/media/pci/saa7134/saa7134-video.c +4 -0

index 1a50ec9..d93cf09 100644
@@ -30,6 +30,8 @@
  #include <media/v4l2-event.h>
  #include <media/i2c/saa6588.h>
 
+#include <linux/nospec.h>
+
  /* ------------------------------------------------------------------ */
 
  unsigned int video_debug;

@@ -1819,6 +1821,8 @@ static int saa7134_enum_fmt_vid_cap(struct file 
 *file, void  *priv,
         if (f->index >= FORMATS)
                 return -EINVAL;
 
+       f->index = array_index_nospec(f->index, FORMATS);
+
         strlcpy(f->description, formats[f->index].name,
                 sizeof(f->description));

drivers/media/pci/tw68/tw68-video.c +4 -1

index 8c1f4a0..a6cfb4b 100644
--- a/drivers/media/pci/tw68/tw68-video.c
  #include <media/v4l2-event.h>
  #include <media/videobuf2-dma-sg.h>
 
+#include <linux/nospec.h>
+
  #include "tw68.h"
  #include "tw68-reg.h"

@@ -789,6 +791,8 @@ static int tw68_enum_fmt_vid_cap(struct file *file, 
 void  *priv,
         if (f->index >= FORMATS)
                 return -EINVAL;
 
+       f->index = array_index_nospec(f->index, FORMATS);
+
         strlcpy(f->description, formats[f->index].name,
                 sizeof(f->description));

drivers/media/pci/tw686x/tw686x-video.c +2 -0

index c3fafa9..281d722 100644
@@ -25,6 +25,8 @@
  #include "tw686x.h"
  #include "tw686x-regs.h"
 
+#include <linux/nospec.h>
+
  #define TW686X_INPUTS_PER_CH           4
  #define TW686X_VIDEO_WIDTH             720
  #define TW686X_VIDEO_HEIGHT(id)                ((id & V4L2_STD_525_60) 

? 480 : 576)

drivers/media/pci/tw686x/tw686x-video.c +1 -0

@@ -981,6 +983,7 @@ static int tw686x_enum_fmt_vid_cap(struct file 
 *file, void *priv,
  {
         if (f->index >= ARRAY_SIZE(formats))
                 return -EINVAL;
+       f->index = array_index_nospec(f->index, ARRAY_SIZE(formats));
         f->pixelformat = formats[f->index].fourcc;
         return 0;

  }

Thanks
--
Gustavo

Quoted 2 lines

I noticed you changed the status of this series from rejected to new.

Yes.

Quoted 1 line

Also, there are other similar issues in media/pci/

Well, the issues will be there everywhere on all media drivers.

I marked your patches because I need to study it carefully, after Peter's explanations. My plan is to do it next week. Still not sure if the approach you took is the best one or not.

As I said, one possibility is to change the way v4l2-core handles VIDIOC_ENUM_foo ioctls, but that would be make harder to -stable backports.

I need a weekend to sleep on it.

Hi Mauro,

Quoted 17 lines · Mauro Carvalho Chehab

Quoted 2 lines

I noticed you changed the status of this series from rejected to new.

Yes.

Quoted 1 line

Also, there are other similar issues in media/pci/

Well, the issues will be there everywhere on all media drivers.

I marked your patches because I need to study it carefully, after Peter's explanations. My plan is to do it next week. Still not sure if the approach you took is the best one or not.

As I said, one possibility is to change the way v4l2-core handles VIDIOC_ENUM_foo ioctls, but that would be make harder to -stable backports.

Quoted 2 lines

I need a weekend to sleep on it.

I'm curious about how you finally resolved to handle these issues.

I noticed Smatch is no longer reporting them.

Thanks
--
Gustavo

Em Mon, 14 May 2018 22:31:37 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 21 lines

Hi Mauro,

Quoted 17 lines · Mauro Carvalho Chehab

Quoted 2 lines

I noticed you changed the status of this series from rejected to new.

Yes.

Quoted 1 line

Also, there are other similar issues in media/pci/

Well, the issues will be there everywhere on all media drivers.

I marked your patches because I need to study it carefully, after Peter's explanations. My plan is to do it next week. Still not sure if the approach you took is the best one or not.

As I said, one possibility is to change the way v4l2-core handles VIDIOC_ENUM_foo ioctls, but that would be make harder to -stable backports.

Quoted 6 lines

Quoted 2 lines

I need a weekend to sleep on it.

I'm curious about how you finally resolved to handle these issues.

I noticed Smatch is no longer reporting them.

There was no direct fix for it, but maybe this patch has something to do with the smatch error report cleanup:

commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56

Author: Sami Tolvanen <[email protected]>
Date:   Tue May 8 13:56:12 2018 -0400

media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions

This change removes IOCTL_INFO_STD and adds stub functions where needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call mismatches with Control-Flow Integrity, caused by calling standard ioctls using a function pointer that doesn't match the function type.

Signed-off-by Sami Tolvanen <[email protected]>

Signed-off-by Hans Verkuil <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 19 lines

Quoted 4 lines

I'm curious about how you finally resolved to handle these issues.

I noticed Smatch is no longer reporting them.

There was no direct fix for it, but maybe this patch has something to do with the smatch error report cleanup:

commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56

Author: Sami Tolvanen <[email protected]>
Date:   Tue May 8 13:56:12 2018 -0400

media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions

This change removes IOCTL_INFO_STD and adds stub functions where needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call mismatches with Control-Flow Integrity, caused by calling standard ioctls using a function pointer that doesn't match the function type.

Quoted 4 lines

Signed-off-by Sami Tolvanen <[email protected]>

Signed-off-by Hans Verkuil <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Possibly... There was an ancient bug in Smatch's function pointer handling. I just pushed a fix for it now so the warning is there on linux-next.

Quoted 19 lines

Quoted 19 lines

Quoted 4 lines

I'm curious about how you finally resolved to handle these issues.

I noticed Smatch is no longer reporting them.

There was no direct fix for it, but maybe this patch has something to do with the smatch error report cleanup:

commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56

Author: Sami Tolvanen <[email protected]>
Date:   Tue May 8 13:56:12 2018 -0400

media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions

This change removes IOCTL_INFO_STD and adds stub functions where needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call mismatches with Control-Flow Integrity, caused by calling standard ioctls using a function pointer that doesn't match the function type.

Quoted 4 lines

Quoted 4 lines

Signed-off-by Sami Tolvanen <[email protected]>

Signed-off-by Hans Verkuil <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Thanks, Mauro.

Quoted 5 lines

Possibly... There was an ancient bug in Smatch's function pointer handling. I just pushed a fix for it now so the warning is there on linux-next.

Dan,

These are all the Spectre media issues I see smatch is reporting in linux-next-20180515:

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args'

drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write()
warn: potential spectre issue 'ca->slot_info' (local cap)

drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

I pulled the latest changes from the smatch repository and compiled it.

I'm running smatch v0.5.0-4459-g2f66d40 now. Is this the latest version?

I wonder if there is anything I might be missing.

Thanks
--
Gustavo

Quoted 12 lines · Dan Carpenter

Quoted 12 lines

Quoted 4 lines

I'm curious about how you finally resolved to handle these issues.

I noticed Smatch is no longer reporting them.

There was no direct fix for it, but maybe this patch has something to do with the smatch error report cleanup:

commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56

Author: Sami Tolvanen <[email protected]>
Date:   Tue May 8 13:56:12 2018 -0400

Quoted 9 lines

Quoted 9 lines

media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions This change removes IOCTL_INFO_STD and adds stub functions where needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call mismatches with Control-Flow Integrity, caused by calling standard ioctls using a function pointer that doesn't match the function type.

Signed-off-by Sami Tolvanen <[email protected]>

Signed-off-by Hans Verkuil <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Thanks, Mauro.

Quoted 5 lines

Possibly... There was an ancient bug in Smatch's function pointer handling. I just pushed a fix for it now so the warning is there on linux-next.

Dan,

These are all the Spectre media issues I see smatch is reporting in linux-next-20180515:

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args' drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() warn:
potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

I pulled the latest changes from the smatch repository and compiled it.

I'm running smatch v0.5.0-4459-g2f66d40 now. Is this the latest version?

I wonder if there is anything I might be missing.

Yeah, that's the same I'm getting from media upstream.

Quoted 2 lines

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args'

This one seems a false positive, as the index var is u8 and the array has 256 elements, as the userspace input from 'op' is initialized with:

	u8 v;
	u32 op;

	if (!kstrtou8(token, 0, &v))
		op = v;

Quoted 2 lines

drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write()
warn: potential spectre issue 'ca->slot_info' (local cap)

This one seems a real issue to me. Sent a patch for it.

Quoted 2 lines

drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

I failed to see what's wrong here, or if this is exploited.

Quoted 9 lines

Yeah, that's the same I'm getting from media upstream.

Quoted 2 lines

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args'

This one seems a false positive, as the index var is u8 and the array has 256 elements, as the userspace input from 'op' is initialized with:

Quoted 6 lines

	u8 v;
	u32 op;

	if (!kstrtou8(token, 0, &v))
		op = v;

It's hard to silence this because Smatch stores the current user controlled range list, not what it was initially. I wrote all this code to detect bounds checking errors, so there wasn't any need to save the range list before the bounds check. Since "op" is a u32, I can't even go by the type of the index....

Quoted 9 lines

Quoted 3 lines

drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write()
warn: potential spectre issue 'ca->slot_info' (local cap)

This one seems a real issue to me. Sent a patch for it.

Quoted 2 lines

drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

Quoted 1 line

I failed to see what's wrong here, or if this is exploited.

Oh... Huh. This is a bug in smatch. That line looks like:

	p->ule_sndu_type = ntohs(*(__be16 *)(p->ule_next_hdr + ((p->ule_dbit ? 2 : 3) * ETH_ALEN)));

Smatch see the ntohs() and marks everything inside it as untrusted network data. I'll fix this.

Quoted 12 lines

Dan,

These are all the Spectre media issues I see smatch is reporting in linux-next-20180515:

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args' drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() warn:
potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

Quoted 6 lines

I pulled the latest changes from the smatch repository and compiled it.

I'm running smatch v0.5.0-4459-g2f66d40 now. Is this the latest version?

I wonder if there is anything I might be missing.

You'd need to rebuild the db (possibly twice but definitely once).

Quoted 12 lines · Dan Carpenter

Quoted 12 lines

Dan,

These are all the Spectre media issues I see smatch is reporting in linux-next-20180515:

drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args' drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() warn:
potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr'

Quoted 9 lines

Quoted 6 lines

I pulled the latest changes from the smatch repository and compiled it.

I'm running smatch v0.5.0-4459-g2f66d40 now. Is this the latest version?

I wonder if there is anything I might be missing.

You'd need to rebuild the db (possibly twice but definitely once).

Hi Dan,

After rebuilding the db (once), these are all the Spectre media warnings I get:

drivers/media/pci/ddbridge/ddbridge-core.c:233 ddb_redirect() warn: potential spectre issue 'ddbs' drivers/media/pci/ddbridge/ddbridge-core.c:243 ddb_redirect() warn: potential spectre issue 'pdev->port' drivers/media/pci/ddbridge/ddbridge-core.c:252 ddb_redirect() warn: potential spectre issue 'idev->input' drivers/media/dvb-core/dvb_ca_en50221.c:1400 dvb_ca_en50221_io_do_ioctl() warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn: potential spectre issue 'p->ule_next_hdr' drivers/media/dvb-core/dvb_net.c:1483 dvb_net_do_ioctl() warn: potential spectre issue 'dvbnet->device' (local cap) drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line() warn: potential spectre issue 'pin->error_inj_args'

I just want to double check if you are getting the same output. In case you are getting the same, then what Mauro commented about these issues:

https://patchwork.linuxtv.org/project/linux-media/list/?submitter=7277

being resolved by commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56 seems to be correct.

Quoted 5 lines · Dan Carpenter

Quoted 2 lines

You'd need to rebuild the db (possibly twice but definitely once).

Quoted 28 lines

Quoted 1 line

Hi Dan,

After rebuilding the db (once), these are all the Spectre media warnings I get:

drivers/media/pci/ddbridge/ddbridge-core.c:233 ddb_redirect() warn:
potential spectre issue 'ddbs' drivers/media/pci/ddbridge/ddbridge-core.c:243 ddb_redirect() warn:
potential spectre issue 'pdev->port' drivers/media/pci/ddbridge/ddbridge-core.c:252 ddb_redirect() warn:
potential spectre issue 'idev->input' drivers/media/dvb-core/dvb_ca_en50221.c:1400 dvb_ca_en50221_io_do_ioctl() warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write()
warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr' drivers/media/dvb-core/dvb_net.c:1483 dvb_net_do_ioctl() warn: potential spectre issue 'dvbnet->device' (local cap) drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args'

I just want to double check if you are getting the same output. In case you are getting the same, then what Mauro commented about these issues:

Quoted 5 lines

https://patchwork.linuxtv.org/project/linux-media/list/?submitter=7277

being resolved by commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56 seems to be correct.

Interesting, I've rebuild the db twice and now I get a total of 75 Spectre warnings in drivers/media

Em Thu, 17 May 2018 05:36:03 -0500
"Gustavo A. R. Silva" <[email protected]> escreveu:

Quoted 5 lines · Gustavo A. R. Silva

Quoted 2 lines

Quoted 3 lines

Quoted 3 lines · Dan Carpenter

Quoted 2 lines

You'd need to rebuild the db (possibly twice but definitely once).

How? Here, I just pull from your git tree and do a "make". At most, make clean; make.

Quoted 35 lines

Quoted 34 lines

Quoted 2 lines

Hi Dan,

After rebuilding the db (once), these are all the Spectre media warnings I get:

drivers/media/pci/ddbridge/ddbridge-core.c:233 ddb_redirect() warn:
potential spectre issue 'ddbs' drivers/media/pci/ddbridge/ddbridge-core.c:243 ddb_redirect() warn:
potential spectre issue 'pdev->port' drivers/media/pci/ddbridge/ddbridge-core.c:252 ddb_redirect() warn:
potential spectre issue 'idev->input' drivers/media/dvb-core/dvb_ca_en50221.c:1400 dvb_ca_en50221_io_do_ioctl() warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write()
warn: potential spectre issue 'ca->slot_info' (local cap) drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn:
potential spectre issue 'p->ule_next_hdr' drivers/media/dvb-core/dvb_net.c:1483 dvb_net_do_ioctl() warn: potential spectre issue 'dvbnet->device' (local cap) drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line()
warn: potential spectre issue 'pin->error_inj_args'

I just want to double check if you are getting the same output. In case you are getting the same, then what Mauro commented about these issues:

https://patchwork.linuxtv.org/project/linux-media/list/?submitter=7277

being resolved by commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56 seems to be correct.

Quoted 2 lines

Interesting, I've rebuild the db twice and now I get a total of 75 Spectre warnings in drivers/media

That makes more sense to me, as the same pattern is used by almost all VIDIOC_ENUM_foo ioctls.

Thanks,
Mauro

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines · Dan Carpenter

Quoted 2 lines

You'd need to rebuild the db (possibly twice but definitely once).

Quoted 2 lines

How? Here, I just pull from your git tree and do a "make". At most, make clean; make.

Never mind. Found it using grep. I'm running this:

make allyesconfig /devel/smatch/smatch_scripts/build_kernel_data.sh /devel/smatch/smatch_scripts/build_kernel_data.sh

Em Thu, 17 May 2018 08:43:24 -0300
Mauro Carvalho Chehab <[email protected]> escreveu:

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines · Dan Carpenter

Quoted 2 lines

You'd need to rebuild the db (possibly twice but definitely once).

How? Here, I just pull from your git tree and do a "make". At most, make clean; make.

Never mind. Found it using grep. I'm running this:

Quoted 3 lines

make allyesconfig /devel/smatch/smatch_scripts/build_kernel_data.sh /devel/smatch/smatch_scripts/build_kernel_data.sh

It seems that something is broken... getting this error/warning:

DBD::SQLite::db do failed: unrecognized token: "'end + strlen("
" at /devel/smatch/smatch_scripts/../smatch_data/db/fill_db_sql.pl line 32, <WARNS> line 2938054.

BTW, Mauro, you sent a patch to fix an spectre v1 issue in this file yesterday: dvb_ca_en50221.c:1480, but it seems there is another instance of the same issue some lines above:

drivers/media/dvb-core/dvb_ca_en50221.c +1 -0

index 1310526..7edd9db 100644
@@ -1398,6 +1398,7 @@ static int dvb_ca_en50221_io_do_ioctl(struct file 
 *file,
 
                 info->type = CA_CI_LINK;
                 info->flags = 0;
+               slot = array_index_nospec(slot, ca->slot_count + 1);
                 sl = &ca->slot_info[slot];
                 if ((sl->slot_state != DVB_CA_SLOTSTATE_NONE) &&

                    (sl->slot_state != DVB_CA_SLOTSTATE_INVALID)) {