linux-kernel - Re: [PATCH 3/3] drm/vmwgfx: fix potential UAF in vmwgfx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 23 Jul 2021 14:44:35 +0800
From:   Desmond Cheong Zhi Xi <desmondcheongzx@...il.com>
To:     Zack Rusin <zackr@...are.com>,
        linux-graphics-maintainer@...are.com, airlied@...ux.ie,
        daniel@...ll.ch, maarten.lankhorst@...ux.intel.com,
        mripard@...nel.org, tzimmermann@...e.de
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, skhan@...uxfoundation.org,
        gregkh@...uxfoundation.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH 3/3] drm/vmwgfx: fix potential UAF in vmwgfx_surface.c

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@...il.com>
> 
> Reviewed-by: Zack Rusin <zackr@...are.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