Re: [PATCH RFC] gpu: host1x: validate the firewall source mapping

[sashiko-bot@kernel.org]

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 <bolin.liu@seu.edu.cn>

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/20260911060933.94619-1-bolin.liu@seu.edu.cn?part=1