Messages in this thread | | | Subject | Re: [PATCH v2 2/2] drm: Protect drm_master pointers in drm_lease.c | From | Desmond Cheong Zhi Xi <> | Date | Fri, 18 Jun 2021 11:05:04 +0800 |
| |
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.
Best wishes, Desmond
| |