[Intel-xe] [PATCH 1/4] drm/xe: Ensure that we don't access the placements array out-of-bounds

[Thomas Hellström]

Hi, Matthew, Thanks for reviewing.

On 11/22/23 13:20, Matthew Auld wrote:
> On Wed, 22 Nov 2023 at 11:04, Thomas Hellström
> <thomas.hellstrom at linux.intel.com> wrote:
>> Ensure, using xe_assert that the various try_add_<placement> functions
>> don't access the bo placements array out-of-bounds.
>>
>> Suggested-by: Ohad Sharabi <osharabi at habana.ai>
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/946
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 4305f5cbc2ab..915910b87faa 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -121,9 +121,11 @@ static struct xe_mem_region *res_to_mem_region(struct ttm_resource *res)
>>          return to_xe_ttm_vram_mgr(mgr)->vram;
>>   }
>>
>> -static void try_add_system(struct xe_bo *bo, struct ttm_place *places,
>> -                          u32 bo_flags, u32 *c)
>> +static void try_add_system(struct xe_device *xe, struct xe_bo *bo,
>> +                          struct ttm_place *places, u32 bo_flags, u32 *c)
>>   {
>> +       xe_assert(xe, *c < ARRAY_SIZE(bo->placements));
> Should we also refer to bo->placements directly, and drop the places argument?
Yeah, good point. I'll make a v2.
>
> Anyway,
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>

/Thomas


>
>> +
>>          if (bo_flags & XE_BO_CREATE_SYSTEM_BIT) {
>>                  places[*c] = (struct ttm_place) {
>>                          .mem_type = XE_PL_TT,
>> @@ -172,6 +174,8 @@ static void add_vram(struct xe_device *xe, struct xe_bo *bo,
>>   static void try_add_vram(struct xe_device *xe, struct xe_bo *bo,
>>                           struct ttm_place *places, u32 bo_flags, u32 *c)
>>   {
>> +       xe_assert(xe, *c < ARRAY_SIZE(bo->placements));
>> +
>>          if (bo->props.preferred_gt == XE_GT1) {
>>                  if (bo_flags & XE_BO_CREATE_VRAM1_BIT)
>>                          add_vram(xe, bo, places, bo_flags, XE_PL_VRAM1, c);
>> @@ -188,6 +192,8 @@ static void try_add_vram(struct xe_device *xe, struct xe_bo *bo,
>>   static void try_add_stolen(struct xe_device *xe, struct xe_bo *bo,
>>                             struct ttm_place *places, u32 bo_flags, u32 *c)
>>   {
>> +       xe_assert(xe, *c < ARRAY_SIZE(bo->placements));
>> +
>>          if (bo_flags & XE_BO_CREATE_STOLEN_BIT) {
>>                  places[*c] = (struct ttm_place) {
>>                          .mem_type = XE_PL_STOLEN,
>> @@ -210,11 +216,11 @@ static int __xe_bo_placement_for_flags(struct xe_device *xe, struct xe_bo *bo,
>>          /* The order of placements should indicate preferred location */
>>
>>          if (bo->props.preferred_mem_class == DRM_XE_MEM_REGION_CLASS_SYSMEM) {
>> -               try_add_system(bo, places, bo_flags, &c);
>> +               try_add_system(xe, bo, places, bo_flags, &c);
>>                  try_add_vram(xe, bo, places, bo_flags, &c);
>>          } else {
>>                  try_add_vram(xe, bo, places, bo_flags, &c);
>> -               try_add_system(bo, places, bo_flags, &c);
>> +               try_add_system(xe, bo, places, bo_flags, &c);
>>          }
>>          try_add_stolen(xe, bo, places, bo_flags, &c);
>>
>> --
>> 2.41.0
>>