On 18/6/21 5:12 pm, Daniel Vetter wrote:
On Fri, Jun 18, 2021 at 5:05 AM Desmond Cheong Zhi Xi
<desmondcheongzx@xxxxxxxxx> 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@xxxxxxxx>
Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@xxxxxxxxx>
Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
---
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?
Sounds good, I'll add it to the patch series.
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
Makes sense to me, since the drm master itself holds the lease, as long
as it isn't freed while we're using it, there's no need to prevent the
value of fpriv->master from changing after we access it in drm_lease.c.
I was going to say that it may be unclear when to use the
master = drm_file_get_master(file_priv);
...
drm_master_put(&master);
pattern, versus when to use
mutex_lock(&dev->master_mutex);
master = file_priv->master;
...
mutex_unlock(&dev->master_mutex);
. The second pattern, for example, is used in drm_getunique, and also in
drm_setversion which calls drm_set_busid.
But on closer inspection, it's clearer to me now that those functions
need the master_mutex because they access protected fields such as
unique and unique_len.
Would it then be correct to state in the kerneldoc that
drm_file_get_master() should be used to look at drm_file->master only if
we aren't already holding master_mutex + have no other need to grab
master_mutex?
