[PATCH] drm/i915/gem: Fix phys BO pread/pwrite with offset

[PATCH] drm/i915/gem: Fix phys BO pread/pwrite with offset

[Joonas Lahtinen]

sg_page() returns struct page pointer not (void *) so the scaling
of pread/pwrite is wrong for phys BO and wrong parts of BO would be
accessed if non-zero offset is used.

Last impacted platform with overlay or cursor planes using phys
mapping was Gen3/945G/Lakeport.

Reported-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Fixes: c6790dc22312 ("drm/i915: Wean off drm_pci_alloc/drm_pci_free")
Cc: <stable@vger.kernel.org> # v4.5+
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_phys.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index e375afbf458e..d53129eb5603 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -18,6 +18,17 @@
 #include "i915_gem_tiling.h"
 #include "i915_scatterlist.h"
 
+/* Abuse scatterlist to store pointer instead of struct page. */
+static inline void __set_phys_vaddr(struct scatterlist *sg, void *vaddr)
+{
+	sg_assign_page(sg, (struct page *)vaddr);
+}
+
+static inline void *__get_phys_vaddr(struct scatterlist *sg)
+{
+	return (void *)sg_page(sg);
+}
+
 static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 {
 	struct address_space *mapping = obj->base.filp->f_mapping;
@@ -58,7 +69,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
 	sg->offset = 0;
 	sg->length = obj->base.size;
 
-	sg_assign_page(sg, (struct page *)vaddr);
+	__set_phys_vaddr(sg, vaddr);
 	sg_dma_address(sg) = dma;
 	sg_dma_len(sg) = obj->base.size;
 
@@ -99,7 +110,7 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
 			       struct sg_table *pages)
 {
 	dma_addr_t dma = sg_dma_address(pages->sgl);
-	void *vaddr = sg_page(pages->sgl);
+	void *vaddr = __get_phys_vaddr(pages->sgl);
 
 	__i915_gem_object_release_shmem(obj, pages, false);
 
@@ -139,7 +150,7 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
 int i915_gem_object_pwrite_phys(struct drm_i915_gem_object *obj,
 				const struct drm_i915_gem_pwrite *args)
 {
-	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
+	void *vaddr = __get_phys_vaddr(obj->mm.pages->sgl) + args->offset;
 	char __user *user_data = u64_to_user_ptr(args->data_ptr);
 	struct drm_i915_private *i915 = to_i915(obj->base.dev);
 	int err;
@@ -170,7 +181,7 @@ int i915_gem_object_pwrite_phys(struct drm_i915_gem_object *obj,
 int i915_gem_object_pread_phys(struct drm_i915_gem_object *obj,
 			       const struct drm_i915_gem_pread *args)
 {
-	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
+	void *vaddr = __get_phys_vaddr(obj->mm.pages->sgl) + args->offset;
 	char __user *user_data = u64_to_user_ptr(args->data_ptr);
 	int err;
 
-- 
2.54.0

Re: [PATCH] drm/i915/gem: Fix phys BO pread/pwrite with offset

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Integer overflow in `roundup_pow_of_two` on 32-bit systems causes a massive heap buffer overflow when allocating physically contiguous pages for large objects.
--

commit bdaf5470cc890b63dd50b4ef6b0bd2b13dcdaa7b
Author: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

drm/i915/gem: Fix phys BO pread/pwrite with offset

This commit fixes a scaling issue in phys BO pread/pwrite when using
non-zero offsets. It updates the code to correctly access the struct
page pointers via scatterlist abstractions instead of void pointers.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index e375afbf458ee..d53129eb56030 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c

[ ... ]

> @@ -58,7 +69,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>  	sg->offset = 0;
>  	sg->length = obj->base.size;
>  
> -	sg_assign_page(sg, (struct page *)vaddr);
> +	__set_phys_vaddr(sg, vaddr);

[Severity: Critical]
This is a pre-existing issue, but does the dma_alloc_coherent() allocation
earlier in this function overflow on 32-bit systems?

Looking at i915_gem_object_get_pages_phys(), we allocate memory using:

    vaddr = dma_alloc_coherent(obj->base.dev->dev,
                               roundup_pow_of_two(obj->base.size),
                               &dma, GFP_KERNEL);

If a user allocates a GEM object larger than 2GB (e.g., 0x80001000),
roundup_pow_of_two() calculates 1UL << fls_long(n - 1). On x86 32-bit,
shift amounts are masked to 5 bits, so 32 & 31 = 0, resulting in an
allocation size of 1 byte (which the allocator rounds up to PAGE_SIZE).

Later in the same function, a loop writes obj->base.size bytes into this
undersized buffer:

    for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
        struct page *page;

        page = shmem_read_mapping_page(mapping, i);
        if (IS_ERR(page))
            goto err_st;

        memcpy_from_page(dst, page, 0, PAGE_SIZE);
        drm_clflush_virt_range(dst, PAGE_SIZE);

        put_page(page);
        dst += PAGE_SIZE;
    }

Can this cause a massive buffer overflow and corrupt physical memory when
handling large objects?

>  	sg_dma_address(sg) = dma;
>  	sg_dma_len(sg) = obj->base.size;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610060314.26111-1-joonas.lahtinen@linux.intel.com?part=1

Re: [PATCH] drm/i915/gem: Fix phys BO pread/pwrite with offset

[Tvrtko Ursulin]

On 10/06/2026 07:03, Joonas Lahtinen wrote:
> sg_page() returns struct page pointer not (void *) so the scaling
> of pread/pwrite is wrong for phys BO and wrong parts of BO would be
> accessed if non-zero offset is used.
> 
> Last impacted platform with overlay or cursor planes using phys
> mapping was Gen3/945G/Lakeport.
> 
> Reported-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Fixes: c6790dc22312 ("drm/i915: Wean off drm_pci_alloc/drm_pci_free")
> Cc: <stable@vger.kernel.org> # v4.5+
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_phys.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index e375afbf458e..d53129eb5603 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -18,6 +18,17 @@
>   #include "i915_gem_tiling.h"
>   #include "i915_scatterlist.h"
>   
> +/* Abuse scatterlist to store pointer instead of struct page. */
> +static inline void __set_phys_vaddr(struct scatterlist *sg, void *vaddr)
> +{
> +	sg_assign_page(sg, (struct page *)vaddr);
> +}
> +
> +static inline void *__get_phys_vaddr(struct scatterlist *sg)
> +{
> +	return (void *)sg_page(sg);
> +}
> +
>   static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>   {
>   	struct address_space *mapping = obj->base.filp->f_mapping;
> @@ -58,7 +69,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>   	sg->offset = 0;
>   	sg->length = obj->base.size;
>   
> -	sg_assign_page(sg, (struct page *)vaddr);
> +	__set_phys_vaddr(sg, vaddr);
>   	sg_dma_address(sg) = dma;
>   	sg_dma_len(sg) = obj->base.size;
>   
> @@ -99,7 +110,7 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
>   			       struct sg_table *pages)
>   {
>   	dma_addr_t dma = sg_dma_address(pages->sgl);
> -	void *vaddr = sg_page(pages->sgl);
> +	void *vaddr = __get_phys_vaddr(pages->sgl);
>   
>   	__i915_gem_object_release_shmem(obj, pages, false);
>   
> @@ -139,7 +150,7 @@ i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj,
>   int i915_gem_object_pwrite_phys(struct drm_i915_gem_object *obj,
>   				const struct drm_i915_gem_pwrite *args)
>   {
> -	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
> +	void *vaddr = __get_phys_vaddr(obj->mm.pages->sgl) + args->offset;
>   	char __user *user_data = u64_to_user_ptr(args->data_ptr);
>   	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   	int err;
> @@ -170,7 +181,7 @@ int i915_gem_object_pwrite_phys(struct drm_i915_gem_object *obj,
>   int i915_gem_object_pread_phys(struct drm_i915_gem_object *obj,
>   			       const struct drm_i915_gem_pread *args)
>   {
> -	void *vaddr = sg_page(obj->mm.pages->sgl) + args->offset;
> +	void *vaddr = __get_phys_vaddr(obj->mm.pages->sgl) + args->offset;
>   	char __user *user_data = u64_to_user_ptr(args->data_ptr);
>   	int err;
>   

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko