[PATCH 1/2] tty: tty_io: update timestamps on all device nodes

[PATCH 1/2] tty: tty_io: update timestamps on all device nodes

[Michal Sekletar]

User space applications watch for timestamp changes on character device
files in order to determine idle time of a given terminal session. For
example, "w" program uses this information to populate the IDLE column
of its output [1]. Similarly, systemd-logind has optional feature where
it uses atime of the tty character device to determine if there was
activity on the terminal associated with the logind's session object. If
there was no activity for a configured period of time then logind will
terminate such session [2].

Now, usually (e.g. bash running on the terminal) the use of the terminal
will update timestamps (atime and mtime) on the corresponding terminal
character device. However, if access to the terminal, e.g. /dev/pts/0,
is performed through magic character device /dev/tty then such access
obviously changes the state of the terminal, however timestamps on the
device that correspond to the terminal (/dev/pts/0) are not updated.

This patch makes sure that we update timestamps on *all* character
devices that correspond to the given tty, because outside observers (w,
systemd-logind) are maybe checking these timestamps. Obviously, they can
not check timestamps on /dev/tty as that has per-process meaning.

[1] https://gitlab.com/procps-ng/procps/-/blob/v4.0.0/w.c#L286
[2] https://github.com/systemd/systemd/blob/v252/NEWS#L477

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---
 drivers/tty/tty_io.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 36fb945fdad4..48e0148b0f3e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -101,6 +101,7 @@
 #include <linux/compat.h>
 #include <linux/uaccess.h>
 #include <linux/termios_internal.h>
