Am 01.03.24 um 15:01 schrieb Lazar, Lijo:
On 3/1/2024 6:15 PM, Srinivasan Shanmugam wrote:
The 'mask' array could be used in a way that would make the code
vulnerable to a Spectre attack. The issue is likely related to the fact
that the 'mask' array is being indexed using values that are derived
from user input (the 'se' and 'sh' variables), which could potentially
be manipulated by an attacker.
The array_index_nospec() function is typically used in these situations
where an array index is derived from user input or other untrusted data.
By sanitizing the index, it helps to ensure that even if an attacker can
influence the index, they cannot use this to read sensitive data from
other parts of the array or memory.
The array indices are now sanitized using the array_index_nospec()
function, which ensures that the index cannot be greater than the size
of the array, helping to mitigate Spectre attacks.
The array_index_nospec() function, takes two parameters: the array index
and the maximum size of the array. It ensures that the array index is
within the bounds of the array, i.e., it is less than the maximum size
of the array.
If the array index is within bounds, the function returns the index. If
the index is out of bounds, the function returns a safe index (usually
0) instead. This prevents out-of-bounds reads that could potentially be
exploited in a speculative execution attack.
Reported by smatch:
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:136 amdgpu_gfx_parse_disable_cu() warn: potential spectre issue 'mask' [w]
Fixes: 6f8941a23088 ("drm/amdgpu: add disable_cu parameter")
Cc: Nicolai Hähnle <nicolai.haehnle@xxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 4835d6d899e7..2ef31dbdbc3d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -24,6 +24,7 @@
*/
#include <linux/firmware.h>
+#include <linux/nospec.h>
#include "amdgpu.h"
#include "amdgpu_gfx.h"
#include "amdgpu_rlc.h"
@@ -132,8 +133,9 @@ void amdgpu_gfx_parse_disable_cu(unsigned int *mask, unsigned int max_se, unsign
}
if (se < max_se && sh < max_sh && cu < 16) {
This check is already there which is equivalent to a case like -
int arr[A][B], then check if (i < A && j < B) before accessing arr[i][j].
I think smatch is missing this.
The problem with spectre is that those checks can run speculative and
you can still get parts of the data by looking at cache behavior and
timings after it returned from kernel space with an error message.
That's why you need the explicit call to array_index_nospec() to avoid
security holes from that.
Regards,
Christian.
Thanks,
Lijo
+ unsigned int index = array_index_nospec(se * max_sh + sh, max_se * max_sh);
DRM_INFO("amdgpu: disabling CU %u.%u.%u\n", se, sh, cu);
- mask[se * max_sh + sh] |= 1u << cu;
+ mask[index] |= 1u << cu;
} else {
DRM_ERROR("amdgpu: disable_cu %u.%u.%u is out of range\n",
se, sh, cu);
