| Message ID | c83b7fffe1e087568f64aba786797d258279948e.1696586632.git.hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: fix smatch warnings | expand |
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@xs4all.nl> > CC: Martin Tuma <martin.tuma@digiteqautomotive.com> > --- > 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.
On Tue, Oct 10, 2023 at 12:31:07PM +0200, Martin Tůma wrote: > 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@xs4all.nl> > > CC: Martin Tuma <martin.tuma@digiteqautomotive.com> > > --- > > 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. I think when there are 2 branch mispredicts this is a valid problem. Below both branches can be trained with a val < 2. Then for a val > 3, both branches can mispredict: video_source_store(buf) { ... ret = kstrtoul(buf, 10, &val); if (ret) return ret; if (val > 3) <------------- predicted as not taken return -EINVAL; ... if (val < MGB4_VIN_DEVICES) <------------- predicted as taken loopin_new = mgbdev->vin[val]; The fix LGTM, although it can also possibly be fixed by masking the index after the first mispredicted branch, like: ret = kstrtoul(buf, 10, &val); if (ret) return ret; + val = array_index_nospec(val, 4); provided, mgbdev->vin[2] and mgbdev->vin[3] can't load a secret.
Hi Martin, On 11/10/2023 02:35, Pawan Gupta wrote: > On Tue, Oct 10, 2023 at 12:31:07PM +0200, Martin Tůma wrote: >> 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@xs4all.nl> >>> CC: Martin Tuma <martin.tuma@digiteqautomotive.com> >>> --- >>> 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. > > I think when there are 2 branch mispredicts this is a valid problem. > Below both branches can be trained with a val < 2. Then for a val > 3, > both branches can mispredict: > > video_source_store(buf) > { > ... > ret = kstrtoul(buf, 10, &val); > if (ret) > return ret; > if (val > 3) <------------- predicted as not taken > return -EINVAL; > ... > > if (val < MGB4_VIN_DEVICES) <------------- predicted as taken > loopin_new = mgbdev->vin[val]; > > The fix LGTM, although it can also possibly be fixed by masking the > index after the first mispredicted branch, like: > > ret = kstrtoul(buf, 10, &val); > if (ret) > return ret; > > + val = array_index_nospec(val, 4); > > provided, mgbdev->vin[2] and mgbdev->vin[3] can't load a secret. Based on that input, and also because I want to shut up that warnings :-), is it OK to merge? Can you proved and Acked-by or Reviewed-by? Thanks! Hans
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@xs4all.nl> > CC: Martin Tuma <martin.tuma@digiteqautomotive.com> > --- > 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); Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>
On 11. 10. 23 2:35, Pawan Gupta wrote: > On Tue, Oct 10, 2023 at 12:31:07PM +0200, Martin Tůma wrote: >> 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@xs4all.nl> >>> CC: Martin Tuma <martin.tuma@digiteqautomotive.com> >>> --- >>> 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. > > I think when there are 2 branch mispredicts this is a valid problem. > Below both branches can be trained with a val < 2. Then for a val > 3, > both branches can mispredict: > > video_source_store(buf) > { > ... > ret = kstrtoul(buf, 10, &val); > if (ret) > return ret; > if (val > 3) <------------- predicted as not taken > return -EINVAL; > ... > > if (val < MGB4_VIN_DEVICES) <------------- predicted as taken > loopin_new = mgbdev->vin[val]; > > The fix LGTM, although it can also possibly be fixed by masking the > index after the first mispredicted branch, like: > > ret = kstrtoul(buf, 10, &val); > if (ret) > return ret; > > + val = array_index_nospec(val, 4); > Ok, thanks for the clarification. > provided, mgbdev->vin[2] and mgbdev->vin[3] can't load a secret. There is nothing secret there. M.
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);
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@xs4all.nl> CC: Martin Tuma <martin.tuma@digiteqautomotive.com> --- drivers/media/pci/mgb4/mgb4_sysfs_out.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)