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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 16 Nov 2022 13:49:43 +1000
From:   Dave Airlie <airlied@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Cc:     dri-devel@...ts.freedesktop.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michel Dänzer <michel@...nzer.net>,
        Christian Brauner <brauner@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel.vetter@...el.com>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        Sean Paul <sean@...rly.run>,
        Nicholas Kazlauskas <nicholas.kazlauskas@....com>
Subject: Re: [PATCH] drm/atomic: do not branch based on the value of current->comm[0]

On Sun, 6 Nov 2022 at 08:21, Jason A. Donenfeld <Jason@...c4.com> 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.

I've been ignoring this because I don't really want to reintroduce a
regression for deployed X servers. I don't see the value.

You use a lot of what I'd call overly not backed up language. Why is
it obviously wrong though? Is it "playing games" or is it accessing
the comm to see if the process starts with X.

Do we have lots of userspace processes starting with X that access
this specific piece of the drm modesetting API. I suppose we might and
if we have complaints about that I'd say let's try to fix it better.

Sometimes engineering is hard, It was a big enough problem that a big
enough hammer was used.

I'd hope @Daniel Vetter can comment as well since the original patch was his.

Dave.