Messages in this thread | | | Subject | Re: [PATCH] drm: Lock pointer access in drm_master_release() | From | Desmond Cheong Zhi Xi <> | Date | Fri, 11 Jun 2021 11:10:55 +0800 |
| |
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
| |