Re: [PATCH v2 2/2] drm: Protect drm_master pointers in drm_lease.c

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 18, 2021 at 5:05 AM Desmond Cheong Zhi Xi
<desmondcheongzx@gmail.com> wrote:
> On 18/6/21 1:12 am, Daniel Vetter wrote:
> > On Tue, Jun 15, 2021 at 10:36:45AM +0800, Desmond Cheong Zhi Xi wrote:
> >> This patch ensures that the device's master mutex is acquired before
> >> accessing pointers to struct drm_master that are subsequently
> >> dereferenced. Without the mutex, the struct drm_master may be freed
> >> concurrently by another process calling drm_setmaster_ioctl(). This
> >> could then lead to use-after-free errors.
> >>
> >> Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> >> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> >> ---
> >>   drivers/gpu/drm/drm_lease.c | 58 +++++++++++++++++++++++++++----------
> >>   1 file changed, 43 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> >> index da4f085fc09e..3e6f689236e5 100644
> >> --- a/drivers/gpu/drm/drm_lease.c
> >> +++ b/drivers/gpu/drm/drm_lease.c
> >> @@ -107,10 +107,16 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> >>    */
> >>   bool _drm_lease_held(struct drm_file *file_priv, int id)
> >>   {
> >> +    bool ret;
> >> +
> >>      if (!file_priv || !file_priv->master)
> >>              return true;
> >>
> >> -    return _drm_lease_held_master(file_priv->master, id);
> >> +    mutex_lock(&file_priv->master->dev->master_mutex);
> >
> > So maybe we have a bug somewhere, and the kerneldoc isn't 100% clear, but
> > I thought file_priv->master is invariant over the lifetime of file_priv.
> > So we don't need a lock to check anything here.
> >
> > It's the drm_device->master derefence that gets us into trouble. Well also
> > file_priv->is_owner is protected by dev->master_mutex.
> >
> > So I think with your previous patch all the access here in drm_lease.c is
> > ok and already protected? Or am I missing something?
> >
> > Thanks, Daniel
> >
>
> My thinking was that file_priv->master is invariant only if it is the
> creator of master. If file_priv->is_master is false, then a call to
> drm_setmaster_ioctl will invoke drm_new_set_master, which then allocates
> a new master for file_priv, and puts the old master.
>
> This could be an issue in _drm_lease_held_master, because we dereference
> master to get master->dev, master->lessor, and master->leases.
>
> With the same reasoning, in other parts of drm_lease.c, if there's an
> access to drm_file->master that's subsequently dereferenced, I added a
> lock around them.
>
> I could definitely be mistaken on this, so apologies if this scenario
> doesn't arise.

You're right, I totally missed that setmaster can create a new master
instance. And the kerneldoc for drm_file->master doesn't explain this
and mention that we must hold drm_device.master_mutex while looking at
that pointer. Can you pls do a patch which improves the documentation
for that?

Now for the patch itself I'm not entirely sure what we should do.
Leaking the dev->master_mutex into drm_lease.c just because of the
setmaster ioctl is kinda unsightly. And we don't really care about the
fpriv->master changing under us, we only need to make sure it doesn't
get freed. And drm_master is refcounted already.

So alternative solution: We add a drm_file_get_master() function which
calls drm_master_get under the lock, and we use that instead of
directly derefencing drm_file->master? Ofc then needs drm_master_put
instead of mutex_unlock. Kerneldoc should then also point at this new
function as the correct way to look at drm_file->master state.

This way it's 100% clear we're dealing with a lifetime issue and not a
consistency issues.

What do you think?
-Daniel


>
> Best wishes,
> Desmond
>
>
>


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