Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Missing IS_ERR() check for host1x_bo_mmap() return values across multiple functions. - [High] BO reference and mapping leak on failure paths in pin_job() - [High] Passing ERR_PTR to drm_syncobj_replace_fence() after host1x_fence_create() fails. -- commit 1a11db6d4b6dd9dc137819f2fb17f9728ae00690 Author: Slavin Liu <[email protected]> gpu: host1x: validate the firewall source mapping This commit adds missing validation for the firewall source mapping in copy_gathers(). It correctly rejects an ERR_PTR or NULL source mapping before copying gather commands, fixing a potential out-of-bounds dereference. > diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c > --- a/drivers/gpu/host1x/job.c > +++ b/drivers/gpu/host1x/job.c > @@ -575,6 +575,10 @@ static inline int copy_gathers(struct device *host, > struct host1x_job *job, > > /* Copy the gather */ > gather = host1x_bo_mmap(g->bo); > + if (IS_ERR(gather)) > + return PTR_ERR(gather); > + if (!gather) > + return -ENOMEM; [Severity: High] This isn't a bug introduced by this patch, but while this adds the proper validation here, there are a few other pre-existing locations that only check for NULL when calling host1x_bo_mmap(). For instance, in do_relocs() in the same file: cmdbuf_addr = host1x_bo_mmap(cmdbuf); if (unlikely(!cmdbuf_addr)) { Could this also evaluate as true for an ERR_PTR and cause an out-of-bounds pointer dereference when patching relocations? Similarly, trace_write_gather() in drivers/gpu/host1x/hw/channel_hw.c: mem = host1x_bo_mmap(bo); if (mem) { And show_channel_gathers() in drivers/gpu/host1x/hw/debug_hw.c: mapped = host1x_bo_mmap(g->bo); if (!mapped) { host1x_debug_output(o, "[could not mmap]\n"); continue; } Do these paths also need IS_ERR() checks to prevent error pointers from falling through to be dereferenced? [Severity: High] This is another pre-existing issue in the same file, but looking at pin_job(), do the failure paths leak BO references or mappings? When iterating over job->relocs, if host1x_bo_pin() fails: map = host1x_bo_pin(dev, bo, direction, NULL); if (IS_ERR(map)) { err = PTR_ERR(map); goto unpin; } Since the jump to the unpin label calls host1x_job_unpin(job), which only processes items already added to the job->unpins array, does this skip releasing the BO reference acquired by the host1x_bo_get() just above it? Also in pin_job(), within the gathers loop, if alloc_iova() fails: alloc = alloc_iova(&host->iova, gather_size >> shift, host->iova_end >> shift, true); if (!alloc) { err = -ENOMEM; goto put; } Does jumping to the put label leave the successfully created map orphaned without calling host1x_bo_unpin(map)? [Severity: High] This is also a pre-existing issue, but I noticed a potential problem with fence creation error handling in drivers/gpu/drm/tegra/submit.c. In tegra_drm_ioctl_channel_submit(), if host1x_fence_create() fails: if (IS_ERR(fence)) { err = PTR_ERR(fence); SUBMIT_ERR(context, "failed to create postfence: %d", err); } drm_syncobj_replace_fence(syncobj, fence); Does execution fall through after the failure and pass the error pointer directly to drm_syncobj_replace_fence()? It appears drm_syncobj_replace_fence() evaluates the pointer as true and calls dma_fence_get(fence), which will dereference the error pointer during kref_get(). > memcpy(job->gather_copy_mapped + offset, gather + g->offset, > g->words * sizeof(u32)); > host1x_bo_munmap(g->bo, gather); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