+#include <linux/fs.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/vt_kern.h>
@@ -811,18 +812,27 @@ void start_tty(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(start_tty);
 
-static void tty_update_time(struct timespec64 *time)
+static void tty_update_time(struct tty_struct *tty, int tstamp)
 {
+	struct tty_file_private *priv;
 	time64_t sec = ktime_get_real_seconds();
 
-	/*
-	 * We only care if the two values differ in anything other than the
-	 * lower three bits (i.e every 8 seconds).  If so, then we can update
-	 * the time of the tty device, otherwise it could be construded as a
-	 * security leak to let userspace know the exact timing of the tty.
-	 */
-	if ((sec ^ time->tv_sec) & ~7)
-		time->tv_sec = sec;
+	spin_lock(&tty->files_lock);
+	list_for_each_entry(priv, &tty->tty_files, list) {
+		struct file *filp = priv->file;
+		struct inode *inode = file_inode(filp);
+		struct timespec64 *time = tstamp == S_MTIME ? &inode->i_mtime : &inode->i_atime;
+
+		/*
+		 * We only care if the two values differ in anything other than the
+		 * lower three bits (i.e every 8 seconds).  If so, then we can update
+		 * the time of the tty device, otherwise it could be construded as a
+		 * security leak to let userspace know the exact timing of the tty.
+		 */
+		if ((sec ^ time->tv_sec) & ~7)
+			time->tv_sec = sec;
+	}
+	spin_unlock(&tty->files_lock);
 }
 
 /*
@@ -928,7 +938,7 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 	tty_ldisc_deref(ld);
 
 	if (i > 0)
-		tty_update_time(&inode->i_atime);
+		tty_update_time(tty, S_ATIME);
 
 	return i;
 }
@@ -1036,7 +1046,7 @@ static inline ssize_t do_tty_write(
 		cond_resched();
 	}
 	if (written) {
-		tty_update_time(&file_inode(file)->i_mtime);
+		tty_update_time(tty, S_MTIME);
 		ret = written;
 	}
 out:
-- 
2.39.2

[PATCH 2/2] selftests: tty: add selftest for tty timestamp updates

[Michal Sekletar]

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---
 tools/testing/selftests/Makefile              |  1 +
 tools/testing/selftests/tty/.gitignore        |  2 +
 tools/testing/selftests/tty/Makefile          |  5 ++
 .../testing/selftests/tty/tty_tstamp_update.c | 88 +++++++++++++++++++
 4 files changed, 96 insertions(+)
 create mode 100644 tools/testing/selftests/tty/.gitignore
 create mode 100644 tools/testing/selftests/tty/Makefile
 create mode 100644 tools/testing/selftests/tty/tty_tstamp_update.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 13a6837a0c6b..fc46926f505b 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -84,6 +84,7 @@ TARGETS += timers
 endif
 TARGETS += tmpfs
 TARGETS += tpm2
+TARGETS += tty
 TARGETS += user
 TARGETS += vDSO
 TARGETS += mm
diff --git a/tools/testing/selftests/tty/.gitignore b/tools/testing/selftests/tty/.gitignore
new file mode 100644
index 000000000000..fe70462a4aad
--- /dev/null
+++ b/tools/testing/selftests/tty/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+tty_tstamp_update
diff --git a/tools/testing/selftests/tty/Makefile b/tools/testing/selftests/tty/Makefile
new file mode 100644
index 000000000000..50d7027b2ae3
--- /dev/null
+++ b/tools/testing/selftests/tty/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS = -O2 -Wall
+TEST_GEN_PROGS := tty_tstamp_update
+
+include ../lib.mk
diff --git a/tools/testing/selftests/tty/tty_tstamp_update.c b/tools/testing/selftests/tty/tty_tstamp_update.c
new file mode 100644
index 000000000000..0ee97943dccc
--- /dev/null
+++ b/tools/testing/selftests/tty/tty_tstamp_update.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <linux/limits.h>
+
+#include "../kselftest.h"
+
+#define MIN_TTY_PATH_LEN 8
+
+static bool tty_valid(char *tty)
+{
+	if (strlen(tty) < MIN_TTY_PATH_LEN)
+		return false;
+
+	if (strncmp(tty, "/dev/tty", MIN_TTY_PATH_LEN) == 0 ||
+	    strncmp(tty, "/dev/pts", MIN_TTY_PATH_LEN) == 0)
+		return true;
+
+	return false;
+}
+
+static int write_dev_tty(void)
+{
+	FILE *f;
+	int r = 0;
+
+	f = fopen("/dev/tty", "r+");
+	if (!f)
+		return -errno;
+
+	r = fprintf(f, "hello, world!\n");
+	if (r != strlen("hello, world!\n"))
+		r = -EIO;
+
+	fclose(f);
+	return r;
+}
+
+int main(int argc, char **argv)
+{
+	int r;
+	char tty[PATH_MAX] = {};
+	struct stat st1, st2;
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	r = readlink("/proc/self/fd/0", tty, PATH_MAX);
+	if (r < 0)
+		ksft_exit_fail_msg("readlink on /proc/self/fd/0 failed: %m\n");
+
+	if (!tty_valid(tty))
+		ksft_exit_skip("invalid tty path '%s'\n", tty);
+
+	r = stat(tty, &st1);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We need to wait at least 8 seconds in order to observe timestamp change */
+	/* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbf47635315ab308c9b58a1ea0906e711a9228de */
+	sleep(10);
+
+	r = write_dev_tty();
+	if (r < 0)
+		ksft_exit_fail_msg("failed to write to /dev/tty: %s\n",
+				   strerror(-r));
+
+	r = stat(tty, &st2);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We wrote to the terminal so timestamps should have been updated */
+	if (st1.st_atim.tv_sec == st2.st_atim.tv_sec &&
+	    st1.st_mtim.tv_sec == st2.st_mtim.tv_sec) {
+		ksft_test_result_fail("tty timestamps not updated\n");
+		ksft_exit_fail();
+	}
+
+	ksft_test_result_pass(
+		"timestamps of terminal '%s' updated after write to /dev/tty\n", tty);
+	return EXIT_SUCCESS;
+}
-- 
2.39.2

Re: [PATCH 1/2] tty: tty_io: update timestamps on all device nodes

[Jiri Slaby]

