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.
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);
