linux-kernel - Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 16 Nov 2022 01:36:00 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     dri-devel@...ts.freedesktop.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     Daniel Vetter <daniel.vetter@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ilia Mirkin <imirkin@...m.mit.edu>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Nicholas Kazlauskas <nicholas.kazlauskas@....com>,
        Christian Brauner <brauner@...nel.org>,
        Michel Dänzer <michel@...nzer.net>,
        Alex Deucher <alexdeucher@...il.com>,
        Adam Jackson <ajax@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Rob Clark <robdclark@...il.com>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] drm/atomic: do not branch based on the value of
 current->comm[0]

Hi David,

I'm a bit surprised that this patch was ignored. I had sort of assumed
the response would be, "good god, I can't believe we're doing this.
Applied to drm-next!" rather than crickets, but maybe it just got buried
under a lot of other patches. So I thought I'd poke about this again.
The original message is reproduced in full below.

Regards,
Jason

On Sat, Nov 05, 2022 at 11:20:12PM +0100, Jason A. Donenfeld wrote:
> This reverts 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from
> X"), a rootkit-like kludge that has no business being inside of a
> general purpose kernel. It's the type of debugging hack I'll use
> momentarily but never commit, or a sort of babbies-first-process-hider
> malware trick.
> 
> The backstory is that some userspace code -- xorg-server -- has a
> modesetting DDX that isn't really coded right. With nobody wanting to
> maintain X11 anymore, rather than fixing the buggy code, the kernel was
> adjusted to avoid having to touch X11. A bummer, but fair enough: if the
> kernel doesn't want to support some userspace API any more, the right
> thing to do is to arrange for a graceful fallback where userspace thinks
> it's not available in a manageable way.
> 
> However, the *way* it goes about doing that is just to check
> `current->comm[0] == 'X'`, and disable it for only that case. So that
> means it's *not* simply a matter of the kernel not wanting to support a
> particular userspace API anymore, but rather it's the kernel not wanting
> to support xorg-server, in theory, but actually, it turns out, that's
> all processes that begin with 'X'.
> 
> Playing games with current->comm like this is obviously wrong, and it's
> pretty shocking that this ever got committed.
> 
> Fortunately, since this was committed, somebody did actually disable
> the userspace side by default in X11:
> https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/180 and
> this was three years ago. So userspace is mostly fine now for ordinary
> default usage. And people who opt into this -- since it does actually
> work fine for many use cases on i915 -- ostensibly know what they're
> getting themselves into (my case).
> 
> So let's just revert this `comm[0] == 'X'` business entirely, but still
> allow for `value == 2`, in case anybody actually started working on that
> part elsewhere.
> 
> Fixes: 26b1d3b527e7 ("drm/atomic: Take the atomic toys away from X")
> Cc: Daniel Vetter <daniel.vetter@...el.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Ilia Mirkin <imirkin@...m.mit.edu>
> Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@....com>
> Cc: Christian Brauner <brauner@...nel.org>
> Cc: Michel Dänzer <michel@...nzer.net>
> Cc: Alex Deucher <alexdeucher@...il.com>
> Cc: Adam Jackson <ajax@...hat.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Sean Paul <sean@...rly.run>
> Cc: David Airlie <airlied@...ux.ie>
> Cc: Rob Clark <robdclark@...il.com>
> Cc: Sultan Alsawaf <sultan@...neltoast.com>
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..017f31e67179 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -336,11 +336,6 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  	case DRM_CLIENT_CAP_ATOMIC:
>  		if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
>  			return -EOPNOTSUPP;
> -		/* The modesetting DDX has a totally broken idea of atomic. */
> -		if (current->comm[0] == 'X' && req->value == 1) {
> -			pr_info("broken atomic modeset userspace detected, disabling atomic\n");
> -			return -EOPNOTSUPP;
> -		}
>  		if (req->value > 2)
>  			return -EINVAL;
>  		file_priv->atomic = req->value;
> -- 
> 2.38.1
>