drm/shmem-helper: Fix BUG_ON() on mmap(PROT_WRITE, MAP_PRIVATE)

Commit Message

Jacek Lawrynowicz May 20, 2024, 10:05 a.m. UTC

From: "Wachowski, Karol" <[e-mail]>

Lack of check for copy-on-write (COW) mapping in drm_gem_shmem_mmap
allows users to call mmap with PROT_WRITE and MAP_PRIVATE flag
causing a kernel panic due to BUG_ON in vmf_insert_pfn_prot:
BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));

Return -EINVAL early if COW mapping is detected.

This bug affects all drm drivers using default shmem helpers.
It can be reproduced by this simple example:
void *ptr = mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, mmap_offset);
ptr[0] = 0;

Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
Cc: Noralf Trønnes <[e-mail]>
Cc: Eric Anholt <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: [e-mail]
Cc: <[e-mail]> # v5.2+
Signed-off-by: Wachowski, Karol <[e-mail]>
Signed-off-by: Jacek Lawrynowicz <[e-mail]>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Daniel Vetter May 21, 2024, 12:38 p.m. UTC | #1

On Mon, May 20, 2024 at 12:05:14PM +0200, Jacek Lawrynowicz wrote:
> From: "Wachowski, Karol" <[e-mail]>
> 
> Lack of check for copy-on-write (COW) mapping in drm_gem_shmem_mmap
> allows users to call mmap with PROT_WRITE and MAP_PRIVATE flag
> causing a kernel panic due to BUG_ON in vmf_insert_pfn_prot:
> BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
> 
> Return -EINVAL early if COW mapping is detected.
> 
> This bug affects all drm drivers using default shmem helpers.
> It can be reproduced by this simple example:
> void *ptr = mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, mmap_offset);
> ptr[0] = 0;
> 
> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
> Cc: Noralf Trønnes <[e-mail]>
> Cc: Eric Anholt <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: [e-mail]
> Cc: <[e-mail]> # v5.2+
> Signed-off-by: Wachowski, Karol <[e-mail]>
> Signed-off-by: Jacek Lawrynowicz <[e-mail]>

Excellent catch!

Reviewed-by: Daniel Vetter <[e-mail]>

I reviewed the other helpers, and ttm/vram helpers already block this with
the check in ttm_bo_mmap_obj.

But the dma helpers does not, because the remap_pfn_range that underlies
the various dma_mmap* function (at least on most platforms) allows some
limited use of cow. But it makes no sense at all to all that only for
gpu buffer objects backed by specific allocators.

Would you be up for the 2nd patch that also adds this check to
drm_gem_dma_mmap, so that we have a consistent uapi?

I'll go ahead and apply this one to drm-misc-fixes meanwhile.

Thanks, Sima

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 177773bcdbfd..885a62c2e1be 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -611,6 +611,9 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>  		return ret;
>  	}
>  
> +	if (is_cow_mapping(vma->vm_flags))
> +		return -EINVAL;
> +
>  	dma_resv_lock(shmem->base.resv, NULL);
>  	ret = drm_gem_shmem_get_pages(shmem);
>  	dma_resv_unlock(shmem->base.resv);
> -- 
> 2.45.1
>

Daniel Vetter May 21, 2024, 12:58 p.m. UTC | #2

On Tue, 21 May 2024 at 14:38, Daniel Vetter <[e-mail]> wrote:
>
> On Mon, May 20, 2024 at 12:05:14PM +0200, Jacek Lawrynowicz wrote:
> > From: "Wachowski, Karol" <[e-mail]>
> >
> > Lack of check for copy-on-write (COW) mapping in drm_gem_shmem_mmap
> > allows users to call mmap with PROT_WRITE and MAP_PRIVATE flag
> > causing a kernel panic due to BUG_ON in vmf_insert_pfn_prot:
> > BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
> >
> > Return -EINVAL early if COW mapping is detected.
> >
> > This bug affects all drm drivers using default shmem helpers.
> > It can be reproduced by this simple example:
> > void *ptr = mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, mmap_offset);
> > ptr[0] = 0;
> >
> > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
> > Cc: Noralf Trønnes <[e-mail]>
> > Cc: Eric Anholt <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Maxime Ripard <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>
> > Cc: [e-mail]
> > Cc: <[e-mail]> # v5.2+
> > Signed-off-by: Wachowski, Karol <[e-mail]>
> > Signed-off-by: Jacek Lawrynowicz <[e-mail]>
>
> Excellent catch!
>
> Reviewed-by: Daniel Vetter <[e-mail]>
>
> I reviewed the other helpers, and ttm/vram helpers already block this with
> the check in ttm_bo_mmap_obj.
>
> But the dma helpers does not, because the remap_pfn_range that underlies
> the various dma_mmap* function (at least on most platforms) allows some
> limited use of cow. But it makes no sense at all to all that only for
> gpu buffer objects backed by specific allocators.
>
> Would you be up for the 2nd patch that also adds this check to
> drm_gem_dma_mmap, so that we have a consistent uapi?
>
> I'll go ahead and apply this one to drm-misc-fixes meanwhile.

