[PATCH 1/3] drm/ttm: check for NULL resource in ttm_bo_kmap()

[PATCH 1/3] drm/ttm: check for NULL resource in ttm_bo_kmap()

[kaii-k]

From: Harsh Parmar <kaii02ctf@gmail.com>

A buffer object can legitimately have no backing store: a driver may
call ttm_bo_init_reserved() with zero placements, which leaves
bo->resource NULL. It can also become NULL after the backing store has
been released.

ttm_bo_kmap() dereferences bo->resource unconditionally (res->size),
causing a NULL pointer dereference and kernel oops when called on such a
buffer.

Bail out with -EINVAL if the buffer has no backing store.

Fixes: d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer")
Signed-off-by: Harsh Parmar <kaii02ctf@gmail.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index f9458a4..158f0df 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -435,6 +435,8 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
 
 	map->virtual = NULL;
 	map->bo = bo;
+	if (!res)
+		return -EINVAL;
 	if (num_pages > PFN_UP(res->size))
 		return -EINVAL;
 	if ((start_page + num_pages) > PFN_UP(res->size))
-- 
2.53.0

[PATCH 2/3] drm/ttm: check for NULL resource in ttm_bo_vmap()

[kaii-k]

From: Harsh Parmar <kaii02ctf@gmail.com>

A buffer object can legitimately have no backing store: a driver may
call ttm_bo_init_reserved() with zero placements, which leaves
bo->resource NULL. It can also become NULL after the backing store has
been released.

ttm_bo_vmap() passes bo->resource to ttm_mem_io_reserve() and then
dereferences it (mem->bus.*), causing a NULL pointer dereference when
called on such a buffer, e.g. via dma_buf_vmap() ->
drm_gem_dmabuf_vmap() -> drm_gem_ttm_vmap().

Bail out with -EINVAL if the buffer has no backing store.

Fixes: d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer")
Signed-off-by: Harsh Parmar <kaii02ctf@gmail.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 158f0df..01017e8 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -508,6 +508,9 @@ int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map)
 
 	dma_resv_assert_held(bo->base.resv);
 
+	if (!mem)
+		return -EINVAL;
+
 	ret = ttm_mem_io_reserve(bo->bdev, mem);
 	if (ret)
 		return ret;
-- 
2.53.0

[PATCH 3/3] drm/ttm: check for NULL resource in ttm_bo_vm_fault_reserved()

[kaii-k]

From: Harsh Parmar <kaii02ctf@gmail.com>

A buffer object can legitimately have no backing store: a driver may
call ttm_bo_init_reserved() with zero placements, which leaves
bo->resource NULL. It can also become NULL after the backing store has
been released.

ttm_bo_vm_fault_reserved() passes bo->resource to ttm_mem_io_reserve()
and dereferences it, causing a NULL pointer dereference when a fault hits
a buffer with no backing store.

Return VM_FAULT_SIGBUS if the buffer has no backing store.

Fixes: d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer")
Signed-off-by: Harsh Parmar <kaii02ctf@gmail.com>
---
 drivers/gpu/drm/ttm/ttm_bo_vm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 872bf44..69dd6bd 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -205,6 +205,9 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
 	if (unlikely(ret != 0))
 		return ret;
 
+	if (unlikely(!bo->resource))
+		return VM_FAULT_SIGBUS;
+
 	err = ttm_mem_io_reserve(bo->bdev, bo->resource);
 	if (unlikely(err != 0))
 		return VM_FAULT_SIGBUS;
-- 
2.53.0

Re: [PATCH 3/3] drm/ttm: check for NULL resource in ttm_bo_vm_fault_reserved()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] drm/xe/oa: missing vm_pgoff check in xe_oa_mmap() [1]
--

commit e95d22b6c456f4e471c0ed18ffb86dc352011af3
Author: Harsh Parmar <kaii02ctf@gmail.com>

drm/ttm: check for NULL resource in ttm_bo_vm_fault_reserved()

This commit adds a NULL check on bo->resource in ttm_bo_vm_fault_reserved()
and returns VM_FAULT_SIGBUS when a buffer object has no backing store.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 872bf444b1f0f..69dd6bdd54022 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -205,6 +205,9 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
>  	if (unlikely(ret != 0))
>  		return ret;
>  
> +	if (unlikely(!bo->resource))
> +		return VM_FAULT_SIGBUS;
> +
>  	err = ttm_mem_io_reserve(bo->bdev, bo->resource);
>  	if (unlikely(err != 0))
>  		return VM_FAULT_SIGBUS;

[Severity: Critical]
This problem wasn't introduced by this patch, but can the prefault loop in
ttm_bo_vm_fault_reserved() read past the end of ttm->pages[] [1] when a
VMA has a non-zero vm_pgoff?

