From: Liviu Dudau <liviu.dudau@arm.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: "Steven Price" <steven.price@arm.com>,
"Adrián Larumbe" <adrian.larumbe@collabora.com>,
dri-devel@lists.freedesktop.org, kernel@collabora.com,
"Eric Smith" <eric.smith@collabora.com>
Subject: Re: [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic
Date: Tue, 30 Apr 2024 17:40:54 +0100 [thread overview]
Message-ID: <ZjEfFiT7k_1y3agC@e110455-lin.cambridge.arm.com> (raw)
In-Reply-To: <20240430112852.486424-5-boris.brezillon@collabora.com>
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.
> {
> - 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! /
---------------
¯\_(ツ)_/¯
next prev parent reply other threads:[~2024-04-30 16:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 11:28 [PATCH v2 0/4] drm/panthor: Collection of tiler heap related fixes Boris Brezillon
2024-04-30 11:28 ` [PATCH v2 1/4] drm/panthor: Fix tiler OOM handling to allow incremental rendering Boris Brezillon
2024-04-30 15:27 ` Liviu Dudau
2024-05-02 14:03 ` Steven Price
2024-04-30 11:28 ` [PATCH v2 2/4] drm/panthor: Make sure the tiler initial/max chunks are consistent Boris Brezillon
2024-04-30 15:31 ` Liviu Dudau
2024-05-02 14:03 ` Steven Price
2024-04-30 11:28 ` [PATCH v2 3/4] drm/panthor: Relax the constraints on the tiler chunk size Boris Brezillon
2024-04-30 13:08 ` Adrián Larumbe
2024-05-02 14:03 ` Steven Price
2024-04-30 16:10 ` Liviu Dudau
2024-04-30 11:28 ` [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic Boris Brezillon
2024-04-30 16:40 ` Liviu Dudau [this message]
2024-04-30 17:07 ` Boris Brezillon
2024-05-02 14:03 ` Steven Price
2024-05-02 14:15 ` Boris Brezillon
2024-05-02 14:26 ` Steven Price
2024-05-02 14:36 ` Boris Brezillon
2024-05-02 14:47 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjEfFiT7k_1y3agC@e110455-lin.cambridge.arm.com \
--to=liviu.dudau@arm.com \
--cc=adrian.larumbe@collabora.com \
--cc=boris.brezillon@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=eric.smith@collabora.com \
--cc=kernel@collabora.com \
--cc=steven.price@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).