Messages in this thread | | | Subject | Re: [PATCH 3/3] drm/vmwgfx: fix potential UAF in vmwgfx_surface.c | From | Desmond Cheong Zhi Xi <> | Date | Fri, 23 Jul 2021 14:44:35 +0800 |
| |
On 23/7/21 3:17 am, Zack Rusin wrote: > On 7/22/21 5:29 AM, Desmond Cheong Zhi Xi wrote: >> drm_file.master should be protected by either drm_device.master_mutex >> or drm_file.master_lookup_lock when being dereferenced. However, >> drm_master_get is called on unprotected file_priv->master pointers in >> vmw_surface_define_ioctl and vmw_gb_surface_define_internal. >> >> This is fixed by replacing drm_master_get with drm_file_get_master. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> > > Reviewed-by: Zack Rusin <zackr@vmware.com> > > Thanks for taking the time to fix this. Apart from the clear logic > error, do you happen to know under what circumstances would this be hit? > We have someone looking at writing some vmwgfx specific igt tests and I > was wondering if I could add this to the list. > > z
Hi Zack,
Thanks for the review.
For some context, the use-after-free happens when there's a race between accessing the value of drm_file.master, and a call to drm_setmaster_ioctl. If drm_file is not the creator of master, then the ioctl allocates a new master for drm_file and puts the old master.
Thus for example, the old value of drm_file.master could be freed in between getting the value of file_priv->master, and the call to drm_master_get.
I'm not entirely sure whether this scenario is a good candidate for a test?
For further reference, the issue was originally caught by Syzbot here: https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f803
And from the logs it seems that the reproducer set up a race between DRM_IOCTL_GET_UNIQUE and DRM_IOCTL_SET_MASTER. So possibly a race between VMW_CREATE_SURFACE and DRM_IOCTL_SET_MASTER could trigger the same bug.
Best wishes, Desmond
| |