[PATCH 00/11] fix potential Spectre variant 1 issues

[PATCH 00/11] fix potential Spectre variant 1 issues

[Gustavo A. R. Silva]

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

-- 
2.7.4

[PATCH 00/11] fix potential Spectre variant 1 issues

[Gustavo A. R. Silva]

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

-- 
2.7.4

[PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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: stable@vger.kernel.org
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/usb/tm6000/tm6000-video.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index b2399d4..d701027 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -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"
@@ -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));
 	strlcpy(f->description, format[f->index].name, sizeof(f->description));
 	f->pixelformat = format[f->index].fourcc;
 	return 0;
-- 
2.7.4

[PATCH 02/11] exynos4-is: mipi-csis: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/exynos4-is/mipi-csis.c:549 s5pcsis_enum_mbus_code() warn: potential spectre issue 's5pcsis_formats'

Fix this by sanitizing code->index before using it to index
s5pcsis_formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/exynos4-is/mipi-csis.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index cba46a6..60104c1 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -35,6 +35,8 @@
 
 #include "mipi-csis.h"
 
+#include <linux/nospec.h>
+
 static int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level (0-2)");
@@ -545,7 +547,8 @@ static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	if (code->index >= ARRAY_SIZE(s5pcsis_formats))
 		return -EINVAL;
-
+	code->index = array_index_nospec(code->index,
+					 ARRAY_SIZE(s5pcsis_formats));
 	code->code = s5pcsis_formats[code->index].code;
 	return 0;
 }
-- 
2.7.4

[PATCH 02/11] exynos4-is: mipi-csis: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/exynos4-is/mipi-csis.c:549 s5pcsis_enum_mbus_code() warn: potential spectre issue 's5pcsis_formats'

Fix this by sanitizing code->index before using it to index
s5pcsis_formats.

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 at vger.kernel.org
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/exynos4-is/mipi-csis.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index cba46a6..60104c1 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -35,6 +35,8 @@
 
 #include "mipi-csis.h"
 
+#include <linux/nospec.h>
+
 static int debug;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, "Debug level (0-2)");
@@ -545,7 +547,8 @@ static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	if (code->index >= ARRAY_SIZE(s5pcsis_formats))
 		return -EINVAL;
-
+	code->index = array_index_nospec(code->index,
+					 ARRAY_SIZE(s5pcsis_formats));
 	code->code = s5pcsis_formats[code->index].code;
 	return 0;
 }
-- 
2.7.4

[PATCH 03/11] fsl-viu: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/fsl-viu.c:587 vidioc_enum_fmt() warn: potential spectre issue 'formats'

Fix this by sanitizing f->index before using it to index
formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/fsl-viu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/fsl-viu.c b/drivers/media/platform/fsl-viu.c
index e41510c..8356d26 100644
--- a/drivers/media/platform/fsl-viu.c
+++ b/drivers/media/platform/fsl-viu.c
@@ -33,6 +33,8 @@
 #include <media/v4l2-event.h>
 #include <media/videobuf-dma-contig.h>
 
+#include <linux/nospec.h>
+
 #define DRV_NAME		"fsl_viu"
 #define VIU_VERSION		"0.5.1"
 
