| Message ID | 20210301095254.1946084-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/etnaviv: Use FOLL_FORCE for userptr | expand |
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);
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); > >
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 >
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 > > >
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);
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(-)