On 03. 03. 23, 14:36, Michal Sekletar wrote:
> User space applications watch for timestamp changes on character device
> files in order to determine idle time of a given terminal session. For
> example, "w" program uses this information to populate the IDLE column
> of its output [1]. Similarly, systemd-logind has optional feature where
> it uses atime of the tty character device to determine if there was
> activity on the terminal associated with the logind's session object. If
> there was no activity for a configured period of time then logind will
> terminate such session [2].
> 
> Now, usually (e.g. bash running on the terminal) the use of the terminal
> will update timestamps (atime and mtime) on the corresponding terminal
> character device. However, if access to the terminal, e.g. /dev/pts/0,
> is performed through magic character device /dev/tty then such access
> obviously changes the state of the terminal, however timestamps on the
> device that correspond to the terminal (/dev/pts/0) are not updated.
> 
> This patch makes sure that we update timestamps on *all* character
> devices that correspond to the given tty, because outside observers (w,
> systemd-logind) are maybe checking these timestamps. Obviously, they can
> not check timestamps on /dev/tty as that has per-process meaning.
> 
> [1] https://gitlab.com/procps-ng/procps/-/blob/v4.0.0/w.c#L286
> [2] https://github.com/systemd/systemd/blob/v252/NEWS#L477
> 
> Signed-off-by: Michal Sekletar <msekleta@redhat.com>
> ---
>   drivers/tty/tty_io.c | 32 +++++++++++++++++++++-----------
>   1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 36fb945fdad4..48e0148b0f3e 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -101,6 +101,7 @@
>   #include <linux/compat.h>
>   #include <linux/uaccess.h>
>   #include <linux/termios_internal.h>
> +#include <linux/fs.h>
>   
>   #include <linux/kbd_kern.h>
>   #include <linux/vt_kern.h>
> @@ -811,18 +812,27 @@ void start_tty(struct tty_struct *tty)
>   }
>   EXPORT_SYMBOL(start_tty);
>   
> -static void tty_update_time(struct timespec64 *time)
> +static void tty_update_time(struct tty_struct *tty, int tstamp)

Why not enum file_time_flags then?

And "tstamp" sounds weird for what it is. It should be something like 
"time" or "time_flag". Or make it simply "bool mtime". And call it with 
true/false.