@@ -579,12 +581,10 @@ static int vidioc_querycap(struct file *file, void *priv,
 static int vidioc_enum_fmt(struct file *file, void  *priv,
 					struct v4l2_fmtdesc *f)
 {
-	int index = f->index;
-
 	if (f->index >= NUM_FORMATS)
 		return -EINVAL;
-
-	f->pixelformat = formats[index].fourcc;
+	f->index = array_index_nospec(f->index, NUM_FORMATS);
+	f->pixelformat = formats[f->index].fourcc;
 	return 0;
 }
 
-- 
2.7.4

[PATCH 04/11] marvell-ccic: mcam-core: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/marvell-ccic/mcam-core.c:1323 mcam_vidioc_enum_fmt_vid_cap() warn: potential spectre issue 'mcam_formats'

Fix this by sanitizing fmt->index before using it to index
mcam_formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 80670ee..0a54dba 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -33,6 +33,8 @@
 
 #include "mcam-core.h"
 
+#include <linux/nospec.h>
+
 #ifdef MCAM_MODE_VMALLOC
 /*
  * Internal DMA buffer management.  Since the controller cannot do S/G I/O,
@@ -1318,6 +1320,7 @@ static int mcam_vidioc_enum_fmt_vid_cap(struct file *filp,
 {
 	if (fmt->index >= N_MCAM_FMTS)
 		return -EINVAL;
+	fmt->index = array_index_nospec(fmt->index, N_MCAM_FMTS);
 	strlcpy(fmt->description, mcam_formats[fmt->index].desc,
 			sizeof(fmt->description));
 	fmt->pixelformat = mcam_formats[fmt->index].pixelformat;
-- 
2.7.4

[PATCH 05/11] omap_vout: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/omap/omap_vout.c:1062 vidioc_enum_fmt_vid_out() warn: potential spectre issue 'omap_formats'

Fix this by sanitizing index before using it to index
omap_formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/omap/omap_vout.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c
index 5700b78..1a7ec39 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -51,6 +51,8 @@
 #include "omap_voutdef.h"
 #include "omap_vout_vrfb.h"
 
+#include <linux/nospec.h>
+
 MODULE_AUTHOR("Texas Instruments");
 MODULE_DESCRIPTION("OMAP Video for Linux Video out driver");
 MODULE_LICENSE("GPL");
@@ -1059,6 +1061,7 @@ static int vidioc_enum_fmt_vid_out(struct file *file, void *fh,
 	if (index >= NUM_OUTPUT_FORMATS)
 		return -EINVAL;
 
+	index = array_index_nospec(index, NUM_OUTPUT_FORMATS);
 	fmt->flags = omap_formats[index].flags;
 	strlcpy(fmt->description, omap_formats[index].description,
 			sizeof(fmt->description));
-- 
2.7.4

[PATCH 06/11] rcar-v4l2: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/rcar-vin/rcar-v4l2.c:344 rvin_enum_fmt_vid_cap() warn: potential spectre issue 'rvin_formats'

Fix this by sanitizing f->index before using it to index
rvin_formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b479b88..bbfc3b8 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -22,6 +22,8 @@
 
 #include "rcar-vin.h"
 
+#include <linux/nospec.h>
+
 #define RVIN_DEFAULT_FORMAT	V4L2_PIX_FMT_YUYV
 #define RVIN_MAX_WIDTH		2048
 #define RVIN_MAX_HEIGHT		2048
@@ -340,7 +342,7 @@ static int rvin_enum_fmt_vid_cap(struct file *file, void *priv,
 {
 	if (f->index >= ARRAY_SIZE(rvin_formats))
 		return -EINVAL;
-
+	f->index = array_index_nospec(f->index, ARRAY_SIZE(rvin_formats));
 	f->pixelformat = rvin_formats[f->index].fourcc;
 
 	return 0;
-- 
2.7.4

[PATCH 07/11] rcar_drif: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/rcar_drif.c:909 rcar_drif_enum_fmt_sdr_cap() warn: potential spectre issue 'formats'

Fix this by sanitizing f->index before using it to index
formats.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/rcar_drif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index dc7e280..2c21ec2 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -66,6 +66,8 @@
 #include <media/videobuf2-v4l2.h>
 #include <media/videobuf2-vmalloc.h>
 
+#include <linux/nospec.h>
+
 /* DRIF register offsets */
 #define RCAR_DRIF_SITMDR1			0x00
 #define RCAR_DRIF_SITMDR2			0x04
@@ -905,7 +907,7 @@ static int rcar_drif_enum_fmt_sdr_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].pixelformat;
 
 	return 0;
-- 
2.7.4

[PATCH 08/11] sh_vou: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/sh_vou.c:407 sh_vou_enum_fmt_vid_out() warn: potential spectre issue 'vou_fmt'

Fix this by sanitizing fmt->index before using it to index
vou_fmt.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/sh_vou.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/sh_vou.c b/drivers/media/platform/sh_vou.c
index 4dccf29..58d8645 100644
--- a/drivers/media/platform/sh_vou.c
+++ b/drivers/media/platform/sh_vou.c
@@ -30,6 +30,8 @@
 #include <media/videobuf2-v4l2.h>
 #include <media/videobuf2-dma-contig.h>
 
