On 06. 10. 23 12:08, Hans Verkuil wrote:
Fix smatch warnings:
drivers/media/pci/mgb4/mgb4_sysfs_out.c:118 video_source_store() warn: potential spectre issue 'mgbdev->vin' [r] (local cap)
drivers/media/pci/mgb4/mgb4_sysfs_out.c:122 video_source_store() warn: possible spectre second half. 'loopin_new'
Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
CC: Martin Tuma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/media/pci/mgb4/mgb4_sysfs_out.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/pci/mgb4/mgb4_sysfs_out.c b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
index 23a9aabf3915..9f6e81c57726 100644
--- a/drivers/media/pci/mgb4/mgb4_sysfs_out.c
+++ b/drivers/media/pci/mgb4/mgb4_sysfs_out.c
@@ -8,6 +8,7 @@
*/
#include <linux/device.h>
+#include <linux/nospec.h>
#include "mgb4_core.h"
#include "mgb4_i2c.h"
#include "mgb4_vout.h"
@@ -114,8 +115,10 @@ static ssize_t video_source_store(struct device *dev,
if (((config & 0xc) >> 2) < MGB4_VIN_DEVICES)
loopin_old = mgbdev->vin[(config & 0xc) >> 2];
- if (val < MGB4_VIN_DEVICES)
+ if (val < MGB4_VIN_DEVICES) {
+ val = array_index_nospec(val, MGB4_VIN_DEVICES);
loopin_new = mgbdev->vin[val];
+ }
if (loopin_old && loopin_cnt(loopin_old) == 1)
mgb4_mask_reg(&mgbdev->video, loopin_old->config->regs.config,
0x2, 0x0);
Hi,
I had investigated the warning when it appeared here on the mailing
list, but IMHO it is a false-positive, so I didn't react to it.
MGB4_VIN_DEVICES in the condition is not a check for array bounds but a
check whether the input source (val) is one of the inputs. Valid "val"
values are <0,3> and only if the value is <0,1> it is used as an array
index for the input devices (vin) array. So this is IMHO a different
situation than desribed in the speculation document
(https://www.kernel.org/doc/html/latest/staging/speculation.html). But I
may be wrong and array_index_nospec() is still required here.
If the goal was just to silence smatch, than I'm ok with that, It
shouldn't harm any performance here. Except that it will look like there
is a possible spectre issue in the code where it in fact isn't, it
should not change anything.
M.