>   {
> +	struct tty_file_private *priv;
>   	time64_t sec = ktime_get_real_seconds();

Likely should be switched to have a reverse xmas tree.

>   
> -	/*
> -	 * We only care if the two values differ in anything other than the
> -	 * lower three bits (i.e every 8 seconds).  If so, then we can update
> -	 * the time of the tty device, otherwise it could be construded as a
> -	 * security leak to let userspace know the exact timing of the tty.
> -	 */
> -	if ((sec ^ time->tv_sec) & ~7)
> -		time->tv_sec = sec;
> +	spin_lock(&tty->files_lock);

Note: this should be fine wrt write lock. Have you tried running w/ 
lockdep enabled?

> +	list_for_each_entry(priv, &tty->tty_files, list) {
> +		struct file *filp = priv->file;

I think you can inline the above ^^ to the bellow vv.

> +		struct inode *inode = file_inode(filp);
> +		struct timespec64 *time = tstamp == S_MTIME ? &inode->i_mtime : &inode->i_atime;

So you'd have:
struct inode *inode = file_inode(priv->file);
struct timespec64 *time = mtime ? &inode->i_mtime : &inode->i_atime;

> +
> +		/*
> +		 * We only care if the two values differ in anything other than the
> +		 * lower three bits (i.e every 8 seconds).  If so, then we can update
> +		 * the time of the tty device, otherwise it could be construded as a
> +		 * security leak to let userspace know the exact timing of the tty.
> +		 */
> +		if ((sec ^ time->tv_sec) & ~7)
> +			time->tv_sec = sec;
> +	}
> +	spin_unlock(&tty->files_lock);
>   }
>   
>   /*
> @@ -928,7 +938,7 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
>   	tty_ldisc_deref(ld);
>   
>   	if (i > 0)
> -		tty_update_time(&inode->i_atime);
> +		tty_update_time(tty, S_ATIME);
>   
>   	return i;
>   }
> @@ -1036,7 +1046,7 @@ static inline ssize_t do_tty_write(
>   		cond_resched();
>   	}
>   	if (written) {
> -		tty_update_time(&file_inode(file)->i_mtime);
> +		tty_update_time(tty, S_MTIME);
>   		ret = written;
>   	}
>   out:

-- 
js
suse labs

[PATCH v2 1/2] tty: tty_io: update timestamps on all device nodes

[Michal Sekletar]

User space applications watch for timestamp changes on character device
files in order to determine idle time of a given terminal session. For
example, "w" program uses this information to populate the IDLE column
of its output [1]. Similarly, systemd-logind has optional feature where
it uses atime of the tty character device to determine if there was
activity on the terminal associated with the logind's session object. If
there was no activity for a configured period of time then logind will
terminate such session [2].

Now, usually (e.g. bash running on the terminal) the use of the terminal
will update timestamps (atime and mtime) on the corresponding terminal
character device. However, if access to the terminal, e.g. /dev/pts/0,
is performed through magic character device /dev/tty then such access
obviously changes the state of the terminal, however timestamps on the
device that correspond to the terminal (/dev/pts/0) are not updated.

This patch makes sure that we update timestamps on *all* character
devices that correspond to the given tty, because outside observers (w,
systemd-logind) are maybe checking these timestamps. Obviously, they can
not check timestamps on /dev/tty as that has per-process meaning.

[1] https://gitlab.com/procps-ng/procps/-/blob/v4.0.0/w.c#L286
[2] https://github.com/systemd/systemd/blob/v252/NEWS#L477

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---

v1 -> v2: Minor style tweaks based on code review comments

drivers/tty/tty_io.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c84be40fb..a505d2c49 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -101,6 +101,7 @@
 #include <linux/compat.h>
 #include <linux/uaccess.h>
 #include <linux/termios_internal.h>
+#include <linux/fs.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/vt_kern.h>
@@ -811,18 +812,26 @@ void start_tty(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(start_tty);
 
-static void tty_update_time(struct timespec64 *time)
+static void tty_update_time(struct tty_struct *tty, bool mtime)
 {
 	time64_t sec = ktime_get_real_seconds();
+	struct tty_file_private *priv;
 
-	/*
-	 * We only care if the two values differ in anything other than the
-	 * lower three bits (i.e every 8 seconds).  If so, then we can update
-	 * the time of the tty device, otherwise it could be construded as a
-	 * security leak to let userspace know the exact timing of the tty.
-	 */
-	if ((sec ^ time->tv_sec) & ~7)
-		time->tv_sec = sec;
+	spin_lock(&tty->files_lock);
+	list_for_each_entry(priv, &tty->tty_files, list) {
+		struct inode *inode = file_inode(priv->file);
+		struct timespec64 *time = mtime ? &inode->i_mtime : &inode->i_atime;
+
+		/*
+		 * We only care if the two values differ in anything other than the
+		 * lower three bits (i.e every 8 seconds).  If so, then we can update
+		 * the time of the tty device, otherwise it could be construded as a
+		 * security leak to let userspace know the exact timing of the tty.
+		 */
+		if ((sec ^ time->tv_sec) & ~7)
+			time->tv_sec = sec;
+	}
+	spin_unlock(&tty->files_lock);
 }
 
 /*
@@ -928,7 +937,7 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 	tty_ldisc_deref(ld);
 
 	if (i > 0)
-		tty_update_time(&inode->i_atime);
+		tty_update_time(tty, false);
 
 	return i;
 }
@@ -1036,7 +1045,7 @@ static inline ssize_t do_tty_write(
 		cond_resched();
 	}
 	if (written) {
-		tty_update_time(&file_inode(file)->i_mtime);
+		tty_update_time(tty, true);
 		ret = written;
 	}
 out:
-- 
2.39.1

[PATCH v2 2/2] selftests: tty: add selftest for tty timestamp updates

[Michal Sekletar]

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---
 tools/testing/selftests/Makefile              |  1 +
 tools/testing/selftests/tty/.gitignore        |  2 +
 tools/testing/selftests/tty/Makefile          |  5 ++
 .../testing/selftests/tty/tty_tstamp_update.c | 88 +++++++++++++++++++
 4 files changed, 96 insertions(+)
 create mode 100644 tools/testing/selftests/tty/.gitignore
 create mode 100644 tools/testing/selftests/tty/Makefile
 create mode 100644 tools/testing/selftests/tty/tty_tstamp_update.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 90a62cf75..862f5f9a7 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -86,6 +86,7 @@ TARGETS += timers
 endif
 TARGETS += tmpfs
 TARGETS += tpm2
+TARGETS += tty
 TARGETS += user
 TARGETS += vDSO
 TARGETS += mm
diff --git a/tools/testing/selftests/tty/.gitignore b/tools/testing/selftests/tty/.gitignore
new file mode 100644
index 000000000..fe70462a4
--- /dev/null
+++ b/tools/testing/selftests/tty/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+tty_tstamp_update
diff --git a/tools/testing/selftests/tty/Makefile b/tools/testing/selftests/tty/Makefile
new file mode 100644
index 000000000..50d7027b2
--- /dev/null
+++ b/tools/testing/selftests/tty/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS = -O2 -Wall
+TEST_GEN_PROGS := tty_tstamp_update
+
+include ../lib.mk
diff --git a/tools/testing/selftests/tty/tty_tstamp_update.c b/tools/testing/selftests/tty/tty_tstamp_update.c
new file mode 100644
index 000000000..0ee97943d
--- /dev/null
+++ b/tools/testing/selftests/tty/tty_tstamp_update.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <linux/limits.h>
+
+#include "../kselftest.h"
+
+#define MIN_TTY_PATH_LEN 8
+
+static bool tty_valid(char *tty)
+{
+	if (strlen(tty) < MIN_TTY_PATH_LEN)
+		return false;
+
+	if (strncmp(tty, "/dev/tty", MIN_TTY_PATH_LEN) == 0 ||
+	    strncmp(tty, "/dev/pts", MIN_TTY_PATH_LEN) == 0)
+		return true;
+
+	return false;
+}
+
+static int write_dev_tty(void)
+{
+	FILE *f;
+	int r = 0;
+
+	f = fopen("/dev/tty", "r+");
+	if (!f)
+		return -errno;
+
+	r = fprintf(f, "hello, world!\n");
+	if (r != strlen("hello, world!\n"))
+		r = -EIO;
+
+	fclose(f);
+	return r;
+}
+
+int main(int argc, char **argv)
+{
+	int r;
+	char tty[PATH_MAX] = {};
+	struct stat st1, st2;
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	r = readlink("/proc/self/fd/0", tty, PATH_MAX);
+	if (r < 0)
+		ksft_exit_fail_msg("readlink on /proc/self/fd/0 failed: %m\n");
+
+	if (!tty_valid(tty))
+		ksft_exit_skip("invalid tty path '%s'\n", tty);
+
+	r = stat(tty, &st1);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We need to wait at least 8 seconds in order to observe timestamp change */
+	/* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbf47635315ab308c9b58a1ea0906e711a9228de */
+	sleep(10);
+
+	r = write_dev_tty();
+	if (r < 0)
+		ksft_exit_fail_msg("failed to write to /dev/tty: %s\n",
+				   strerror(-r));
+
+	r = stat(tty, &st2);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We wrote to the terminal so timestamps should have been updated */
+	if (st1.st_atim.tv_sec == st2.st_atim.tv_sec &&
+	    st1.st_mtim.tv_sec == st2.st_mtim.tv_sec) {
+		ksft_test_result_fail("tty timestamps not updated\n");
+		ksft_exit_fail();
+	}
+
+	ksft_test_result_pass(
+		"timestamps of terminal '%s' updated after write to /dev/tty\n", tty);
+	return EXIT_SUCCESS;
+}
-- 
2.39.1