+#include <linux/nospec.h>
+
 /* Mirror addresses are not available for all registers */
 #define VOUER	0
 #define VOUCR	4
@@ -398,6 +400,7 @@ static int sh_vou_enum_fmt_vid_out(struct file *file, void  *priv,
 
 	if (fmt->index >= ARRAY_SIZE(vou_fmt))
 		return -EINVAL;
+	fmt->index = array_index_nospec(fmt->index, ARRAY_SIZE(vou_fmt));
 
 	dev_dbg(vou_dev->v4l2_dev.dev, "%s()\n", __func__);
 
-- 
2.7.4

[PATCH 09/11] vimc-debayer: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/vimc/vimc-debayer.c:182 vimc_deb_enum_mbus_code() warn: potential spectre issue 'vimc_deb_pix_map_list'

Fix this by sanitizing code->index before using it to index
vimc_deb_pix_map_list.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/vimc/vimc-debayer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 6e10b63..bdd96bb 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -24,6 +24,8 @@
 
 #include "vimc-common.h"
 
+#include <linux/nospec.h>
+
 #define VIMC_DEB_DRV_NAME "vimc-debayer"
 
 static unsigned int deb_mean_win_size = 3;
@@ -178,7 +180,8 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd,
 	} else {
 		if (code->index >= ARRAY_SIZE(vimc_deb_pix_map_list))
 			return -EINVAL;
-
+		code->index = array_index_nospec(code->index,
+					    ARRAY_SIZE(vimc_deb_pix_map_list));
 		code->code = vimc_deb_pix_map_list[code->index].code;
 	}
 
-- 
2.7.4

[PATCH 10/11] vivid-sdr-cap: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/vivid/vivid-sdr-cap.c:323 vivid_sdr_enum_freq_bands() warn: potential spectre issue 'bands_adc'

Fix this by sanitizing band->index before using it to index
bands_adc and bands_fm.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/vivid/vivid-sdr-cap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
index cfb7cb4..684d8a2 100644
--- a/drivers/media/platform/vivid/vivid-sdr-cap.c
+++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
@@ -22,6 +22,8 @@
 #include "vivid-ctrls.h"
 #include "vivid-sdr-cap.h"
 
