Re: [PATCH] drm: Return -ENOTTY for non-drm ioctls

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 16, 2021 at 05:43:12PM +0100, Charles Baylis wrote:
> 
> Hi
> 
> The attached patch fixes a problem where non-drm ioctls are incorrectly
> handled by drm drivers. 
> 
> This causes problems when isatty() is called on a file descriptor which
> was opened on a drm device node. Glibc implements isatty() by invoking
> the TCGETS ioctl on the fd. TCGETS is 0x5401, so this is handled by drm_ioctl
> as DRM_IOCTL_GET_UNIQUE, which succeeds, so isatty() returns true.
> 
> As a side effect, DRM_IOCTL_GET_UNIQUE also writes to a pointer, in the
> argument buffer, so the calling application's memory can be corrupted, causing
> a crash later.
> 
> Tested on an Ubuntu 20.10 VM under qemu with virgl:
> * "if [ -t 0 ]; then echo is a tty; fi < /dev/dri/card0" outputs nothing
> * glxgears still works
> 
> Thanks
> Charles

> commit 0072b12086cdf7350df75d36a12d392e3ba92865
> Author: Charles Baylis <cb-kernel@fishzet.co.uk>
> Date:   Fri Jul 16 11:58:47 2021 +0100
> 
>     drm: Return -ENOTTY for non-drm ioctls
>     
>     Return -ENOTTY from drm_ioctl() when userspace passes in a cmd number
>     which doesn't relate to the drm subsystem.
>     
>     Glibc uses the TCGETS ioctl to implement isatty(), and without this
>     change isatty() returns it incorrectly returns true for drm devices.
>     
>     To test run this command:
>        $ if [ -t 0 ]; then echo is a tty; fi < /dev/dri/card0
>     which shows "is a tty" without this patch.
>     
>     This may also modify memory which the userspace application is not
>     expecting.

I fixed up the formatting here a bit to get git apply-mbox to accept this,
please try to use git send-email. Anyway this is bug is as old as drm, did
you see whether other subsystems are buggy like this too?

Patch applied to drm-misc-fixes with cc: stable.

Since this is pretty blantant issue, care to also type up an igt tests to
verify this stays fixed?

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#validating-changes-with-igt

Cheers, Daniel
>     
>     Signed-off-by: Charles Baylis <cb-kernel@fishzet.co.uk>
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 98ae00661656..f454e0424086 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -834,6 +834,9 @@ long drm_ioctl(struct file *filp,
>  	if (drm_dev_is_unplugged(dev))
>  		return -ENODEV;
>  
> +       if (DRM_IOCTL_TYPE(cmd) != DRM_IOCTL_BASE)
> +               return -ENOTTY;
> +
>  	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>  
>  	if (is_driver_ioctl) {
> diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
> index 10100a4bbe2a..afb27cb6a7bd 100644
> --- a/include/drm/drm_ioctl.h
> +++ b/include/drm/drm_ioctl.h
> @@ -68,6 +68,7 @@ typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd,
>  			       unsigned long arg);
>  
>  #define DRM_IOCTL_NR(n)                _IOC_NR(n)
> +#define DRM_IOCTL_TYPE(n)              _IOC_TYPE(n)
>  #define DRM_MAJOR       226
>  
>  /**


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