Re: [PATCH v2 2/2] selftests: tty: add selftest for tty timestamp updates

[Greg KH]

On Thu, Jun 08, 2023 at 12:16:16PM +0200, Michal Sekletar wrote:
> Signed-off-by: Michal Sekletar <msekleta@redhat.com>

Sorry, but for obvious reasons, I can't take patches without any
changelog text at all (and neither would you want me to.)

thanks,

greg k-h

Re: [PATCH v2 1/2] tty: tty_io: update timestamps on all device nodes

[Greg KH]

On Thu, Jun 08, 2023 at 12:16:15PM +0200, Michal Sekletar wrote:
> User space applications watch for timestamp changes on character device
> files in order to determine idle time of a given terminal session. For
> example, "w" program uses this information to populate the IDLE column
> of its output [1]. Similarly, systemd-logind has optional feature where
> it uses atime of the tty character device to determine if there was
> activity on the terminal associated with the logind's session object. If
> there was no activity for a configured period of time then logind will
> terminate such session [2].
> 
> Now, usually (e.g. bash running on the terminal) the use of the terminal
> will update timestamps (atime and mtime) on the corresponding terminal
> character device. However, if access to the terminal, e.g. /dev/pts/0,
> is performed through magic character device /dev/tty then such access
> obviously changes the state of the terminal, however timestamps on the
> device that correspond to the terminal (/dev/pts/0) are not updated.
> 
> This patch makes sure that we update timestamps on *all* character
> devices that correspond to the given tty, because outside observers (w,
> systemd-logind) are maybe checking these timestamps. Obviously, they can
> not check timestamps on /dev/tty as that has per-process meaning.

