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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Ohad.

On 11/22/23 18:18, Ohad Sharabi wrote:
> On 22/11/2023 13:03, Thomas Hellström 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@habana.ai>
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/946
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Reviewed-by: Ohad Sharabi<osharabi@habana.ai>

Thanks for reviewing. I'll make a v2 of this patch according to Matthew 
Auld's suggestion.

/Thomas


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