Forgot to add: A testcase in igt would also be really lovely.

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt
-Sima


>
> Thanks, Sima
>
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 177773bcdbfd..885a62c2e1be 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -611,6 +611,9 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
> >               return ret;
> >       }
> >
> > +     if (is_cow_mapping(vma->vm_flags))
> > +             return -EINVAL;
> > +
> >       dma_resv_lock(shmem->base.resv, NULL);
> >       ret = drm_gem_shmem_get_pages(shmem);
> >       dma_resv_unlock(shmem->base.resv);
> > --
> > 2.45.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Jacek Lawrynowicz May 23, 2024, 6:29 a.m. UTC | #3

Hi,

On 21.05.2024 14:58, Daniel Vetter wrote:
> On Tue, 21 May 2024 at 14:38, Daniel Vetter <[e-mail]> wrote:
>>
>> On Mon, May 20, 2024 at 12:05:14PM +0200, Jacek Lawrynowicz wrote:
>>> From: "Wachowski, Karol" <[e-mail]>
>>>
>>> Lack of check for copy-on-write (COW) mapping in drm_gem_shmem_mmap
>>> allows users to call mmap with PROT_WRITE and MAP_PRIVATE flag
>>> causing a kernel panic due to BUG_ON in vmf_insert_pfn_prot:
>>> BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags));
>>>
>>> Return -EINVAL early if COW mapping is detected.
>>>
>>> This bug affects all drm drivers using default shmem helpers.
>>> It can be reproduced by this simple example:
>>> void *ptr = mmap(0, size, PROT_WRITE, MAP_PRIVATE, fd, mmap_offset);
>>> ptr[0] = 0;
>>>
>>> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
>>> Cc: Noralf Trønnes <[e-mail]>
>>> Cc: Eric Anholt <[e-mail]>
>>> Cc: Rob Herring <[e-mail]>
>>> Cc: Maarten Lankhorst <[e-mail]>
>>> Cc: Maxime Ripard <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: David Airlie <[e-mail]>
>>> Cc: Daniel Vetter <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: <[e-mail]> # v5.2+
>>> Signed-off-by: Wachowski, Karol <[e-mail]>
>>> Signed-off-by: Jacek Lawrynowicz <[e-mail]>
>>
>> Excellent catch!
>>
>> Reviewed-by: Daniel Vetter <[e-mail]>
>>
>> I reviewed the other helpers, and ttm/vram helpers already block this with
>> the check in ttm_bo_mmap_obj.
>>
>> But the dma helpers does not, because the remap_pfn_range that underlies
>> the various dma_mmap* function (at least on most platforms) allows some
>> limited use of cow. But it makes no sense at all to all that only for
>> gpu buffer objects backed by specific allocators.
>>
>> Would you be up for the 2nd patch that also adds this check to
>> drm_gem_dma_mmap, so that we have a consistent uapi?
>>
>> I'll go ahead and apply this one to drm-misc-fixes meanwhile.
> 
> Forgot to add: A testcase in igt would also be really lovely.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt
> -Sima

OK, we will take a look at the test case.
We have no easy way to test dma helpers, so it would be best if someone using them could make a fix.


Regards,
Jacek

Patch

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 177773bcdbfd..885a62c2e1be 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -611,6 +611,9 @@  int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
 		return ret;
 	}
 
+	if (is_cow_mapping(vma->vm_flags))
+		return -EINVAL;
+
 	dma_resv_lock(shmem->base.resv, NULL);
 	ret = drm_gem_shmem_get_pages(shmem);
 	dma_resv_unlock(shmem->base.resv);