+#include <linux/nospec.h>
+
 /* stream formats */
 struct vivid_format {
 	u32	pixelformat;
@@ -320,11 +322,15 @@ int vivid_sdr_enum_freq_bands(struct file *file, void *fh,
 	case 0:
 		if (band->index >= ARRAY_SIZE(bands_adc))
 			return -EINVAL;
+		band->index = array_index_nospec(band->index,
+						 ARRAY_SIZE(bands_adc));
 		*band = bands_adc[band->index];
 		return 0;
 	case 1:
 		if (band->index >= ARRAY_SIZE(bands_fm))
 			return -EINVAL;
+		band->index = array_index_nospec(band->index,
+						 ARRAY_SIZE(bands_fm));
 		*band = bands_fm[band->index];
 		return 0;
 	default:
-- 
2.7.4

[PATCH 11/11] vsp1_rwpf: fix potential Spectre variant 1

[Gustavo A. R. Silva]

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

Smatch warning:
drivers/media/platform/vsp1/vsp1_rwpf.c:47 vsp1_rwpf_enum_mbus_code() warn: potential spectre issue 'codes'

Fix this by sanitizing code->index before using it to index
codes.

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
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/media/platform/vsp1/vsp1_rwpf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c b/drivers/media/platform/vsp1/vsp1_rwpf.c
index cfd8f19..6e887be 100644
--- a/drivers/media/platform/vsp1/vsp1_rwpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
@@ -13,6 +13,8 @@
 
 #include <media/v4l2-subdev.h>
 
+#include <linux/nospec.h>
+
 #include "vsp1.h"
 #include "vsp1_rwpf.h"
 #include "vsp1_video.h"
@@ -44,6 +46,7 @@ static int vsp1_rwpf_enum_mbus_code(struct v4l2_subdev *subdev,
 	if (code->index >= ARRAY_SIZE(codes))
 		return -EINVAL;
 
+	code->index = array_index_nospec(code->index, ARRAY_SIZE(codes));
 	code->code = codes[code->index];
 
 	return 0;
-- 
2.7.4

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Mon, 23 Apr 2018 12:38:03 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> 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: stable@vger.kernel.org
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
> index b2399d4..d701027 100644
> --- a/drivers/media/usb/tm6000/tm6000-video.c
> +++ b/drivers/media/usb/tm6000/tm6000-video.c
> @@ -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"
> @@ -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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 23 Apr 2018 12:38:03 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> 
>> 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: stable@vger.kernel.org
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>   drivers/media/usb/tm6000/tm6000-video.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
>> index b2399d4..d701027 100644
>> --- a/drivers/media/usb/tm6000/tm6000-video.c
>> +++ b/drivers/media/usb/tm6000/tm6000-video.c
>> @@ -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"
>> @@ -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.
> 
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.

Thanks
--
Gustavo

Re: [PATCH 00/11] fix potential Spectre variant 1 issues

[Gustavo A. R. Silva]

Hi,

Please, drop this series. Further analysis is required as it seems all 
these are False Positives.

Sorry for the noise.

Thanks
--
Gustavo

On 04/23/2018 12:37 PM, Gustavo A. R. Silva wrote:
> 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(-)
> 

[PATCH 00/11] fix potential Spectre variant 1 issues

[Gustavo A. R. Silva]

Hi,

Please, drop this series. Further analysis is required as it seems all 
these are False Positives.

Sorry for the noise.

Thanks
--
Gustavo

On 04/23/2018 12:37 PM, Gustavo A. R. Silva wrote:
> 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(-)
> 

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Mon, 23 Apr 2018 14:11:02 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 23 Apr 2018 12:38:03 -0500
> > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> >   
> >> 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: stable@vger.kernel.org
> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >> ---
> >>   drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
> >> index b2399d4..d701027 100644
> >> --- a/drivers/media/usb/tm6000/tm6000-video.c
> >> +++ b/drivers/media/usb/tm6000/tm6000-video.c
> >> @@ -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"
> >> @@ -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.
> >   
> 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.

> Sorry for the noise and thanks for the feedback.

Anytime.

Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 23 Apr 2018 14:11:02 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> 
>> On 04/23/2018 01:24 PM, Mauro Carvalho Chehab wrote:
>>> Em Mon, 23 Apr 2018 12:38:03 -0500
>>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
>>>    
>>>> 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: stable@vger.kernel.org
>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>>    drivers/media/usb/tm6000/tm6000-video.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
>>>> index b2399d4..d701027 100644
>>>> --- a/drivers/media/usb/tm6000/tm6000-video.c
>>>> +++ b/drivers/media/usb/tm6000/tm6000-video.c
>>>> @@ -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"
>>>> @@ -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.
>>>    
>> 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.
> 

Yeah. I got it.

>> Sorry for the noise and thanks for the feedback.
> 
> Anytime.
> 

Much appreciated. :)

Thanks
--
Gustavo

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Dan Carpenter]

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

On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 23 Apr 2018 12:38:03 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> 
> > 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: stable@vger.kernel.org
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
> > index b2399d4..d701027 100644
> > --- a/drivers/media/usb/tm6000/tm6000-video.c
> > +++ b/drivers/media/usb/tm6000/tm6000-video.c
> > @@ -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"
> > @@ -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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Hi Dan,

Em Tue, 24 Apr 2018 12:35:00 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> 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?

The intent of that comment is to be provocative, in the sense that
people would argue against and point flaws (if any) on my rationale.

As I explained when reviewing this patch, I don't care much if an
automatic tool is saying that there's a vulnerability at the code,
as it could be a false positive. So, what I want at the patch description
is a threat analysis explaining how an algorithm is exploited.

With regards to Spectre, I never tried to write an exploit myself, nor
had to study it in detail in order to mitigate it. So, what I know about
it is what I read on a few places. From the places where I read, the 
boundaries for an array exploit are limited to L1 cache, but,
as I said before, I can be wrong on that.

It will be great to hear to Peter's comment on that, as he knows a
lot more than me about it.
> 
> 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
> 
> On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 23 Apr 2018 12:38:03 -0500
> > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> >   
> > > 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: stable@vger.kernel.org
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > > ---
> > >  drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
> > > index b2399d4..d701027 100644
> > > --- a/drivers/media/usb/tm6000/tm6000-video.c
> > > +++ b/drivers/media/usb/tm6000/tm6000-video.c
> > > @@ -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"
> > > @@ -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  



Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Peter Zijlstra]