So how are you protecting this from being an information leak like we
have had in the past where you could monitor how many characters were
being sent to the tty through a proc file?  Seems like now you can just
monitor any tty node in the system and get the same information, while
today you can only do it for the tty devices you have permissions for,
right?

thanks,

greg k-h

Re: [PATCH v2 1/2] tty: tty_io: update timestamps on all device nodes

[Greg KH]

On Thu, Jun 08, 2023 at 07:52:54PM +0200, Michal Sekletar wrote:
> On Thu, Jun 8, 2023 at 1:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > So how are you protecting this from being an information leak like we
> > have had in the past where you could monitor how many characters were
> > being sent to the tty through a proc file?  Seems like now you can just
> > monitor any tty node in the system and get the same information, while
> > today you can only do it for the tty devices you have permissions for,
> > right?
> 
> Hi Greg,
> 
> I am not protecting against it in any way, but proposed changes are only
> about timestamp updates which still happen in at least 8 seconds intervals
> so exact timing of read/writes to tty can't be inferred. Frankly, I may
> have misunderstood something. It would be great if you could mention a bit
> more details about CVE you had in mind.

Ah, I missed that this is in 8 second increments, nevermind then!

thanks,

greg k-h

Re: [PATCH v2 1/2] tty: tty_io: update timestamps on all device nodes

[Greg KH]

On Tue, Jun 13, 2023 at 12:23:41PM +0200, Greg KH wrote:
> On Thu, Jun 08, 2023 at 07:52:54PM +0200, Michal Sekletar wrote:
> > On Thu, Jun 8, 2023 at 1:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > So how are you protecting this from being an information leak like we
> > > have had in the past where you could monitor how many characters were
> > > being sent to the tty through a proc file?  Seems like now you can just
> > > monitor any tty node in the system and get the same information, while
> > > today you can only do it for the tty devices you have permissions for,
> > > right?
> > 
> > Hi Greg,
> > 
> > I am not protecting against it in any way, but proposed changes are only
> > about timestamp updates which still happen in at least 8 seconds intervals
> > so exact timing of read/writes to tty can't be inferred. Frankly, I may
> > have misunderstood something. It would be great if you could mention a bit
> > more details about CVE you had in mind.
> 
> Ah, I missed that this is in 8 second increments, nevermind then!
> 

Note, I still can't take this series for the obvious reason in patch
2/2.  Please fix.

thanks,

greg k-h

[PATCH v3 1/2] tty: tty_io: update timestamps on all device nodes

[Michal Sekletar]

