login: fixed PID matching when PAM (and forking) used

[Karlson2k]

An entry with parent process PID is also valid for the current entry, specially when PAM is used as login fork itself before updating utmp.
Without this patch a duplicated utmp entry is produced each time (one entry is LOGIN_PROCESS and additional entry is USER_PROCESS).

[Karlson2k]

Updated as requested.

[alejandro-colomar]

An entry with parent process PID is also valid for the current entry, specially when PAM is used as login fork itself before updating utmp. Without this patch a duplicated utmp entry is produced each time (one entry is LOGIN_PROCESS and additional entry is USER_PROCESS).

Would you mind showing an example of a shell session where you show the problem, and another with the fixed program where it's solved? That would help review this.

Also, if you could link to some documentation that confirms the first sentence, it would also help review.

Thanks!

[Karlson2k]

Here is an example of work of unpatched login.
Performed in tty3 on Gentoo with systemd, parallel login in KDE. shadow built without patch, without logind support, with PAM support.

 $ logname
LOGIN

 $ utmpdump /run/utmp
Utmp dump of /run/utmp
[2] [00000] [~~  ] [reboot  ] [~           ] [6.15.5-gentoo-k2k-special10] [0.0.0.0        ] [2025-07-10T14:46:16,621237+00:00]
[7] [00796] [    ] [k2k     ] [tty2        ] [:0                  ] [0.0.0.0        ] [2025-07-10T14:46:18,945439+00:00]
[7] [00900] [ts/0] [k2k     ] [pts/0       ] [:0                  ] [0.0.0.0        ] [2025-07-10T14:46:19,557053+00:00]
[6] [118299] [tty3] [LOGIN   ] [tty3        ] [                    ] [0.0.0.0        ] [2025-07-11T18:20:23,728441+00:00]
[7] [118306] [3   ] [k2k     ] [tty3        ] [                    ] [0.0.0.0        ] [2025-07-11T18:20:34,419835+00:00]

 $ pstree -aps $$
systemd,1
  └─login,118299 --
      └─bash,118306
          └─pstree,118400 -aps 118306

 $ systemctl status getty@tty3.service
■ getty@tty3.service - Getty on tty3
     Loaded: loaded (/usr/lib/systemd/system/getty@.service; disabled; preset: enabled)
     Active: active (running) since Fri 2025-07-11 20:20:23 CEST; 1min 0s ago
 Invocation: 29c9b16245d2470182603a413b2f213e
       Docs: man:agetty(8)
             man:systemd-getty-generator(8)
             https://0pointer.de/blog/projects/serial-console.html
   Main PID: 118299 (login)
      Tasks: 0 (limit: 112791)
     Memory: 688K (peak: 1.8M)
        CPU: 18ms
     CGroup: /system.slice/system-getty.slice/getty@tty3.service
             ■ 118299 /bin/login --

The next example is on the same system, but with purposed patch:

 $ logname
k2k

 $ utmpdump /run/utmp
Utmp dump of /run/utmp
[2] [00000] [~~  ] [reboot  ] [~           ] [6.15.5-gentoo-k2k-special10] [0.0.0.0        ] [2025-07-10T14:46:16,621237+00:00]
[7] [00796] [    ] [k2k     ] [tty2        ] [:0                  ] [0.0.0.0        ] [2025-07-10T14:46:18,945439+00:00]
[7] [00900] [ts/0] [k2k     ] [pts/0       ] [:0                  ] [0.0.0.0        ] [2025-07-10T14:46:19,557053+00:00]
[7] [99316] [tty3] [k2k     ] [tty3        ] [                    ] [0.0.0.0        ] [2025-07-11T18:07:06,899838+00:00]

 $ pstree -aps $$
systemd,1
  └─login,3777 --
      └─bash,99316
          └─pstree,100149 -aps 99316

 $ systemctl status getty@tty3.service
■ getty@tty3.service - Getty on tty3
     Loaded: loaded (/usr/lib/systemd/system/getty@.service; disabled; preset: enabled)
     Active: active (running) since Thu 2025-07-10 16:53:45 CEST; 1 day 3h ago
 Invocation: 9167c89332fc4a4b9eaafc2b50784d3e
       Docs: man:agetty(8)
             man:systemd-getty-generator(8)
             https://0pointer.de/blog/projects/serial-console.html
   Main PID: 3777 (login)
      Tasks: 0 (limit: 112791)
     Memory: 776K (peak: 2.2M)
        CPU: 19ms
     CGroup: /system.slice/system-getty.slice/getty@tty3.service
             ■ 3777 /bin/login --

When login binary has PAM enabled, it forks itself before calling update_utmp(). Obviously, update_utmp() cannot find any matches when used without patch.

shadow/src/login.c

