Re: [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic

[Boris Brezillon <boris.brezillon@collabora.com>]

On Tue, 30 Apr 2024 17:40:54 +0100
Liviu Dudau <liviu.dudau@arm.com> wrote:

> 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@collabora.com>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Tested-by: Eric Smith <eric.smith@collabora.com>
> > ---
> >  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.

Actually, I had the prototype changed to take/return an u32 locally, but
decided to drop it to both keep the amount of changes minimal and keep
prototype consistent with the new helper. I'm fine switching to u32s
though.

> 
> >  {
> > -	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.

The reason I made it a signed value is so we could return an error code
too

-  > 0 => valid
-  < 0 error, with the value encoding the error

though we're likely to always return EINVAL anyway.

> 
> >  	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.

We need to replace that by an extra check on the VA, and given this is
done in two different places, I thought having an helper that does both
the VA to offset and the offset consistency check was simpler. I mean,
I could make this helper return an u32, and consider 0 as the
'no-context-found' special-value, but I can't drop this check.