User space applications watch for timestamp changes on character device
files in order to determine idle time of a given terminal session. For
example, "w" program uses this information to populate the IDLE column
of its output [1]. Similarly, systemd-logind has optional feature where
it uses atime of the tty character device to determine if there was
activity on the terminal associated with the logind's session object. If
there was no activity for a configured period of time then logind will
terminate such session [2].

Now, usually (e.g. bash running on the terminal) the use of the terminal
will update timestamps (atime and mtime) on the corresponding terminal
character device. However, if access to the terminal, e.g. /dev/pts/0,
is performed through magic character device /dev/tty then such access
obviously changes the state of the terminal, however timestamps on the
device that correspond to the terminal (/dev/pts/0) are not updated.

This patch makes sure that we update timestamps on *all* character
devices that correspond to the given tty, because outside observers (w,
systemd-logind) are maybe checking these timestamps. Obviously, they can
not check timestamps on /dev/tty as that has per-process meaning.

[1] https://gitlab.com/procps-ng/procps/-/blob/v4.0.0/w.c#L286
[2] https://github.com/systemd/systemd/blob/v252/NEWS#L477

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---

v2 -> v3: Added more ellaborate commit message for newly introduced kselftest

 drivers/tty/tty_io.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c84be40fb8df..a505d2c49110 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -101,6 +101,7 @@
 #include <linux/compat.h>
 #include <linux/uaccess.h>
 #include <linux/termios_internal.h>
+#include <linux/fs.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/vt_kern.h>
@@ -811,18 +812,26 @@ void start_tty(struct tty_struct *tty)
 }
 EXPORT_SYMBOL(start_tty);
 
