Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 2/2] drm: Protect drm_master pointers in drm_lease.c
To:     maarten.lankhorst@linux.intel.com, mripard@kernel.org,
        tzimmermann@suse.de, airlied@linux.ie,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        skhan@linuxfoundation.org, gregkh@linuxfoundation.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        emil.l.velikov@gmail.com
References: <20210615023645.6535-1-desmondcheongzx@gmail.com>
 <20210615023645.6535-3-desmondcheongzx@gmail.com>
 <YMuCYqLafn5sGcFo@phenom.ffwll.local>
From:   Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Message-ID: <c384d835-d910-5b04-e88c-a7878ce6d37d@gmail.com>
Date:   Fri, 18 Jun 2021 11:05:04 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <YMuCYqLafn5sGcFo@phenom.ffwll.local>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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