On Tue, Apr 24, 2018 at 12:35:00PM +0300, Dan Carpenter wrote:
> On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 23 Apr 2018 12:38:03 -0500
> > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> > > @@ -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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Peter Zijlstra]

On Tue, Apr 24, 2018 at 12:36:09PM +0200, Peter Zijlstra wrote:
> 
> 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.

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Tue, 24 Apr 2018 12:36:09 +0200
Peter Zijlstra <peterz@infradead.org> escreveu:

> On Tue, Apr 24, 2018 at 12:35:00PM +0300, Dan Carpenter wrote:
> > On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 23 Apr 2018 12:38:03 -0500
> > > "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:  
> 
> > > > @@ -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

> On Tue, Apr 24, 2018 at 12:36:09PM +0200, Peter Zijlstra wrote:
> > 
> > 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.

Peter,

Thanks for a comprehensive explanation about that. It now makes more
sense to me.

Yeah, better to apply a fix to avoid the issue with VIDIOC_ENUM_FMT. 

Btw, on almost all media drivers, the implementation for enumerating
the supported formats are the same (and we have a few other VIDOC_ENUM_foo
ioctls that usually do similar stuff): the V4L2 core calls a driver,
with looks into an array, returning the results to the core.

So, a fix like that should likely go to almost all media drivers
(there are a lot of them!), and, for every new one, to take care
to avoid introducing it again during patch review process.

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.


Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Peter Zijlstra]

On Tue, Apr 24, 2018 at 02:47:55PM -0300, Mauro Carvalho Chehab wrote:
> 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.

Sadly no; the whole crux is the array bound check itself. You could
maybe pass around the array size to the core code and then do something
like:

	if (f->index >= f->array_size)
		return -EINVAL;

	f->index = nospec_array_index(f->index, f->array_size);

in generic code, and have all the drivers use f->index as usual, but
even that would be quite a bit of code churn I guess.

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

Hi Mauro,

On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> 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.
> 
>> 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:

diff --git a/drivers/media/pci/cx18/cx18-ioctl.c 
b/drivers/media/pci/cx18/cx18-ioctl.c
index 80b902b..63f4388 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -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;
  }
diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index 1a50ec9..d93cf09 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -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));

diff --git a/drivers/media/pci/tw68/tw68-video.c 
b/drivers/media/pci/tw68/tw68-video.c
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));

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index c3fafa9..281d722 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -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)
@@ -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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Thu, 26 Apr 2018 16:41:56 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> Hi Mauro,
> 
> On 04/23/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> > 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.
> > 
> >> Sorry for the noise and thanks for the feedback.
> > 
> > Anytime.
> > 
> 
> I noticed you changed the status of this series from rejected to new.

Yes.

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

> 
> I can write proper patches for all of them if you agree those are not 
> False Positives:
> 
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c 
> b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b..63f4388 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -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;
>   }
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
> b/drivers/media/pci/saa7134/saa7134-video.c
> index 1a50ec9..d93cf09 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -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));
> 
> diff --git a/drivers/media/pci/tw68/tw68-video.c 
> b/drivers/media/pci/tw68/tw68-video.c
> 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));
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
> b/drivers/media/pci/tw686x/tw686x-video.c
> index c3fafa9..281d722 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -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)
> @@ -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



Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

Hi Mauro,

On 04/26/2018 06:42 PM, Mauro Carvalho Chehab wrote:

>>
>> I noticed you changed the status of this series from rejected to new.
> 
> Yes.
> 
>> 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.
> 

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

I noticed Smatch is no longer reporting them.