-static void tty_update_time(struct timespec64 *time)
+static void tty_update_time(struct tty_struct *tty, bool mtime)
 {
 	time64_t sec = ktime_get_real_seconds();
+	struct tty_file_private *priv;
 
-	/*
-	 * We only care if the two values differ in anything other than the
-	 * lower three bits (i.e every 8 seconds).  If so, then we can update
-	 * the time of the tty device, otherwise it could be construded as a
-	 * security leak to let userspace know the exact timing of the tty.
-	 */
-	if ((sec ^ time->tv_sec) & ~7)
-		time->tv_sec = sec;
+	spin_lock(&tty->files_lock);
+	list_for_each_entry(priv, &tty->tty_files, list) {
+		struct inode *inode = file_inode(priv->file);
+		struct timespec64 *time = mtime ? &inode->i_mtime : &inode->i_atime;
+
+		/*
+		 * We only care if the two values differ in anything other than the
+		 * lower three bits (i.e every 8 seconds).  If so, then we can update
+		 * the time of the tty device, otherwise it could be construded as a
+		 * security leak to let userspace know the exact timing of the tty.
+		 */
+		if ((sec ^ time->tv_sec) & ~7)
+			time->tv_sec = sec;
+	}
+	spin_unlock(&tty->files_lock);
 }
 
 /*
@@ -928,7 +937,7 @@ static ssize_t tty_read(struct kiocb *iocb, struct iov_iter *to)
 	tty_ldisc_deref(ld);
 
 	if (i > 0)
-		tty_update_time(&inode->i_atime);
+		tty_update_time(tty, false);
 
 	return i;
 }
@@ -1036,7 +1045,7 @@ static inline ssize_t do_tty_write(
 		cond_resched();
 	}
 	if (written) {
-		tty_update_time(&file_inode(file)->i_mtime);
+		tty_update_time(tty, true);
 		ret = written;
 	}
 out:
-- 
2.40.0

[PATCH v3 2/2] selftests: tty: add selftest for tty timestamp updates

[Michal Sekletar]

Add new test case which checks that timestamp updates on actual terminal
character device (e.g. /dev/pts/0) happen even if the terminal is
accessed via magic /dev/tty file.

Signed-off-by: Michal Sekletar <msekleta@redhat.com>
---
 tools/testing/selftests/Makefile              |  1 +
 tools/testing/selftests/tty/.gitignore        |  2 +
 tools/testing/selftests/tty/Makefile          |  5 ++
 .../testing/selftests/tty/tty_tstamp_update.c | 88 +++++++++++++++++++
 4 files changed, 96 insertions(+)
 create mode 100644 tools/testing/selftests/tty/.gitignore
 create mode 100644 tools/testing/selftests/tty/Makefile
 create mode 100644 tools/testing/selftests/tty/tty_tstamp_update.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 90a62cf75008..862f5f9a76a0 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -86,6 +86,7 @@ TARGETS += timers
 endif
 TARGETS += tmpfs
 TARGETS += tpm2
+TARGETS += tty
 TARGETS += user
 TARGETS += vDSO
 TARGETS += mm
diff --git a/tools/testing/selftests/tty/.gitignore b/tools/testing/selftests/tty/.gitignore
new file mode 100644
index 000000000000..fe70462a4aad
--- /dev/null
+++ b/tools/testing/selftests/tty/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+tty_tstamp_update
diff --git a/tools/testing/selftests/tty/Makefile b/tools/testing/selftests/tty/Makefile
new file mode 100644
index 000000000000..50d7027b2ae3
--- /dev/null
+++ b/tools/testing/selftests/tty/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS = -O2 -Wall
+TEST_GEN_PROGS := tty_tstamp_update
+
+include ../lib.mk
diff --git a/tools/testing/selftests/tty/tty_tstamp_update.c b/tools/testing/selftests/tty/tty_tstamp_update.c
new file mode 100644
index 000000000000..0ee97943dccc
--- /dev/null
+++ b/tools/testing/selftests/tty/tty_tstamp_update.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <linux/limits.h>
+
+#include "../kselftest.h"
+
+#define MIN_TTY_PATH_LEN 8
+
+static bool tty_valid(char *tty)
+{
+	if (strlen(tty) < MIN_TTY_PATH_LEN)
+		return false;
+
+	if (strncmp(tty, "/dev/tty", MIN_TTY_PATH_LEN) == 0 ||
+	    strncmp(tty, "/dev/pts", MIN_TTY_PATH_LEN) == 0)
+		return true;
+
+	return false;
+}
+
+static int write_dev_tty(void)
+{
+	FILE *f;
+	int r = 0;
+
+	f = fopen("/dev/tty", "r+");
+	if (!f)
+		return -errno;
+
+	r = fprintf(f, "hello, world!\n");
+	if (r != strlen("hello, world!\n"))
+		r = -EIO;
+
+	fclose(f);
+	return r;
+}
+
+int main(int argc, char **argv)
+{
+	int r;
+	char tty[PATH_MAX] = {};
+	struct stat st1, st2;
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	r = readlink("/proc/self/fd/0", tty, PATH_MAX);
+	if (r < 0)
+		ksft_exit_fail_msg("readlink on /proc/self/fd/0 failed: %m\n");
+
+	if (!tty_valid(tty))
+		ksft_exit_skip("invalid tty path '%s'\n", tty);
+
+	r = stat(tty, &st1);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We need to wait at least 8 seconds in order to observe timestamp change */
+	/* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbf47635315ab308c9b58a1ea0906e711a9228de */
+	sleep(10);
+
+	r = write_dev_tty();
+	if (r < 0)
+		ksft_exit_fail_msg("failed to write to /dev/tty: %s\n",
+				   strerror(-r));
+
+	r = stat(tty, &st2);
+	if (r < 0)
+		ksft_exit_fail_msg("stat failed on tty path '%s': %m\n", tty);
+
+	/* We wrote to the terminal so timestamps should have been updated */
+	if (st1.st_atim.tv_sec == st2.st_atim.tv_sec &&
+	    st1.st_mtim.tv_sec == st2.st_mtim.tv_sec) {
+		ksft_test_result_fail("tty timestamps not updated\n");
+		ksft_exit_fail();
+	}
+
+	ksft_test_result_pass(
+		"timestamps of terminal '%s' updated after write to /dev/tty\n", tty);
+	return EXIT_SUCCESS;
+}
-- 
2.40.0