drivers/gpu/drm/ttm/ttm_bo_vm.c:ttm_bo_vm_fault_reserved() {
	...
	page_offset = linear_page_index(vma, address) -
		drm_vma_node_start(&bo->base.vma_node);
	page_last = vma_end_pgoff(vma) -
		drm_vma_node_start(&bo->base.vma_node);

	if (unlikely(page_offset >= PFN_UP(bo->base.size)))
		return VM_FAULT_SIGBUS;
	...
	for (i = 0; i < num_prefault; ++i) {
		if (bo->resource->bus.is_iomem) {
			pfn = ttm_bo_io_mem_pfn(bo, page_offset);
		} else {
			page = ttm->pages[page_offset];
			...
			pfn = page_to_pfn(page);
		}
		...
		ret = vmf_insert_pfn_prot(vma, address, pfn, prot);
		...
		address += PAGE_SIZE;
		if (unlikely(++page_offset >= page_last))
			break;
	}
}

For example, xe_oa_mmap() checks that the VMA size equals xe_bo_size(bo)
before calling drm_gem_mmap_obj(), but neither checks nor zeroes
vma->vm_pgoff.  Because stream->oa_buffer.bo is a ttm_bo_type_kernel object,
drm_vma_node_start(&bo->base.vma_node) is 0 and page_last becomes
vma->vm_pgoff + PFN_UP(bo->base.size).  A fault near the end of the buffer
passes the initial PFN_UP(bo->base.size) check, and the prefault loop then
steps page_offset past PFN_UP(bo->base.size) toward page_last, dereferencing
out-of-bounds ttm->pages[] entries or mapping out-of-bounds PFNs into
userspace.

[1] https://sashiko.dev/#/bug/linux-d5cc9df6-b93b-44e4-8837-59f1909411f7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109202330.1816799-1-harshicon28@gmail.com?part=3

Re: [PATCH 1/3] drm/ttm: check for NULL resource in ttm_bo_kmap()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/ttm: inaccurate Fixes trailer in ttm_bo_kmap() NULL check
  commit
--

commit 96feeda5517d178ff64fe467f6cc8933b44eb0d5
Author: Harsh Parmar <kaii02ctf@gmail.com>

> drm/ttm: check for NULL resource in ttm_bo_kmap()
>
> A buffer object can legitimately have no backing store: a driver may
> call ttm_bo_init_reserved() with zero placements, which leaves
> bo->resource NULL. It can also become NULL after the backing store has
> been released.
>
> ttm_bo_kmap() dereferences bo->resource unconditionally (res->size),
> causing a NULL pointer dereference and kernel oops when called on such a
> buffer.
>
> Bail out with -EINVAL if the buffer has no backing store.
>
> Fixes: d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer")
> Signed-off-by: Harsh Parmar <kaii02ctf@gmail.com>

[Severity: Low]
In d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer"),
ttm_bo_init_reserved() still unconditionally pointed bo->resource at the
embedded bo->_mem, so bo->resource could not be NULL on an initialized
buffer object.

Leaving bo->resource NULL when ttm_bo_init_reserved() is called with zero
placements was introduced by 180253782038 ("drm/ttm: stop allocating dummy
resources during BO creation"), and leaving it NULL after releasing backing
store in ttm_bo_pipeline_gutting() was introduced by c7ea16f6a47f
("drm/ttm: stop allocating a dummy resource for pipelined gutting").
Should the Fixes: tag reference those commits instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109202330.1816799-1-harshicon28@gmail.com?part=1

Re: [PATCH 1/3] drm/ttm: check for NULL resource in ttm_bo_kmap()

[Christian König]

On 11/9/26 21:23, kaii-k wrote:
> From: Harsh Parmar <kaii02ctf@gmail.com>
> 
> A buffer object can legitimately have no backing store: a driver may
> call ttm_bo_init_reserved() with zero placements, which leaves
> bo->resource NULL. It can also become NULL after the backing store has
> been released.
> 
> ttm_bo_kmap() dereferences bo->resource unconditionally (res->size),
> causing a NULL pointer dereference and kernel oops when called on such a
> buffer.
> 
> Bail out with -EINVAL if the buffer has no backing store.
> 
> Fixes: d3116756a710 ("drm/ttm: rename bo->mem and make it a pointer")
> Signed-off-by: Harsh Parmar <kaii02ctf@gmail.com>

Reviewed-by: Christian König <christian.koenig@amd.com> for the entire series.

> ---
>  drivers/gpu/drm/ttm/ttm_bo_util.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index f9458a4..158f0df 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -435,6 +435,8 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
> 
>         map->virtual = NULL;
>         map->bo = bo;
> +       if (!res)
> +               return -EINVAL;
>         if (num_pages > PFN_UP(res->size))
>                 return -EINVAL;
>         if ((start_page + num_pages) > PFN_UP(res->size))
> --
> 2.53.0
>