Thanks
--
Gustavo

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Mon, 14 May 2018 22:31:37 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> Hi Mauro,
> 
> On 04/26/2018 06:42 PM, Mauro Carvalho Chehab wrote:
> 
> >>
> >> I noticed you changed the status of this series from rejected to new.
> > 
> > Yes.
> > 
> >> 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.
> > 
> 
> 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 <samitolvanen@google.com>
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 <samitolvanen@google.com>
    Signed-off-by: Hans Verkuil <hansverk@cisco.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>



> 
> Thanks
> --
> Gustavo



Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Dan Carpenter]

On Tue, May 15, 2018 at 08:59:53AM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 14 May 2018 22:31:37 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On 04/26/2018 06:42 PM, Mauro Carvalho Chehab wrote:
> > 
> > >>
> > >> I noticed you changed the status of this series from rejected to new.
> > > 
> > > Yes.
> > > 
> > >> 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.
> > > 
> > 
> > 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 <samitolvanen@google.com>
> 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 <samitolvanen@google.com>
>     Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> 

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.

regards,
dan carpenter

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 05/15/2018 09:16 AM, Dan Carpenter wrote:
>>>
>>> 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 <samitolvanen@google.com>
>> 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 <samitolvanen@google.com>
>>      Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>      Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>>

Thanks, Mauro.

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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Tue, 15 May 2018 12:29:10 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> On 05/15/2018 09:16 AM, Dan Carpenter wrote:
> >>>
> >>> 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 <samitolvanen@google.com>
> >> 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 <samitolvanen@google.com>
> >>      Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >>      Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> >>  
> 
> Thanks, Mauro.
> 
> > 
> > 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:

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

> 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;

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

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

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

Here, I'm at this commit:

commit 2f66d40cbf57b0bd581fe75447d2a8625fc7bb1d (origin/master, origin/HEAD)
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Tue May 15 16:35:20 2018 +0300

    db: make call_implies rows unique

Plus the diff below (that won't affect Spectre errors).

Regards,
Mauro

> 
> Thanks
> --
> Gustavo
> 

diff --git a/check_missing_break.c b/check_missing_break.c
index 434b7283fc94..5bba6e919521 100644
--- a/check_missing_break.c
+++ b/check_missing_break.c
@@ -73,7 +73,7 @@ static void print_missing_break(struct expression *expr)
 	last_print_expr = get_switch_expr();
 
 	name = expr_to_var(expr);
-	sm_msg("warn: missing break? reassigning '%s'", name);
+//	sm_msg("warn: missing break? reassigning '%s'", name);
 	free_string(name);
 }
 
diff --git a/smatch_flow.c b/smatch_flow.c
index dc0e78824370..cd72a9ded375 100644
--- a/smatch_flow.c
+++ b/smatch_flow.c
@@ -1005,8 +1005,7 @@ void __split_stmt(struct statement *stmt)
 
 		__bail_on_rest_of_function = 1;
 		final_pass = 1;
-		sm_msg("Function too hairy.  Giving up. %lu seconds",
-		       stop.tv_sec - fn_start_time.tv_sec);
+		sm_msg("__split_smt: function too hairy.  Giving up.");
 		fake_a_return();
 		final_pass = 0;  /* turn off sm_msg() from here */
 		return;
