| Message ID | 4675c8e726c7d55bbecf9f4772370daa8b46ccd3.1729074076.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media: fix several issues on drivers | expand |
On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote: > Frequency range is set from sysfs via frequency_range_store(), > being vulnerable to spectre, as reported by smatch: > > drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r] > drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half. 'reg_set' > > Fix it. > > Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver") > Cc: stable@vger.kernel.org > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > --- > drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c > index 70dc78ef193c..a25b68403bc6 100644 > --- a/drivers/media/pci/mgb4/mgb4_cmt.c > +++ b/drivers/media/pci/mgb4/mgb4_cmt.c > @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, > u32 config; > size_t i; > > + freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in)); > + > addr = cmt_addrs_in[vindev->config->id]; > reg_set = cmt_vals_in[freq_range]; > I still do not fully understand the exact vulnerability here, but the patch should definitely not do any harm, so I'm ok with it even if it's real purpose would only be to silence the smatch warning :-) Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com>
Em Wed, 16 Oct 2024 13:59:18 +0200 Martin Tůma <tumic@gpxsee.org> escreveu: > On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote: > > Frequency range is set from sysfs via frequency_range_store(), > > being vulnerable to spectre, as reported by smatch: > > > > drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r] > > drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half. 'reg_set' > > > > Fix it. > > > > Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver") > > Cc: stable@vger.kernel.org > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > --- > > drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c > > index 70dc78ef193c..a25b68403bc6 100644 > > --- a/drivers/media/pci/mgb4/mgb4_cmt.c > > +++ b/drivers/media/pci/mgb4/mgb4_cmt.c > > @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, > > u32 config; > > size_t i; > > > > + freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in)); > > + > > addr = cmt_addrs_in[vindev->config->id]; > > reg_set = cmt_vals_in[freq_range]; > > > > I still do not fully understand the exact vulnerability here, but the > patch should definitely not do any harm, so I'm ok with it even if it's > real purpose would only be to silence the smatch warning :-) With Spectre, just checking if freq_range is between 0 and the size of the array is not enough, as malicious code could use CPU speculative logic to retrieve data from memory outside the limits of the array. As freq_range is specified by the user via sysfs attribute frequency_range, it is subject to Spectre v1 attack as described at Documentation/admin-guide/hw-vuln/spectre.rst. Silencing smatch is a plus. > > Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com> Thanks! Thanks, Mauro
Hi, On 18. 10. 24 6:32 dop., Mauro Carvalho Chehab wrote: > Em Wed, 16 Oct 2024 13:59:18 +0200 > Martin Tůma <tumic@gpxsee.org> escreveu: > >> On 16. 10. 24 12:22 odp., Mauro Carvalho Chehab wrote: >>> Frequency range is set from sysfs via frequency_range_store(), >>> being vulnerable to spectre, as reported by smatch: >>> >>> drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r] >>> drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half. 'reg_set' >>> >>> Fix it. >>> >>> Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> >>> --- >>> drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c >>> index 70dc78ef193c..a25b68403bc6 100644 >>> --- a/drivers/media/pci/mgb4/mgb4_cmt.c >>> +++ b/drivers/media/pci/mgb4/mgb4_cmt.c >>> @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, >>> u32 config; >>> size_t i; >>> >>> + freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in)); >>> + >>> addr = cmt_addrs_in[vindev->config->id]; >>> reg_set = cmt_vals_in[freq_range]; >>> >> >> I still do not fully understand the exact vulnerability here, but the >> patch should definitely not do any harm, so I'm ok with it even if it's >> real purpose would only be to silence the smatch warning :-) > > With Spectre, just checking if freq_range is between 0 and the > size of the array is not enough, as malicious code could use CPU > speculative logic to retrieve data from memory outside the limits > of the array. > > As freq_range is specified by the user via sysfs attribute > frequency_range, it is subject to Spectre v1 attack as described > at Documentation/admin-guide/hw-vuln/spectre.rst. > I do know the general theory about the "spectre bounds check fix/workaround", what I was referring is this part in the documentation: "Such speculative memory accesses can leave side effects, creating side channels which leak information to the attacker." I do not see/understand the exact "information leak" that could happen here on this particular place. But as already stated in the original answer, I don't have to understand everything ;-) M. > Silencing smatch is a plus. > >> >> Reviewed-by: Martin Tůma <martin.tuma@digiteqautomotive.com> > > Thanks! > > Thanks, > Mauro >
diff --git a/drivers/media/pci/mgb4/mgb4_cmt.c b/drivers/media/pci/mgb4/mgb4_cmt.c index 70dc78ef193c..a25b68403bc6 100644 --- a/drivers/media/pci/mgb4/mgb4_cmt.c +++ b/drivers/media/pci/mgb4/mgb4_cmt.c @@ -227,6 +227,8 @@ void mgb4_cmt_set_vin_freq_range(struct mgb4_vin_dev *vindev, u32 config; size_t i; + freq_range = array_index_nospec(freq_range, ARRAY_SIZE(cmt_vals_in)); + addr = cmt_addrs_in[vindev->config->id]; reg_set = cmt_vals_in[freq_range];
Frequency range is set from sysfs via frequency_range_store(), being vulnerable to spectre, as reported by smatch: drivers/media/pci/mgb4/mgb4_cmt.c:231 mgb4_cmt_set_vin_freq_range() warn: potential spectre issue 'cmt_vals_in' [r] drivers/media/pci/mgb4/mgb4_cmt.c:238 mgb4_cmt_set_vin_freq_range() warn: possible spectre second half. 'reg_set' Fix it. Fixes: 0ab13674a9bd ("media: pci: mgb4: Added Digiteq Automotive MGB4 driver") Cc: stable@vger.kernel.org Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/pci/mgb4/mgb4_cmt.c | 2 ++ 1 file changed, 2 insertions(+)