Lines 1086 to 1128 in 36debf3

	#ifdef USE_PAM
	/*
	* We must fork before setuid() because we need to call
	* pam_close_session() as root.
	*/
	(void) signal (SIGINT, SIG_IGN);
	child = fork ();
	if (child < 0) {
	/* error in fork() */
	fprintf (stderr, _("%s: failure forking: %s"),
	Prog, strerror (errno));
	PAM_END;
	exit (0);
	} else if (child != 0) {
	/*
	* parent - wait for child to finish, then cleanup
	* session
	*/
	wait (NULL);
	PAM_END;
	exit (0);
	}
	/* child */
	#endif
	/* If we were init, we need to start a new session */
	if (getppid() == 1) {
	setsid();
	if (ioctl(0, TIOCSCTTY, 1) != 0) {
	fprintf (stderr, _("TIOCSCTTY failed on %s"), tty);
	}
	}
	#ifndef ENABLE_LOGIND
	/*
	* The utmp entry needs to be updated to indicate the new status
	* of the session, the new PID and SID.
	*/
	err = update_utmp (username, tty, hostname);
	if (err != 0) {
	SYSLOG ((LOG_WARN, "Unable to update utmp entry for %s", username));
	}
	#endif /* ENABLE_LOGIND */

Formally, match with parent PID could be guarded with #ifdef USE_PAM, but I don't see a reason why PID with matching parent PID on the same tty should not be replaced.

Strictly speaking, the we can try to match own PID only when PAM is not used, and parent PID only when PAM is used.

[alejandro-colomar]

Sounds reasonable. Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

I'd like a second review before merging, though. I'm not familiar enough with this code.

Cc: @ikerexxe , @hallyn

[alejandro-colomar]

Cc: @thesamesam

[Karlson2k]

Updated to have a stricter matching:

• only parent PID when PAM is used (fork() used for the current process)
• only own PID when PAM is not used (the current process is the originally started process)

[Karlson2k]

Added a second commit for one more similar bug

[alejandro-colomar]

Updated to have a stricter matching:

* only parent PID when PAM is used (fork() used for the current process)

* only own PID when PAM is not used (the current process is the originally started process)

TBH, I preferred the previous approach. I hate preprocessor conditionals. Is it incorrect to do this for non-PAM?

[Karlson2k]

TBH, I preferred the previous approach. I hate preprocessor conditionals. Is it incorrect to do this for non-PAM?

Changed back, added more explanations to the commit message.
When not forking, there is no chance that parent ID will accidentally match wrong entry with the same TTY, so it should be safe to check both (parent and own PIDs).

[Karlson2k]

Moved second commit to separate PR (#1286) to unblock this one.

[alejandro-colomar]

TBH, I preferred the previous approach. I hate preprocessor conditionals. Is it incorrect to do this for non-PAM?

Changed back, added more explanations to the commit message. When not forking, there is no chance that parent ID will accidentally match wrong entry with the same TTY, so it should be safe to check both (parent and own PIDs).

Hmm, sounds reasonable.

What if the process was run from another user's account with something like su(1) from the same tty? Is it still impossible? (Just trying to rule out weird cases; I didn't think too much of it.)

[Karlson2k]

What if the process was run from another user's account with something like su(1) from the same tty? Is it still impossible? (Just trying to rule out weird cases; I didn't think too much of it.)

su would not update utmp entries.

[alejandro-colomar]

Sounds good. Is there any Gentoo bug associated with this? If so, please include it in the commit message. Else, I'll merge.

[Karlson2k]

Sounds good. Is there any Gentoo bug associated with this? If so, please include it in the commit message. Else, I'll merge.

No, I haven't open a Gentoo bug as it is a clear upstream bug.
No downstream bugs to link.

[alejandro-colomar]

Thanks! I'll merge, then.

[hallyn]

(sorry I'm late, but)

Is there a chance that in fact both pid and ppid have (different) valid entries already? Would it be better to cache the ut if ut->ut->pid == getppid(), keep running through the list, and then only return the cached ut if no ut->ut_pid == getpid() was found?

[alejandro-colomar]

@Karlson2k

[Karlson2k]

My reply is here: #1282 (comment)

[Karlson2k]

Probably all PID checks are redundant.
I do not really see how two processes may have valid utmp entries with the same tty simultaneously.
Probably any login and user entry with the same tty must be replaced. For example, utmp with the same TTY and dead process must be replaced if found.

Or a safer (for any reason) approach:

• when login does not fork itself: check own PID first and then parent PID (for the case when atty forks before starting login).
• when login does fork itself: check parent PID first and then grandparent PID (for the case when atty forks before starting login). No need to check own PID as it is the freshly started process.
• in any case: any entry with dead process and the same tty must be replaced.

[Karlson2k]

Actually utmp handling is described here: https://man7.org/linux/man-pages/man5/utmp.5.html
When PAM (and forking) has been introduced, it broke correct utmp processing as utmp must not have records for "forked" processes. It should has only records of the original "login" process.

I'll add another PR with correction to utmp handling (to bring it back to the originally expected behaviour).

[Karlson2k]

More fixes are here