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];
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
linux-stable-mirror@lists.linaro.org