On Tue, Apr 30, 2024 at 01:28:52PM +0200, Boris Brezillon wrote:
> ID 0 is reserved to encode 'no-tiler-heap', the heap ID range is
> [1:MAX_HEAPS_PER_POOL], which we occasionally need to turn into an index
> in the [0:MAX_HEAPS_PER_POOL-1] when we want to access the context object.
>
> v2:
> - New patch
>
> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> Reported-by: Eric Smith <eric.smith@xxxxxxxxxxxxx>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Tested-by: Eric Smith <eric.smith@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/panthor/panthor_heap.c | 35 +++++++++++++++++++-------
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 683bb94761bc..b1a7dbf25fb2 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -109,7 +109,11 @@ static int panthor_heap_ctx_stride(struct panthor_device *ptdev)
>
> static int panthor_get_heap_ctx_offset(struct panthor_heap_pool *pool, int id)
Can we make id and the return type here u32? I keep thinking about returning large negative
values here and how they can end up being exploited.
> {
> - return panthor_heap_ctx_stride(pool->ptdev) * id;
> + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range
> + * is [1:MAX_HEAPS_PER_POOL], which we need to turn into a
> + * [0:MAX_HEAPS_PER_POOL-1] context index, hence the minus one here.
> + */
> + return panthor_heap_ctx_stride(pool->ptdev) * (id - 1);
> }
>
> static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id)
> @@ -118,6 +122,21 @@ static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id)
> panthor_get_heap_ctx_offset(pool, id);
> }
>
> +static int panthor_get_heap_ctx_id(struct panthor_heap_pool *pool,
> + u64 heap_ctx_gpu_va)
> +{
> + u64 offset = heap_ctx_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> + u32 heap_idx = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> +
> + if (offset > U32_MAX || heap_idx >= MAX_HEAPS_PER_POOL)
> + return -EINVAL;
> +
> + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range
> + * is [1:MAX_HEAPS_PER_POOL], hence the plus one here.
> + */
> + return heap_idx + 1;
> +}
> +
> static void panthor_free_heap_chunk(struct panthor_vm *vm,
> struct panthor_heap *heap,
> struct panthor_heap_chunk *chunk)
> @@ -364,14 +383,13 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
> u64 heap_gpu_va,
> u64 chunk_gpu_va)
> {
> - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va);
I would keep heap_id here u32. Why do we need to change it? Also, I don't see how
panthor_get_heap_ctx_id() can ever return negative values unless we expect MAX_HEAPS_PER_POOL
to be S32_MAX at some moment.
> struct panthor_heap_chunk *chunk, *tmp, *removed = NULL;
> struct panthor_heap *heap;
> int ret;
>
> - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
> - return -EINVAL;
> + if (heap_id < 0)
> + return heap_id;
This can then be removed if heap_id is u32.
>
> down_read(&pool->lock);
> heap = xa_load(&pool->xa, heap_id);
> @@ -427,14 +445,13 @@ int panthor_heap_grow(struct panthor_heap_pool *pool,
> u32 pending_frag_count,
> u64 *new_chunk_gpu_va)
> {
> - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va);
Again, keep u32 unless you have good reasons ...
> struct panthor_heap_chunk *chunk;
> struct panthor_heap *heap;
> int ret;
>
> - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
> - return -EINVAL;
> + if (heap_id < 0)
> + return heap_id;
... and we will not need this.
Best regards,
Liviu
>
> down_read(&pool->lock);
> heap = xa_load(&pool->xa, heap_id);
> --
> 2.44.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
