[PATCH] drm/amdgpu: Fix potential Spectre vulnerability in amdgpu_gfx_parse_disable_cu()

[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 at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  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);