[PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[Daniel Vetter]

Nothing checks userptr.ro except this call to pup_fast, which means
there's nothing actually preventing userspace from writing to this.
Which means you can just read-only mmap any file you want, userptr it
and then write to it with the gpu. Not good.

The right way to handle this is FOLL_WRITE | FOLL_FORCE, which will
break any COW mappings and update tracking for MAY_WRITE mappings so
there's no exploit and the vm isn't confused about what's going on.
For any legit use case there's no difference from what userspace can
observe and do.

Cc: stable@vger.kernel.org
Cc: John Hubbard <jhubbard@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: etnaviv@lists.freedesktop.org
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 6d38c5c17f23..a9e696d05b33 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -689,7 +689,7 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
 		struct page **pages = pvec + pinned;
 
 		ret = pin_user_pages_fast(ptr, num_pages,
-					  !userptr->ro ? FOLL_WRITE : 0, pages);
+					  FOLL_WRITE | FOLL_FORCE, pages);
 		if (ret < 0) {
 			unpin_user_pages(pvec, pinned);
 			kvfree(pvec);
-- 
2.30.0

Re: [PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[Lucas Stach]

Am Montag, dem 01.03.2021 um 10:52 +0100 schrieb Daniel Vetter:
> Nothing checks userptr.ro except this call to pup_fast, which means
> there's nothing actually preventing userspace from writing to this.
> Which means you can just read-only mmap any file you want, userptr it
> and then write to it with the gpu. Not good.

I agree about the "not good" part.

> The right way to handle this is FOLL_WRITE | FOLL_FORCE, which will
> break any COW mappings and update tracking for MAY_WRITE mappings so
> there's no exploit and the vm isn't confused about what's going on.
> For any legit use case there's no difference from what userspace can
> observe and do.

This however seems pretty heavy handed. Does this mean we do a full COW
cycle of the userpages on BO creation? This most likely kills a lot of
the performance benefits that one might seek by using userptr. If
that's the case I might still take this patch for stable, but then we
should rather just disallow writable GPU mappings to this BO.

Regards,
Lucas

> 
> Cc: stable@vger.kernel.org
> Cc: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: etnaviv@lists.freedesktop.org
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 6d38c5c17f23..a9e696d05b33 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -689,7 +689,7 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
>  		struct page **pages = pvec + pinned;
>  
> 
> 
> 
> 
> 
> 
> 
>  		ret = pin_user_pages_fast(ptr, num_pages,
> -					  !userptr->ro ? FOLL_WRITE : 0, pages);
> +					  FOLL_WRITE | FOLL_FORCE, pages);
>  		if (ret < 0) {
>  			unpin_user_pages(pvec, pinned);
>  			kvfree(pvec);

Re: [PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[Daniel Vetter]

On Mon, Mar 1, 2021 at 11:28 AM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Montag, dem 01.03.2021 um 10:52 +0100 schrieb Daniel Vetter:
> > Nothing checks userptr.ro except this call to pup_fast, which means
> > there's nothing actually preventing userspace from writing to this.
> > Which means you can just read-only mmap any file you want, userptr it
> > and then write to it with the gpu. Not good.
>
> I agree about the "not good" part.
>
> > The right way to handle this is FOLL_WRITE | FOLL_FORCE, which will
> > break any COW mappings and update tracking for MAY_WRITE mappings so
> > there's no exploit and the vm isn't confused about what's going on.
> > For any legit use case there's no difference from what userspace can
> > observe and do.
>
> This however seems pretty heavy handed. Does this mean we do a full COW
> cycle of the userpages on BO creation? This most likely kills a lot of
> the performance benefits that one might seek by using userptr. If
> that's the case I might still take this patch for stable, but then we
> should rather just disallow writable GPU mappings to this BO.

That's not what's happening. If the mmap is writeable already (like
any malloc'ed area, and anything you might vacuum up with Xshm), then
FOLL_FORCE does nothing. The difference only happens when the current
mmap region (or some of the pte at least) is read-only. Then:
- for MAP_SHARED with the VM_MAYWRITE flag set, we simply adjust some
book-keeping (no copying of pages), so that the core mm doesn't get
confused about the potentially changed pages contents due to gpu
writes. Without this you could corrupt fs state (e.g. when the fs
checksums file contents or does in-place mmap and stuff like that).
- for MAP_PRIVATE we force the CoW. Just don't do userptr on these,
really, it doesn't make much sense anyway. And note again, if the
mapping is currently writeable, then there's no copying going on, this
is only when the mmap/pte is currently read-only. This is the "let's
overwrite libc.so" attack vector :-)

So really in practice nothing should happen here aside from plugging
the "not good" part. Note that on recent kernels the CoW breaking on
fork() happens irrespective of FOLL_FORCE or not once you have the
mapping established. So if you do a lot of userptr on MAP_PRIVATE
already and applications are using fork(), then you're already
suffering big time (since 5.10 or so iirc, John probably knows the
exact commit without looking).
-Daniel

> Regards,
> Lucas
>
> >
> > Cc: stable@vger.kernel.org
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> > Cc: etnaviv@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > index 6d38c5c17f23..a9e696d05b33 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > @@ -689,7 +689,7 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
> >               struct page **pages = pvec + pinned;
> >
> >
> >
> >
> >
> >
> >
> >
> >               ret = pin_user_pages_fast(ptr, num_pages,
> > -                                       !userptr->ro ? FOLL_WRITE : 0, pages);
> > +                                       FOLL_WRITE | FOLL_FORCE, pages);
> >               if (ret < 0) {
> >                       unpin_user_pages(pvec, pinned);
> >                       kvfree(pvec);
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[Daniel Vetter]

On Mon, Mar 01, 2021 at 10:52:53AM +0100, Daniel Vetter wrote:
> Nothing checks userptr.ro except this call to pup_fast, which means
> there's nothing actually preventing userspace from writing to this.
> Which means you can just read-only mmap any file you want, userptr it
> and then write to it with the gpu. Not good.
> 
> The right way to handle this is FOLL_WRITE | FOLL_FORCE, which will
> break any COW mappings and update tracking for MAY_WRITE mappings so
> there's no exploit and the vm isn't confused about what's going on.
> For any legit use case there's no difference from what userspace can
> observe and do.
> 
> Cc: stable@vger.kernel.org
> Cc: John Hubbard <jhubbard@nvidia.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: etnaviv@lists.freedesktop.org

Can I please have an ack on this so I can apply it? It's stuck.

Thanks, Daniel

> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 6d38c5c17f23..a9e696d05b33 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -689,7 +689,7 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
>  		struct page **pages = pvec + pinned;
>  
>  		ret = pin_user_pages_fast(ptr, num_pages,
> -					  !userptr->ro ? FOLL_WRITE : 0, pages);
> +					  FOLL_WRITE | FOLL_FORCE, pages);
>  		if (ret < 0) {
>  			unpin_user_pages(pvec, pinned);
>  			kvfree(pvec);
> -- 
> 2.30.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm/etnaviv: Use FOLL_FORCE for userptr

[Lucas Stach]

Am Freitag, dem 19.03.2021 um 20:09 +0100 schrieb Daniel Vetter:
> On Mon, Mar 01, 2021 at 10:52:53AM +0100, Daniel Vetter wrote:
> > Nothing checks userptr.ro except this call to pup_fast, which means
> > there's nothing actually preventing userspace from writing to this.
> > Which means you can just read-only mmap any file you want, userptr it
> > and then write to it with the gpu. Not good.
> > 
> > The right way to handle this is FOLL_WRITE | FOLL_FORCE, which will
> > break any COW mappings and update tracking for MAY_WRITE mappings so
> > there's no exploit and the vm isn't confused about what's going on.
> > For any legit use case there's no difference from what userspace can
> > observe and do.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> > Cc: etnaviv@lists.freedesktop.org
> 
> Can I please have an ack on this so I can apply it? It's stuck.

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > index 6d38c5c17f23..a9e696d05b33 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > @@ -689,7 +689,7 @@ static int etnaviv_gem_userptr_get_pages(struct etnaviv_gem_object *etnaviv_obj)
> >  		struct page **pages = pvec + pinned;
> >  
> > 
> > 
> > 
> >  		ret = pin_user_pages_fast(ptr, num_pages,
> > -					  !userptr->ro ? FOLL_WRITE : 0, pages);
> > +					  FOLL_WRITE | FOLL_FORCE, pages);
> >  		if (ret < 0) {
> >  			unpin_user_pages(pvec, pinned);
> >  			kvfree(pvec);
> > -- 
> > 2.30.0
> > 
>