Re: [PATCH 5/6] gpu: host1x: Change pin_job() return type to int

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] An integer overflow in copy_gathers() leads to a massive heap buffer overflow during memcpy.
- [High] Silent failure when iommu_map_sgtable() returns a negative error code.
- [High] Multiple error paths in pin_job() leak host1x_bo references and pinned host1x_bo_mapping objects.
- [High] Missing or incorrect validation of host1x_bo_mmap() return value causes kernel panic.
--

commit 49509dd1b9a9c195fff52b9936852ca75bde12b1
Author: Mikko Perttunen <mperttunen@nvidia.com>

gpu: host1x: Change pin_job() return type to int

pin_job() returns negative errno values on error paths (-EINVAL,
-ENOMEM, PTR_ERR() of mapping) but was declared as unsigned int.
The caller would immediately cast back to int, so there was no
functional issue, but it still warrants fixing.

> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 3ed49e1fd9332..1659793195991 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
[ ... ]
> @@ -138,7 +138,7 @@ void host1x_job_add_wait(struct host1x_job *job, u32 id, u32 thresh,
>  }
>  EXPORT_SYMBOL(host1x_job_add_wait);
>  
> -static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
> +static int pin_job(struct host1x *host, struct host1x_job *job)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing pin_job(),
does this code silently ignore mapping failures?

drivers/gpu/host1x/job.c:pin_job() {
    ...
			err = iommu_map_sgtable(host->domain,
						iova_dma_addr(&host->iova, alloc),
						map->sgt, IOMMU_READ);
			if (err == 0) {
				__free_iova(&host->iova, alloc);
				err = -EINVAL;
				goto put;
			}
    ...
}

iommu_map_sgtable() returns a positive mapped size on success and a negative
error code (like -EINVAL or -ENOMEM) on failure.

Since the check is exactly if (err == 0), a negative error code evaluates
to false, skipping the error path (goto put;).

This allows the code to proceed with an unmapped IOVA, which would cause the
IOMMU to block hardware memory access and trigger faults during execution.

Is this check intended to be if (err <= 0)?


[Severity: High]
This is a pre-existing issue, but do these error paths leak host1x_bo
references and mappings?

drivers/gpu/host1x/job.c:pin_job() {
    ...
		map = host1x_bo_pin(dev, bo, direction, NULL);
		if (IS_ERR(map)) {
			err = PTR_ERR(map);
			goto unpin;
		}

		if (map->chunks > 1) {
			err = -EINVAL;
			goto unpin;
		}

		job->addr_phys[job->num_unpins] = map->phys;
		job->unpins[job->num_unpins].map = map;
		job->num_unpins++;
    ...
}

In the loop, job->num_unpins is only incremented at the end of each iteration.

If an error occurs midway (e.g., map->chunks > 1), the loop jumps to unpin.

The cleanup routine host1x_job_unpin() iterates strictly up to job->num_unpins,
failing to release the BO reference and unpin the memory allocated during the
currently failing iteration.

Could a malicious user repeatedly trigger this error path to exhaust kernel
memory?


[Severity: High]
This is a pre-existing issue, but does this code dereference an error
pointer if host1x_bo_mmap() fails?

drivers/gpu/host1x/job.c:do_relocs() {
    ...
		if (!cmdbuf_addr) {
			cmdbuf_addr = host1x_bo_mmap(cmdbuf);

			if (unlikely(!cmdbuf_addr)) {
    ...
}

And similarly in copy_gathers():

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
		gather = host1x_bo_mmap(g->bo);
		memcpy(job->gather_copy_mapped + offset, gather + g->offset,
		       g->words * sizeof(u32));
    ...
}

host1x_bo_mmap() returns an ERR_PTR() on failure (e.g., -ENOMEM), not NULL.

In do_relocs(), the check !cmdbuf_addr fails to detect an error pointer.
In copy_gathers(), the result is passed directly to memcpy() without any
checks.

Could this lead to a page fault and kernel panic if userspace triggers a
vmap failure?


[Severity: Critical]
This isn't a bug introduced by this patch, but can this size calculation
overflow on 32-bit platforms?

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
	for (i = 0; i < job->num_cmds; i++) {
		struct host1x_job_gather *g;

		if (job->cmds[i].is_wait)
			continue;

		g = &job->cmds[i].gather;

		size += g->words * sizeof(u32);
	}
    ...
}

On 32-bit platforms, size_t size is 32 bits.

If an ioctl is submitted with a large number of command buffers
(e.g., num_cmdbufs = 65541 and words = 16383), the size calculation results
in 4,295,027,340, wrapping around to 60,044.

dma_alloc_wc() then allocates this undersized buffer, and the subsequent
memcpy() loop would copy out of bounds.

Could this lead to a massive heap buffer overflow?

>  	unsigned long mask = HOST1X_RELOC_READ | HOST1X_RELOC_WRITE;
>  	struct host1x_client *client = job->client;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-b4-host1x-small-fixes-a-v1-0-7c1131c0b3ad@nvidia.com?part=5