From: GuoHan Zhao <[email protected]>
ethosu_ioctl_submit_job() allocates done_fence before validating buffer
handles. Errors after allocation call ethosu_job_err_cleanup(), which frees
the job but leaks the uninitialized fence.
A scheduler dependency error also lets ethosu_job_run() return before
dma_fence_init(). Normal cleanup then passes a zeroed refcount to
dma_fence_put().
Release done_fence in the common cleanup path and use
dma_fence_was_initialized() to distinguish initialized fences from raw
allocations.
Fixes: 5a5e9c0228e6 ("accel: Add Arm Ethos-U NPU driver")
Cc: [email protected]
Reported-by: Sashiko <[email protected]>
Link:
https://sashiko.dev/#/patchset/[email protected]?part=1
Signed-off-by: GuoHan Zhao <[email protected]>
---
Sashiko reports:
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_err_cleanup() {
> unsigned int i;
>
> for (i = 0; i < job->region_cnt; i++)
> drm_gem_object_put(job->region_bo[i]);
>
> drm_gem_object_put(job->cmd_bo);
>
> kfree(job);
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> ethosu_ioctl_submit_job() allocates done_fence. If a subsequent
> validation fails, the error path calls ethosu_job_err_cleanup().
>
> This function frees the job structure but misses freeing job->done_fence.
>
> Can an unprivileged user repeatedly call the submit ioctl with invalid
> handles to trigger a done_fence leak?
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_run() {
> ...
> if (unlikely(job->base.s_fence->finished.error))
> return NULL;
>
> dma_fence_init(fence, ðosu_fence_ops, &dev->fence_lock,
> dev->fence_context, ++dev->emit_seqno);
> ...
> }
>
> This is a pre-existing issue, but was not introduced by this patch.
>
> If job->base.s_fence->finished.error is true, the function returns early
> without calling dma_fence_init() on job->done_fence.
>
> When the job is later destroyed, ethosu_job_cleanup() unconditionally
> calls dma_fence_put() on the fence:
>
> drivers/accel/ethosu/ethosu_job.c:ethosu_job_cleanup() {
> ...
> dma_fence_put(job->done_fence);
> ...
> }
>
> Since the fence is zero-initialized, will calling kref_put() on it
> violate the API and cause a refcount underflow?
drivers/accel/ethosu/ethosu_job.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/accel/ethosu/ethosu_job.c
b/drivers/accel/ethosu/ethosu_job.c
index b76924645aaa..be745e36f1fa 100644
--- a/drivers/accel/ethosu/ethosu_job.c
+++ b/drivers/accel/ethosu/ethosu_job.c
@@ -152,6 +152,13 @@ static void ethosu_job_err_cleanup(struct ethosu_job *job)
drm_gem_object_put(job->cmd_bo);
+ if (job->done_fence) {
+ if (dma_fence_was_initialized(job->done_fence))
+ dma_fence_put(job->done_fence);
+ else
+ dma_fence_free(job->done_fence);
+ }
+
kfree(job);
}
@@ -162,7 +169,6 @@ static void ethosu_job_cleanup(struct kref *ref)
pm_runtime_put_autosuspend(job->dev->base.dev);
- dma_fence_put(job->done_fence);
dma_fence_put(job->inference_done_fence);
ethosu_job_err_cleanup(job);
--
2.43.0
