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

[Lazar, Lijo]

On 3/1/2024 7:52 PM, Christian König wrote:
> 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 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.
> 
> 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.
> 

Thanks, found the doc -
https://www.kernel.org/doc/Documentation/speculation.txt

Thanks,
Lijo

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