[systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt

[David Herrmann]

Hi

On Mon, Aug 11, 2014 at 5:05 PM, Olivier Brunel <jjk at jjacky.com> wrote:
> On 08/11/14 16:54, Lennart Poettering wrote:
>> On Mon, 11.08.14 16:39, Olivier Brunel (jjk at jjacky.com) wrote:
>>
>>>
>>> On 08/11/14 16:25, Lennart Poettering wrote:
>>>> On Fri, 08.08.14 20:45, Olivier Brunel (jjk at jjacky.com) wrote:
>>>>
>>>>> In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
>>>>> needed for things to work. However, we shouldn't "reset" it to root on
>>>>> session_restore_vt() since it could have in fact already been set to
>>>>> the user.
>>>>
>>>> I don't follow here, can't parse this. Could you please elaborate?
>>>
>>> I meant, before the call to session_prepare_vt() the owner of /dev/ttyX
>>> might not be root, but already set to the user. In which case setting it
>>> "back" to root might not be expected/best.
>>
>> But that sounds more as if session_restore_vt() should not be used as-is
>> as cleanup path for session_prepare_vt(), no?
>>
>>> E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user,
>>> then if a process try a TakeControl() and it failed (or after it's done)
>>> the ownership would be set to root, even though it wasn't actually root
>>> to begin with.
>>
>> Isn't this very theoretic? I mean, when does TakeControl() actually
>> really fail for you IRL?
>
> Right, this was noticed when trying to start a second rootless X, since
> with systemd-215 things currently fail, and the ownership of /dev/ttyX
> would then be switched/changed to root.

Wait, what? Can you please elaborate. Currently, only one process can
be controller at a time, and session_prepare_vt() is called *after*
the controller is set. Therefore, it is not called when you try
starting a second controller.

The only race I see is this:
 * start legacy Xserver (which doesn't use TakeControl()) which sets
user-id on the TTY
 * start new Xserver which uses TakeControl() (and thus calls
session_prepare_vt())
 * stop new Xserver (thus drop Control and reset the TTY)

In this case, the new xserver will try to start up, but fail as the
old one is still running. Therefore, it *might* call ReleaseControl()
and thus reset the TTY. However, you're not supposed to mix both and
this is not a legitimate use-case. I mean, the old server is root and
modifies the TTY by itself (without using systemd). Obviously, this is
racy.

>From a systemd-logind perspective, this is the same as if you run
"sudo chown xyz /dev/tty" manually.

> session_prepare_vt() could also check the owner first, and note what to
> reset to on session_restore_vt(), to effectively restore things as they
> were.

Even though I don't see any problem here, I'd be fine with such a
patch. Care to send one? Note that you probably need to store that
information in the session-file (session_save() / session_load()) too.

Thanks
David