linux-kernel - Re: [PATCH] drm/msm: Ensure mmap offset is initialized

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 1 Jun 2022 00:11:15 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Rob Clark <robdclark@...omium.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Gerd Hoffmann <kraxel@...hat.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: Ensure mmap offset is initialized

On Tue, 31 May 2022 at 23:37, Rob Clark <robdclark@...il.com> wrote:
>
> On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov
> <dmitry.baryshkov@...aro.org> wrote:
> >
> > On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@...il.com> wrote:
> > >
> > > From: Rob Clark <robdclark@...omium.org>
> > >
> > > If a GEM object is allocated, and then exported as a dma-buf fd which is
> > > mmap'd before or without the GEM buffer being directly mmap'd, the
> > > vma_node could be unitialized.  This leads to a situation where the CPU
> > > mapping is not correctly torn down in drm_vma_node_unmap().
> > >
> > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> > > Signed-off-by: Rob Clark <robdclark@...omium.org>
> > > ---
> > >  drivers/gpu/drm/msm/msm_drv.c       |  2 +-
> > >  drivers/gpu/drm/msm/msm_drv.h       |  1 +
> > >  drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> > >  3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 44485363f37a..14ab9a627d8b 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> > >         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > >         .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > >         .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > > -       .gem_prime_mmap     = drm_gem_prime_mmap,
> > > +       .gem_prime_mmap     = msm_gem_prime_mmap,
> > >  #ifdef CONFIG_DEBUG_FS
> > >         .debugfs_init       = msm_debugfs_init,
> > >  #endif
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > > index bb052071b16d..090b8074fec7 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.h
> > > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> > >  void msm_gem_shrinker_init(struct drm_device *dev);
> > >  void msm_gem_shrinker_cleanup(struct drm_device *dev);
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > >  struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> > >  int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> > >  void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> > > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > index 94ab705e9b8a..dcc8a573bc76 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > @@ -11,6 +11,21 @@
> > >  #include "msm_drv.h"
> > >  #include "msm_gem.h"
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > > +{
> > > +       int ret;
> > > +
> > > +       /* Ensure the mmap offset is initialized.  We lazily initialize it,
> > > +        * so if it has not been first mmap'd directly as a GEM object, the
> > > +        * mmap offset will not be already initialized.
> > > +        */
> > > +       ret = drm_gem_create_mmap_offset(obj);
> > > +       if (ret)
> > > +               return ret;
> >
> > Wouldn't it be better to have this call directly in the
> > drm_gem_prime_mmap() ? This way all drivers can be lazy.
> >
>
> yes.. that was my first[1] proposal.  But there are differences of
> opinion, and in the mean time I want to get this particular issue
> fixed ;-)

Ack, excuse me, I probably skipped the thread.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

>
> BR,
> -R
>
> [1] https://patchwork.freedesktop.org/patch/487597/
>
> >
> > > +
> > > +       return drm_gem_prime_mmap(obj, vma);
> > > +}
> > > +
> > >  struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > >  {
> > >         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > --
> > > 2.36.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry