[PATCH] drm: fix use-after-free read in drm_mode_create_lease_ioctl()

[PATCH] drm: fix use-after-free read in drm_mode_create_lease_ioctl()

[Jann Horn]

fd_install() moves the reference given to it into the file descriptor table
of the current process. If the current process is multithreaded, then
immediately after fd_install(), another thread can close() the file
descriptor and cause the file's resources to be cleaned up.

Since the reference to "lessee" is held by the file, we must not access
"lessee" after the fd_install() call.

As far as I can tell, to reach this codepath, the caller must have an open
file descriptor to a DRI device in master mode. I'm not sure what the
requirements for that are.

Signed-off-by: Jann Horn <jannh@google.com>
Fixes: 62884cd386b8 ("drm: Add four ioctls for managing drm mode object leases [v7]")
Cc: stable@vger.kernel.org
---
I'm not sure how to actually use this ioctl, so I have neither verified
the bug experimentally nor experimentally verified the fix. I would
appreciate it if someone could confirm my analysis.

There have been a number of fd_install() bugs over time; I think it's
probably time to rename fd_install() to fd_install_dropref(), or
something like that.

 drivers/gpu/drm/drm_lease.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index b54fb78a283c..b82da96ded5c 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -566,14 +566,14 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
 	lessee_priv->is_master = 1;
 	lessee_priv->authenticated = 1;
 
-	/* Hook up the fd */
-	fd_install(fd, lessee_file);
-
 	/* Pass fd back to userspace */
 	DRM_DEBUG_LEASE("Returning fd %d id %d\n", fd, lessee->lessee_id);
 	cl->fd = fd;
 	cl->lessee_id = lessee->lessee_id;
 
+	/* Hook up the fd */
+	fd_install(fd, lessee_file);
+
 	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
 	return 0;
 
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH] drm: fix use-after-free read in drm_mode_create_lease_ioctl()

[Daniel Vetter]

On Mon, Oct 01, 2018 at 05:31:17PM +0200, Jann Horn wrote:
> fd_install() moves the reference given to it into the file descriptor table
> of the current process. If the current process is multithreaded, then
> immediately after fd_install(), another thread can close() the file
> descriptor and cause the file's resources to be cleaned up.
> 
> Since the reference to "lessee" is held by the file, we must not access
> "lessee" after the fd_install() call.
> 
> As far as I can tell, to reach this codepath, the caller must have an open
> file descriptor to a DRI device in master mode. I'm not sure what the
> requirements for that are.
> 
> Signed-off-by: Jann Horn <jannh@google.com>
> Fixes: 62884cd386b8 ("drm: Add four ioctls for managing drm mode object leases [v7]")
> Cc: stable@vger.kernel.org
> ---
> I'm not sure how to actually use this ioctl, so I have neither verified
> the bug experimentally nor experimentally verified the fix. I would
> appreciate it if someone could confirm my analysis.
> 
> There have been a number of fd_install() bugs over time; I think it's
> probably time to rename fd_install() to fd_install_dropref(), or
> something like that.

Publishing an object to the world needs to happen last, only once it's
fully set up. It's unfortunately a very common bug, and definitely not
limited to use-after-free fun. E.g. here you could also confuse the kernel
if you manage to sneak in an ioctl on the new fd, while it's not yet fully
ready for those. fd_install() is just one of these.

Except review and maybe automatic analysis tools for the common I'm not
sure how to catch these better. Because the race is generally small,
tests&fuzzing tend to not hit these.

Thanks a lot for spotting this issue, patch applied.

Cheers, Daniel
 
>  drivers/gpu/drm/drm_lease.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index b54fb78a283c..b82da96ded5c 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -566,14 +566,14 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	lessee_priv->is_master = 1;
>  	lessee_priv->authenticated = 1;
>  
> -	/* Hook up the fd */
> -	fd_install(fd, lessee_file);
> -
>  	/* Pass fd back to userspace */
>  	DRM_DEBUG_LEASE("Returning fd %d id %d\n", fd, lessee->lessee_id);
>  	cl->fd = fd;
>  	cl->lessee_id = lessee->lessee_id;
>  
> +	/* Hook up the fd */
> +	fd_install(fd, lessee_file);
> +
>  	DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n");
>  	return 0;
>  
> -- 
> 2.19.0.605.g01d371f741-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch