Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] ejob->done_fence is leaked on all error paths, and a NULL pointer dereference occurs if its initial allocation fails. - [High] Hardware is not stopped and in-flight jobs are not synchronized in driver removal, leading to Use-After-Free. -- commit 5e4bb4618ed5dae092c0c36b58730b5281d2a249 Author: Rob Herring (Arm) <[email protected]> accel: ethosu: Ensure SRAM size is 0 on mapping failure On a mapping failure of the SRAM, the SRAM size is left as non-zero. The probe will succeed as the error return is not checked since having SRAM is not a hard requirement. The non-zero size allows jobs to access SRAM which is left pointing to physical base address 0x0. > diff --git a/drivers/accel/ethosu/ethosu_drv.c > b/drivers/accel/ethosu/ethosu_drv.c > index d121fb0d77328..f1af7b3ea0385 100644 > --- a/drivers/accel/ethosu/ethosu_drv.c > +++ b/drivers/accel/ethosu/ethosu_drv.c > @@ -281,8 +281,6 @@ static int ethosu_device_suspend(struct device *dev) > > static int ethosu_sram_init(struct ethosu_device *ethosudev) > { > - ethosudev->npu_info.sram_size = 0; > - > ethosudev->srampool = of_gen_pool_get(ethosudev->base.dev->of_node, > "sram", 0); > if (!ethosudev->srampool) > return 0; > @@ -293,6 +291,7 @@ static int ethosu_sram_init(struct ethosu_device > *ethosudev) > > ethosudev->npu_info.sram_size, > > ðosudev->sramphys); > if (!ethosudev->sram) { > + ethosudev->npu_info.sram_size = 0; > dev_err(ethosudev->base.dev, "failed to allocate from SRAM > pool\n"); > return -ENOMEM; > } While reviewing this patch, I noticed a few pre-existing issues in this driver. > static void ethosu_remove(struct platform_device *pdev) > { > struct ethosu_device *ethosudev = dev_get_drvdata(&pdev->dev); > > drm_dev_unregister(ðosudev->base); > ethosu_job_fini(ethosudev); > if (ethosudev->sram) > gen_pool_free(ethosudev->srampool, (unsigned > long)ethosudev->sram, > ethosudev->npu_info.sram_size); > } [Severity: High] This is a pre-existing issue, but is it safe to tear down the device this way in ethosu_remove()? It appears that ethosu_remove() tears down the DRM scheduler via ethosu_job_fini() and frees the SRAM, but does not stop the hardware or wait for the active in_flight_job to complete. If a user unbinds the driver via sysfs while a job is running, could the hardware continue executing and access SRAM or DMA buffers that have already been freed, leading to memory corruption? > diff --git a/drivers/accel/ethosu/ethosu_job.c > b/drivers/accel/ethosu/ethosu_job.c > --- a/drivers/accel/ethosu/ethosu_job.c > +++ b/drivers/accel/ethosu/ethosu_job.c [ ... ] > static void ethosu_job_err_cleanup(struct ethosu_job *job) > { > unsigned int i; > > ethosu_perfmon_put(job->perfmon); > > 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); > } [Severity: High] This isn't a bug introduced by this patch, but can ethosu_job_err_cleanup() leak job->done_fence? It frees the job object itself, but I don't see job->done_fence being freed, which might cause a memory leak on every job submission failure path. [ ... ] > static int ethosu_ioctl_submit_job(struct drm_device *dev, struct drm_file > *file, > struct drm_ethosu_job *job, > int perfmon_id) > { [ ... ] > ejob->done_fence = kzalloc_obj(*ejob->done_fence); > if (!ejob->done_fence) { > ret = -ENOMEM; > goto out_cleanup_job; > } > > ret = drm_sched_job_init(&ejob->base, > &file_priv->sched_entity, > 1, NULL, file->client_id); > if (ret) > goto out_put_job; [ ... ] > out_cleanup_job: > if (ret) > drm_sched_job_cleanup(&ejob->base); > out_put_job: > ethosu_job_err_cleanup(ejob); > > return ret; > } [Severity: High] This is a pre-existing issue, but could jumping to out_cleanup_job when allocating ejob->done_fence fails cause a NULL pointer dereference? If allocating ejob->done_fence fails, the code jumps to out_cleanup_job, which calls drm_sched_job_cleanup(&ejob->base). Because drm_sched_job_init() hasn't run yet, ejob->base.s_fence is NULL, which might lead to a crash when it is dereferenced during cleanup. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