diff --git a/smatch_implied.c b/smatch_implied.c
index 3588816361fe..f3ccd4b6d79e 100644
--- a/smatch_implied.c
+++ b/smatch_implied.c
@@ -594,7 +594,7 @@ static void separate_and_filter(struct sm_state *sm, int comparison, struct rang
 
 	gettimeofday(&time_after, NULL);
 	sec = time_after.tv_sec - time_before.tv_sec;
-	if (sec > 20) {
+	if (sec > 60) {
 		sm->nr_children = 4000;
 		sm_msg("Function too hairy.  Ignoring implications after %d seconds.", sec);
 	}
diff --git a/smatch_slist.c b/smatch_slist.c
index e1eb1b999b2a..2f8ba34a4b9a 100644
--- a/smatch_slist.c
+++ b/smatch_slist.c
@@ -237,12 +237,14 @@ char *alloc_sname(const char *str)
 int out_of_memory(void)
 {
 	/*
-	 * I decided to use 50M here based on trial and error.
+	 * I decided to use 6GB here based on trial and error.
 	 * It works out OK for the kernel and so it should work
 	 * for most other projects as well.
 	 */
-	if (sm_state_counter * sizeof(struct sm_state) >= 100000000)
+	if (sm_state_counter * sizeof(struct sm_state) >= 6000000000) {
+		sm_msg("Out of memory");
 		return 1;
+	}
 	return 0;
 }
 




Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Dan Carpenter]

On Tue, May 15, 2018 at 12:29:10PM -0500, Gustavo A. R. Silva wrote:
> 
> 
> On 05/15/2018 09:16 AM, Dan Carpenter wrote:
> > > > 
> > > > 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 <samitolvanen@google.com>
> > > 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 <samitolvanen@google.com>
> > >      Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> > >      Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> > > 
> 
> Thanks, Mauro.
> 
> > 
> > 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.
> 

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

regards,
dan carpenter

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Dan Carpenter]

On Tue, May 15, 2018 at 04:00:33PM -0300, Mauro Carvalho Chehab wrote:
> Yeah, that's the same I'm getting from media upstream.
> 
> > 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;
> 

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

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

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.

regards,
dan carpenter

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Wed, 16 May 2018 16:11:08 +0300
Dan Carpenter <dan.carpenter@oracle.com> escreveu:

> On Tue, May 15, 2018 at 04:00:33PM -0300, Mauro Carvalho Chehab wrote:
> > Yeah, that's the same I'm getting from media upstream.
> >   
> > > 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;
> >   
> 
> 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....

Yeah, I was thinking that is would be harder to clean this up on
smatch. I proposed a patch to the ML that simplifies the logic,
making easier for both humans and Smatch to better understand how
the arrays are indexed.

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

Thanks!

Regards,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 05/15/2018 02:39 PM, Dan Carpenter wrote:
>> 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.
>>
> 
> 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.

Thanks
--
Gustavo

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 05/16/2018 08:14 PM, Gustavo A. R. Silva wrote:
> 
> 
> On 05/15/2018 02:39 PM, Dan Carpenter wrote:
>>> 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.
>>>
>>
>> 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.
> 

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

--
Gustavo

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Thu, 17 May 2018 05:36:03 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:

> 
> 
> On 05/16/2018 08:14 PM, Gustavo A. R. Silva wrote:
> > 
> > 
> > On 05/15/2018 02:39 PM, Dan Carpenter wrote:

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

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

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Thu, 17 May 2018 08:34:40 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Thu, 17 May 2018 05:36:03 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> escreveu:
> 
> > 
> > 
> > On 05/16/2018 08:14 PM, Gustavo A. R. Silva wrote:
> > > 
> > > 
> > > On 05/15/2018 02:39 PM, Dan Carpenter wrote:
> 
> > >> 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:

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


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



Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Mauro Carvalho Chehab]

Em Thu, 17 May 2018 08:43:24 -0300
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> > > > On 05/15/2018 02:39 PM, Dan Carpenter wrote:  
> >   
> > > >> 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:
> 
> 	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.


Thanks,
Mauro

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 05/17/2018 07:13 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 17 May 2018 08:43:24 -0300
> Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:
> 
>>>>> On 05/15/2018 02:39 PM, Dan Carpenter wrote:
>>>    
>>>>>> 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:
>>
>> 	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.
> 

Yep. I get the same warning multiple times.

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:

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 1310526..7edd9db 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -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)) {


Thanks
--
Gustavo

Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1

[Gustavo A. R. Silva]

On 05/17/2018 01:08 PM, Gustavo A. R. Silva wrote:
> 
> 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:
> 
> diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c 
> b/drivers/media/dvb-core/dvb_ca_en50221.c
> index 1310526..7edd9db 100644
> --- a/drivers/media/dvb-core/dvb_ca_en50221.c
> +++ b/drivers/media/dvb-core/dvb_ca_en50221.c
> @@ -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)) {
> 
> 

Hi Mauro,

Just to let you know, I was running smatch during the weekend and the 
tool is still reporting all these Spectre media warnings (and a lot more):

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

Thanks
--
Gustavo