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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx>
Thanks!
Thanks,
Mauro
