| Message ID | 20210609092119.173590-1-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Lock pointer access in drm_master_release() | expand |
On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: > This patch eliminates the following smatch warning: > drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' > > The 'file_priv->master' field should be protected by the mutex lock to > '&dev->master_mutex'. This is because other processes can concurrently > modify this field and free the current 'file_priv->master' > pointer. This could result in a use-after-free error when 'master' is > dereferenced in subsequent function calls to > 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. > > An example of a scenario that would produce this error can be seen > from a similar bug in 'drm_getunique()' that was reported by Syzbot: > https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 > > In the Syzbot report, another process concurrently acquired the > device's master mutex in 'drm_setmaster_ioctl()', then overwrote > 'fpriv->master' in 'drm_new_set_master()'. The old value of > 'fpriv->master' was subsequently freed before the mutex was unlocked. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> Thanks a lot. I've done an audit of this code, and I found another potential problem in drm_is_current_master. The callers from drm_auth.c hold the dev->master_mutex, but all the external ones dont. I think we need to split this into a _locked function for use within drm_auth.c, and the exported one needs to grab the dev->master_mutex while it's checking master status. Ofc there will still be races, those are ok, but right now we run the risk of use-after free problems in drm_lease_owner. Are you up to do that fix too? I think the drm_lease.c code also needs an audit, there we'd need to make sure that we hold hold either the lock or a full master reference to avoid the use-after-free issues here. Patch merged to drm-misc-fixes with cc: stable. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index f00e5abdbbf4..b59b26a71ad5 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) > void drm_master_release(struct drm_file *file_priv) > { > struct drm_device *dev = file_priv->minor->dev; > - struct drm_master *master = file_priv->master; > + struct drm_master *master; > > mutex_lock(&dev->master_mutex); > + master = file_priv->master; > if (file_priv->magic) > idr_remove(&file_priv->master->magic_map, file_priv->magic); > > -- > 2.25.1 >
On 10/6/21 6:10 pm, Daniel Vetter wrote: > On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: >> This patch eliminates the following smatch warning: >> drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' >> >> The 'file_priv->master' field should be protected by the mutex lock to >> '&dev->master_mutex'. This is because other processes can concurrently >> modify this field and free the current 'file_priv->master' >> pointer. This could result in a use-after-free error when 'master' is >> dereferenced in subsequent function calls to >> 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. >> >> An example of a scenario that would produce this error can be seen >> from a similar bug in 'drm_getunique()' that was reported by Syzbot: >> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 >> >> In the Syzbot report, another process concurrently acquired the >> device's master mutex in 'drm_setmaster_ioctl()', then overwrote >> 'fpriv->master' in 'drm_new_set_master()'. The old value of >> 'fpriv->master' was subsequently freed before the mutex was unlocked. >> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > Thanks a lot. I've done an audit of this code, and I found another > potential problem in drm_is_current_master. The callers from drm_auth.c > hold the dev->master_mutex, but all the external ones dont. I think we > need to split this into a _locked function for use within drm_auth.c, and > the exported one needs to grab the dev->master_mutex while it's checking > master status. Ofc there will still be races, those are ok, but right now > we run the risk of use-after free problems in drm_lease_owner. > > Are you up to do that fix too? > Hi Daniel, Thanks for the pointer, I'm definitely up for it! > I think the drm_lease.c code also needs an audit, there we'd need to make > sure that we hold hold either the lock or a full master reference to avoid > the use-after-free issues here. > I'd be happy to look into drm_lease.c as well. > Patch merged to drm-misc-fixes with cc: stable. > -Daniel > >> --- >> drivers/gpu/drm/drm_auth.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c >> index f00e5abdbbf4..b59b26a71ad5 100644 >> --- a/drivers/gpu/drm/drm_auth.c >> +++ b/drivers/gpu/drm/drm_auth.c >> @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) >> void drm_master_release(struct drm_file *file_priv) >> { >> struct drm_device *dev = file_priv->minor->dev; >> - struct drm_master *master = file_priv->master; >> + struct drm_master *master; >> >> mutex_lock(&dev->master_mutex); >> + master = file_priv->master; >> if (file_priv->magic) >> idr_remove(&file_priv->master->magic_map, file_priv->magic); >> >> -- >> 2.25.1 >> > From what I can see, there are other places in the kernel that could use the _locked version of drm_is_current_master as well, such as drm_mode_getfb in drm_framebuffer.c. I'll take a closer look, and if the changes make sense I'll prepare a patch series for them. Best wishes, Desmond
On Thu, Jun 10, 2021 at 11:21:39PM +0800, Desmond Cheong Zhi Xi wrote: > On 10/6/21 6:10 pm, Daniel Vetter wrote: > > On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: > > > This patch eliminates the following smatch warning: > > > drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' > > > > > > The 'file_priv->master' field should be protected by the mutex lock to > > > '&dev->master_mutex'. This is because other processes can concurrently > > > modify this field and free the current 'file_priv->master' > > > pointer. This could result in a use-after-free error when 'master' is > > > dereferenced in subsequent function calls to > > > 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. > > > > > > An example of a scenario that would produce this error can be seen > > > from a similar bug in 'drm_getunique()' that was reported by Syzbot: > > > https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 > > > > > > In the Syzbot report, another process concurrently acquired the > > > device's master mutex in 'drm_setmaster_ioctl()', then overwrote > > > 'fpriv->master' in 'drm_new_set_master()'. The old value of > > > 'fpriv->master' was subsequently freed before the mutex was unlocked. > > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > > > Thanks a lot. I've done an audit of this code, and I found another > > potential problem in drm_is_current_master. The callers from drm_auth.c > > hold the dev->master_mutex, but all the external ones dont. I think we > > need to split this into a _locked function for use within drm_auth.c, and > > the exported one needs to grab the dev->master_mutex while it's checking > > master status. Ofc there will still be races, those are ok, but right now > > we run the risk of use-after free problems in drm_lease_owner. > > > > Are you up to do that fix too? > > > > Hi Daniel, > > Thanks for the pointer, I'm definitely up for it! > > > I think the drm_lease.c code also needs an audit, there we'd need to make > > sure that we hold hold either the lock or a full master reference to avoid > > the use-after-free issues here. > > > > I'd be happy to look into drm_lease.c as well. > > > Patch merged to drm-misc-fixes with cc: stable. > > -Daniel > > > > > --- > > > drivers/gpu/drm/drm_auth.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > > > index f00e5abdbbf4..b59b26a71ad5 100644 > > > --- a/drivers/gpu/drm/drm_auth.c > > > +++ b/drivers/gpu/drm/drm_auth.c > > > @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) > > > void drm_master_release(struct drm_file *file_priv) > > > { > > > struct drm_device *dev = file_priv->minor->dev; > > > - struct drm_master *master = file_priv->master; > > > + struct drm_master *master; > > > > > > mutex_lock(&dev->master_mutex); > > > + master = file_priv->master; > > > if (file_priv->magic) > > > idr_remove(&file_priv->master->magic_map, file_priv->magic); > > > -- > > > 2.25.1 > > > > > > > From what I can see, there are other places in the kernel that could use the > _locked version of drm_is_current_master as well, such as drm_mode_getfb in > drm_framebuffer.c. I'll take a closer look, and if the changes make sense > I'll prepare a patch series for them. Oh maybe we have a naming confusion: the _locked is the one where the caller must grab the lock already, whereas drm_is_current_master would grab the master_mutex internally to do the check. The one in drm_framebuffer.c looks like it'd need the internal one since there's no other need to grab the master_mutex. -Daniel
On Thu, 10 Jun 2021 at 11:10, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: > > This patch eliminates the following smatch warning: > > drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' > > > > The 'file_priv->master' field should be protected by the mutex lock to > > '&dev->master_mutex'. This is because other processes can concurrently > > modify this field and free the current 'file_priv->master' > > pointer. This could result in a use-after-free error when 'master' is > > dereferenced in subsequent function calls to > > 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. > > > > An example of a scenario that would produce this error can be seen > > from a similar bug in 'drm_getunique()' that was reported by Syzbot: > > https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 > > > > In the Syzbot report, another process concurrently acquired the > > device's master mutex in 'drm_setmaster_ioctl()', then overwrote > > 'fpriv->master' in 'drm_new_set_master()'. The old value of > > 'fpriv->master' was subsequently freed before the mutex was unlocked. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > Thanks a lot. I've done an audit of this code, and I found another > potential problem in drm_is_current_master. The callers from drm_auth.c > hold the dev->master_mutex, but all the external ones dont. I think we > need to split this into a _locked function for use within drm_auth.c, and > the exported one needs to grab the dev->master_mutex while it's checking > master status. Ofc there will still be races, those are ok, but right now > we run the risk of use-after free problems in drm_lease_owner. > Note that some code does acquire the mutex via drm_master_internal_acquire - so we should be careful. As mentioned elsewhere - having a _locked version of drm_is_current_master sounds good. Might as well throw a lockdep_assert_held_once in there just in case :-P Happy to help review the follow-up patches. -Emil
On 11/6/21 12:48 am, Daniel Vetter wrote: > On Thu, Jun 10, 2021 at 11:21:39PM +0800, Desmond Cheong Zhi Xi wrote: >> On 10/6/21 6:10 pm, Daniel Vetter wrote: >>> On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: >>>> This patch eliminates the following smatch warning: >>>> drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' >>>> >>>> The 'file_priv->master' field should be protected by the mutex lock to >>>> '&dev->master_mutex'. This is because other processes can concurrently >>>> modify this field and free the current 'file_priv->master' >>>> pointer. This could result in a use-after-free error when 'master' is >>>> dereferenced in subsequent function calls to >>>> 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. >>>> >>>> An example of a scenario that would produce this error can be seen >>>> from a similar bug in 'drm_getunique()' that was reported by Syzbot: >>>> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 >>>> >>>> In the Syzbot report, another process concurrently acquired the >>>> device's master mutex in 'drm_setmaster_ioctl()', then overwrote >>>> 'fpriv->master' in 'drm_new_set_master()'. The old value of >>>> 'fpriv->master' was subsequently freed before the mutex was unlocked. >>>> >>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >>> >>> Thanks a lot. I've done an audit of this code, and I found another >>> potential problem in drm_is_current_master. The callers from drm_auth.c >>> hold the dev->master_mutex, but all the external ones dont. I think we >>> need to split this into a _locked function for use within drm_auth.c, and >>> the exported one needs to grab the dev->master_mutex while it's checking >>> master status. Ofc there will still be races, those are ok, but right now >>> we run the risk of use-after free problems in drm_lease_owner. >>> >>> Are you up to do that fix too? >>> >> >> Hi Daniel, >> >> Thanks for the pointer, I'm definitely up for it! >> >>> I think the drm_lease.c code also needs an audit, there we'd need to make >>> sure that we hold hold either the lock or a full master reference to avoid >>> the use-after-free issues here. >>> >> >> I'd be happy to look into drm_lease.c as well. >> >>> Patch merged to drm-misc-fixes with cc: stable. >>> -Daniel >>> >>>> --- >>>> drivers/gpu/drm/drm_auth.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c >>>> index f00e5abdbbf4..b59b26a71ad5 100644 >>>> --- a/drivers/gpu/drm/drm_auth.c >>>> +++ b/drivers/gpu/drm/drm_auth.c >>>> @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) >>>> void drm_master_release(struct drm_file *file_priv) >>>> { >>>> struct drm_device *dev = file_priv->minor->dev; >>>> - struct drm_master *master = file_priv->master; >>>> + struct drm_master *master; >>>> >>>> mutex_lock(&dev->master_mutex); >>>> + master = file_priv->master; >>>> if (file_priv->magic) >>>> idr_remove(&file_priv->master->magic_map, file_priv->magic); >>>> -- >>>> 2.25.1 >>>> >>> >> >> From what I can see, there are other places in the kernel that could use the >> _locked version of drm_is_current_master as well, such as drm_mode_getfb in >> drm_framebuffer.c. I'll take a closer look, and if the changes make sense >> I'll prepare a patch series for them. > > Oh maybe we have a naming confusion: the _locked is the one where the > caller must grab the lock already, whereas drm_is_current_master would > grab the master_mutex internally to do the check. The one in > drm_framebuffer.c looks like it'd need the internal one since there's no > other need to grab the master_mutex. > -Daniel > Ah ok got it, I think I confused myself earlier. Just to check, may I include you in a Reported-by: tag?
On 11/6/21 1:49 am, Emil Velikov wrote: > On Thu, 10 Jun 2021 at 11:10, Daniel Vetter <daniel@ffwll.ch> wrote: >> >> On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: >>> This patch eliminates the following smatch warning: >>> drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' >>> >>> The 'file_priv->master' field should be protected by the mutex lock to >>> '&dev->master_mutex'. This is because other processes can concurrently >>> modify this field and free the current 'file_priv->master' >>> pointer. This could result in a use-after-free error when 'master' is >>> dereferenced in subsequent function calls to >>> 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. >>> >>> An example of a scenario that would produce this error can be seen >>> from a similar bug in 'drm_getunique()' that was reported by Syzbot: >>> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 >>> >>> In the Syzbot report, another process concurrently acquired the >>> device's master mutex in 'drm_setmaster_ioctl()', then overwrote >>> 'fpriv->master' in 'drm_new_set_master()'. The old value of >>> 'fpriv->master' was subsequently freed before the mutex was unlocked. >>> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> >> Thanks a lot. I've done an audit of this code, and I found another >> potential problem in drm_is_current_master. The callers from drm_auth.c >> hold the dev->master_mutex, but all the external ones dont. I think we >> need to split this into a _locked function for use within drm_auth.c, and >> the exported one needs to grab the dev->master_mutex while it's checking >> master status. Ofc there will still be races, those are ok, but right now >> we run the risk of use-after free problems in drm_lease_owner. >> > Note that some code does acquire the mutex via > drm_master_internal_acquire - so we should be careful. > As mentioned elsewhere - having a _locked version of > drm_is_current_master sounds good. > > Might as well throw a lockdep_assert_held_once in there just in case :-P > > Happy to help review the follow-up patches. > -Emil > Thanks for the advice, Emil! I did a preliminary check on the code that calls drm_master_internal_acquire in drm_client_modeset.c and drm_fb_helper.c, and it doesn't seem like they eventually call drm_is_current_master. So we should be good on that front. lockdep_assert_held_once sounds good :) Best wishes, Desmond
On Fri, Jun 11, 2021 at 4:18 AM Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> wrote: On 11/6/21 12:48 am, Daniel Vetter wrote: > > On Thu, Jun 10, 2021 at 11:21:39PM +0800, Desmond Cheong Zhi Xi wrote: > >> On 10/6/21 6:10 pm, Daniel Vetter wrote: > >>> On Wed, Jun 09, 2021 at 05:21:19PM +0800, Desmond Cheong Zhi Xi wrote: > >>>> This patch eliminates the following smatch warning: > >>>> drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' > >>>> > >>>> The 'file_priv->master' field should be protected by the mutex lock to > >>>> '&dev->master_mutex'. This is because other processes can concurrently > >>>> modify this field and free the current 'file_priv->master' > >>>> pointer. This could result in a use-after-free error when 'master' is > >>>> dereferenced in subsequent function calls to > >>>> 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. > >>>> > >>>> An example of a scenario that would produce this error can be seen > >>>> from a similar bug in 'drm_getunique()' that was reported by Syzbot: > >>>> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 > >>>> > >>>> In the Syzbot report, another process concurrently acquired the > >>>> device's master mutex in 'drm_setmaster_ioctl()', then overwrote > >>>> 'fpriv->master' in 'drm_new_set_master()'. The old value of > >>>> 'fpriv->master' was subsequently freed before the mutex was unlocked. > >>>> > >>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > >>> > >>> Thanks a lot. I've done an audit of this code, and I found another > >>> potential problem in drm_is_current_master. The callers from drm_auth.c > >>> hold the dev->master_mutex, but all the external ones dont. I think we > >>> need to split this into a _locked function for use within drm_auth.c, and > >>> the exported one needs to grab the dev->master_mutex while it's checking > >>> master status. Ofc there will still be races, those are ok, but right now > >>> we run the risk of use-after free problems in drm_lease_owner. > >>> > >>> Are you up to do that fix too? > >>> > >> > >> Hi Daniel, > >> > >> Thanks for the pointer, I'm definitely up for it! > >> > >>> I think the drm_lease.c code also needs an audit, there we'd need to make > >>> sure that we hold hold either the lock or a full master reference to avoid > >>> the use-after-free issues here. > >>> > >> > >> I'd be happy to look into drm_lease.c as well. > >> > >>> Patch merged to drm-misc-fixes with cc: stable. > >>> -Daniel > >>> > >>>> --- > >>>> drivers/gpu/drm/drm_auth.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > >>>> index f00e5abdbbf4..b59b26a71ad5 100644 > >>>> --- a/drivers/gpu/drm/drm_auth.c > >>>> +++ b/drivers/gpu/drm/drm_auth.c > >>>> @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) > >>>> void drm_master_release(struct drm_file *file_priv) > >>>> { > >>>> struct drm_device *dev = file_priv->minor->dev; > >>>> - struct drm_master *master = file_priv->master; > >>>> + struct drm_master *master; > >>>> > >>>> mutex_lock(&dev->master_mutex); > >>>> + master = file_priv->master; > >>>> if (file_priv->magic) > >>>> idr_remove(&file_priv->master->magic_map, file_priv->magic); > >>>> -- > >>>> 2.25.1 > >>>> > >>> > >> > >> From what I can see, there are other places in the kernel that could use the > >> _locked version of drm_is_current_master as well, such as drm_mode_getfb in > >> drm_framebuffer.c. I'll take a closer look, and if the changes make sense > >> I'll prepare a patch series for them. > > > > Oh maybe we have a naming confusion: the _locked is the one where the > > caller must grab the lock already, whereas drm_is_current_master would > > grab the master_mutex internally to do the check. The one in > > drm_framebuffer.c looks like it'd need the internal one since there's no > > other need to grab the master_mutex. > > -Daniel > > > > Ah ok got it, I think I confused myself earlier. > > Just to check, may I include you in a Reported-by: tag? Sure. -Daniel
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index f00e5abdbbf4..b59b26a71ad5 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -315,9 +315,10 @@ int drm_master_open(struct drm_file *file_priv) void drm_master_release(struct drm_file *file_priv) { struct drm_device *dev = file_priv->minor->dev; - struct drm_master *master = file_priv->master; + struct drm_master *master; mutex_lock(&dev->master_mutex); + master = file_priv->master; if (file_priv->magic) idr_remove(&file_priv->master->magic_map, file_priv->magic);
This patch eliminates the following smatch warning: drivers/gpu/drm/drm_auth.c:320 drm_master_release() warn: unlocked access 'master' (line 318) expected lock '&dev->master_mutex' The 'file_priv->master' field should be protected by the mutex lock to '&dev->master_mutex'. This is because other processes can concurrently modify this field and free the current 'file_priv->master' pointer. This could result in a use-after-free error when 'master' is dereferenced in subsequent function calls to 'drm_legacy_lock_master_cleanup()' or to 'drm_lease_revoke()'. An example of a scenario that would produce this error can be seen from a similar bug in 'drm_getunique()' that was reported by Syzbot: https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803 In the Syzbot report, another process concurrently acquired the device's master mutex in 'drm_setmaster_ioctl()', then overwrote 'fpriv->master' in 'drm_new_set_master()'. The old value of 'fpriv->master' was subsequently freed before the mutex was unlocked. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- drivers/gpu/drm/drm_auth.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)