Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Tue, Apr 30, 2013 at 06:41:44PM -0700, Linus Torvalds wrote:
> On Tue, Apr 30, 2013 at 5:57 PM, Linus Torvalds
> wrote:
> >
> > Patch is whitespace-damaged and totally untested! Caveat applicator.
>
> Ok, so it's still whitespace-damaged, but it seems to work. The
> appended has the "8 second rule" too..
>
> Comments? Simon?
Tested -- both hunks seem to work as intended. Thanks!
Simon-
Below became b0b885657b6c8ef63a46bc9299b2a7715d19acde
>Linus
>
> --- snip snip ---
> drivers/tty/pty.c| 3 +++
> drivers/tty/tty_io.c | 4 ++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index a62798fcc014..59bfaecc4e14 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -681,6 +681,9 @@ static int ptmx_open(struct inode *inode, struct file
> *filp)
>
> nonseekable_open(inode, filp);
>
> + /* We refuse fsnotify events on ptmx, since it's a shared resource */
> + filp->f_mode |= FMODE_NONOTIFY;
> +
> retval = tty_alloc_file(filp);
> if (retval)
> return retval;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 97ebc8c5864e..6464029e4860 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -988,10 +988,10 @@ void start_tty(struct tty_struct *tty)
>
> EXPORT_SYMBOL(start_tty);
>
> +/* We limit tty time update visibility to every 8 seconds or so. */
> static void tty_update_time(struct timespec *time)
> {
> - unsigned long sec = get_seconds();
> - sec -= sec % 60;
> + unsigned long sec = get_seconds() & ~7;
> if ((long)(sec - time->tv_sec) > 0)
> time->tv_sec = sec;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
Hi,
> --- snip snip ---
> drivers/tty/pty.c| 3 +++
> drivers/tty/tty_io.c | 4 ++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index a62798fcc014..59bfaecc4e14 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -681,6 +681,9 @@ static int ptmx_open(struct inode *inode, struct file
> *filp)
>
> nonseekable_open(inode, filp);
>
> + /* We refuse fsnotify events on ptmx, since it's a shared resource */
> + filp->f_mode |= FMODE_NONOTIFY;
> +
> retval = tty_alloc_file(filp);
> if (retval)
> return retval;
This is definitely good. But of course you can still poll on mtime.
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 97ebc8c5864e..6464029e4860 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -988,10 +988,10 @@ void start_tty(struct tty_struct *tty)
>
> EXPORT_SYMBOL(start_tty);
>
> +/* We limit tty time update visibility to every 8 seconds or so. */
> static void tty_update_time(struct timespec *time)
> {
> - unsigned long sec = get_seconds();
> - sec -= sec % 60;
> + unsigned long sec = get_seconds() & ~7;
> if ((long)(sec - time->tv_sec) > 0)
> time->tv_sec = sec;
> }
I still find this mildly ugly. I would prefer this:
--- linux-3.8.10/drivers/tty/tty_io.c~ 2013-02-19 00:58:34.0 +0100
+++ linux-3.8.10/drivers/tty/tty_io.c 2013-05-01 13:46:16.0 +0200
@@ -1080,8 +1080,11 @@
cond_resched();
}
if (written) {
+ if (tty->driver->type != TTY_DRIVER_TYPE_PTY ||
+ tty->driver->subtype != PTY_TYPE_MASTER) {
struct inode *inode = file->f_path.dentry->d_inode;
inode->i_mtime = current_fs_time(inode->i_sb);
+ }
ret = written;
}
out:
(without the tty_update_time change). This prevents polling on
/dev/ptmx, but not on /dev/pts/*. The latter seems unnecessary to me,
because during password entry, echo mode is off so no bytes are read,
and canonical mode is ON so no bytes are written until NL is entered.
So you can only obtain the total time taken to enter the password, not
individual keystrokes.
The canonical mode is also the reason why my suggestion to fix this in
userspace (in the other subthread) is quite problematic (I tried to
change PAM) as it looks impossible to delay or obfuscate the write
events on /dev/ptmx.
Regards,
Wolfram.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On 05/01/2013 03:41 AM, Linus Torvalds wrote:
> On Tue, Apr 30, 2013 at 5:57 PM, Linus Torvalds
> wrote:
>>
>> Patch is whitespace-damaged and totally untested! Caveat applicator.
>
> Ok, so it's still whitespace-damaged, but it seems to work. The
> appended has the "8 second rule" too..
>
> Comments?
Yeah, looks good to me.
> --- snip snip ---
> drivers/tty/pty.c| 3 +++
> drivers/tty/tty_io.c | 4 ++--
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index a62798fcc014..59bfaecc4e14 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -681,6 +681,9 @@ static int ptmx_open(struct inode *inode, struct file
> *filp)
>
> nonseekable_open(inode, filp);
>
> + /* We refuse fsnotify events on ptmx, since it's a shared resource */
> + filp->f_mode |= FMODE_NONOTIFY;
> +
> retval = tty_alloc_file(filp);
> if (retval)
> return retval;
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 97ebc8c5864e..6464029e4860 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -988,10 +988,10 @@ void start_tty(struct tty_struct *tty)
>
> EXPORT_SYMBOL(start_tty);
>
> +/* We limit tty time update visibility to every 8 seconds or so. */
> static void tty_update_time(struct timespec *time)
> {
> - unsigned long sec = get_seconds();
> - sec -= sec % 60;
> + unsigned long sec = get_seconds() & ~7;
> if ((long)(sec - time->tv_sec) > 0)
> time->tv_sec = sec;
> }
>
thanks,
--
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Tue, Apr 30, 2013 at 5:57 PM, Linus Torvalds
wrote:
>
> Patch is whitespace-damaged and totally untested! Caveat applicator.
Ok, so it's still whitespace-damaged, but it seems to work. The
appended has the "8 second rule" too..
Comments? Simon?
Linus
--- snip snip ---
drivers/tty/pty.c| 3 +++
drivers/tty/tty_io.c | 4 ++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a62798fcc014..59bfaecc4e14 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -681,6 +681,9 @@ static int ptmx_open(struct inode *inode, struct file *filp)
nonseekable_open(inode, filp);
+ /* We refuse fsnotify events on ptmx, since it's a shared resource */
+ filp->f_mode |= FMODE_NONOTIFY;
+
retval = tty_alloc_file(filp);
if (retval)
return retval;
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 97ebc8c5864e..6464029e4860 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -988,10 +988,10 @@ void start_tty(struct tty_struct *tty)
EXPORT_SYMBOL(start_tty);
+/* We limit tty time update visibility to every 8 seconds or so. */
static void tty_update_time(struct timespec *time)
{
- unsigned long sec = get_seconds();
- sec -= sec % 60;
+ unsigned long sec = get_seconds() & ~7;
if ((long)(sec - time->tv_sec) > 0)
time->tv_sec = sec;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 6:37 PM, Greg Kroah-Hartman wrote: > > Ah, it's using inotify on the /dev/ptmx device. Jiri, your change > really doesn't affect that at all :( Hmm. Maybe something like the appended? Together with making the time modification be 10 seconds to make Simon happy (that's what Jiri originally did, it was me who said "why not make it a natural human timeframe"). In fact, maybe we should just make it a power-of-two (8? 4?) and avoid the nasty division.. Patch is whitespace-damaged and totally untested! Caveat applicator. Linus --- snip snip --- drivers/tty/pty.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index a62798fcc014..59bfaecc4e14 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -681,6 +681,9 @@ static int ptmx_open(struct inode *inode, struct file *filp) nonseekable_open(inode, filp); + /* We refuse fsnotify events on ptmx, since it's a shared resource */ + filp->f_mode |= FMODE_NONOTIFY; + retval = tty_alloc_file(filp); if (retval) return retval; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 06:37:24PM -0700, Greg Kroah-Hartman wrote: > On Mon, Apr 29, 2013 at 05:36:40PM -0700, Simon Kirby wrote: > > On Mon, Apr 29, 2013 at 05:21:17PM -0700, Greg Kroah-Hartman wrote: > > > > > On Mon, Apr 29, 2013 at 05:14:45PM -0700, Simon Kirby wrote: > > > > On Mon, Apr 29, 2013 at 12:01:44PM -0700, Greg Kroah-Hartman wrote: > > > > > > > > > 3.8-stable review patch. If anyone has any objections, please let me > > > > > know. > > > > > > > > I object. This breaks functionality I use every day (seeing who else is > > > > working on stuff with "w"). > > > > > > > > Furthermore, the patch does not actually fix the hole referenced (see > > > > ptmx-keystroke-latency.c on > > > > http://vladz.devzero.fr/013_ptmx-timing.php). > > > > I can still reproduce the timing capture even with this patch applied > > > > (in 3.9-rc8). > > > > > > How? There are no keystrokes being reported to other users, or did we > > > miss something with this patch? > > > > wget http://vladz.devzero.fr/svn/codes/PoC/ptmx-keystroke-latency.c > > gcc -O ptmx-keystroke-latency ptmx-keystroke-latency.c > > ./ptmx-keystroke-latency > > > > Log in to another tty, as another user. See keystroke timing. 3.9-rc8. > > > > Seems like it was missed. Meanwhile, idle times in "w" do not update. > > Ah, it's using inotify on the /dev/ptmx device. Jiri, your change > really doesn't affect that at all :( > > Simon, you mention a grsec change somewhere that addresses this issue. > Any hints on where that would be? Yes, see Jiri's comments in the original patch (b0de59b5733d): http://vladz.devzero.fr/013_ptmx-timing.php The grsec patch is linked from there: http://grsecurity.net/~spender/sidechannel.diff Simon- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 05:36:40PM -0700, Simon Kirby wrote: > On Mon, Apr 29, 2013 at 05:21:17PM -0700, Greg Kroah-Hartman wrote: > > > On Mon, Apr 29, 2013 at 05:14:45PM -0700, Simon Kirby wrote: > > > On Mon, Apr 29, 2013 at 12:01:44PM -0700, Greg Kroah-Hartman wrote: > > > > > > > 3.8-stable review patch. If anyone has any objections, please let me > > > > know. > > > > > > I object. This breaks functionality I use every day (seeing who else is > > > working on stuff with "w"). > > > > > > Furthermore, the patch does not actually fix the hole referenced (see > > > ptmx-keystroke-latency.c on http://vladz.devzero.fr/013_ptmx-timing.php). > > > I can still reproduce the timing capture even with this patch applied > > > (in 3.9-rc8). > > > > How? There are no keystrokes being reported to other users, or did we > > miss something with this patch? > > wget http://vladz.devzero.fr/svn/codes/PoC/ptmx-keystroke-latency.c > gcc -O ptmx-keystroke-latency ptmx-keystroke-latency.c > ./ptmx-keystroke-latency > > Log in to another tty, as another user. See keystroke timing. 3.9-rc8. > > Seems like it was missed. Meanwhile, idle times in "w" do not update. Ah, it's using inotify on the /dev/ptmx device. Jiri, your change really doesn't affect that at all :( Simon, you mention a grsec change somewhere that addresses this issue. Any hints on where that would be? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 05:21:17PM -0700, Greg Kroah-Hartman wrote: > On Mon, Apr 29, 2013 at 05:14:45PM -0700, Simon Kirby wrote: > > On Mon, Apr 29, 2013 at 12:01:44PM -0700, Greg Kroah-Hartman wrote: > > > > > 3.8-stable review patch. If anyone has any objections, please let me > > > know. > > > > I object. This breaks functionality I use every day (seeing who else is > > working on stuff with "w"). > > > > Furthermore, the patch does not actually fix the hole referenced (see > > ptmx-keystroke-latency.c on http://vladz.devzero.fr/013_ptmx-timing.php). > > I can still reproduce the timing capture even with this patch applied > > (in 3.9-rc8). > > How? There are no keystrokes being reported to other users, or did we > miss something with this patch? wget http://vladz.devzero.fr/svn/codes/PoC/ptmx-keystroke-latency.c gcc -O ptmx-keystroke-latency ptmx-keystroke-latency.c ./ptmx-keystroke-latency Log in to another tty, as another user. See keystroke timing. 3.9-rc8. Seems like it was missed. Meanwhile, idle times in "w" do not update. > > The grsec patch instead introdues another test within the inotify code > > (is_sidechannel_device()-related bits) -- untested by me, but probably > > more relevant. > > > > Even 37b7f3c76595e23257f61bd80b223de8658617ee, the "regression fix", > > which Linus merged in for the 3.9 release, is still a regression for me. > > And I applied that one as well. Right, so this restores updates but increases the granularity to 60 seconds. I'm complaining that this is still affects my occupational performance. > > 60 seconds means somebody is asleep in my environment, and so is still > > the kind of thing that just pisses me off. I'd rather revert this whole > > thing. > > Users taking a break for longer than a minute upset you? What are you > really trying to keep track of here? Really? In a team environment, a person idle for 30 seconds means they've stopped to look at something else. Now we have to wait 2 minutes to know if this has happened or not. Now it becomes faster to interrupt somebody to ask them if maintenance can be done, etc. > > I'd stand maybe 1 seconds as maximum granularity. You could do that with > > less code and no test. > > Patch to show this? I was thinking of just updating the seconds field of the timespec struct, or leaving this particular part and setting sb->s_time_gran to 1, though that would probably break other things. Since I've never looked at this stuff before, I'm not sure I should make a patch, but I can... Simon- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 12:01:44PM -0700, Greg Kroah-Hartman wrote:
> 3.8-stable review patch. If anyone has any objections, please let me know.
I object. This breaks functionality I use every day (seeing who else is
working on stuff with "w").
Furthermore, the patch does not actually fix the hole referenced (see
ptmx-keystroke-latency.c on http://vladz.devzero.fr/013_ptmx-timing.php).
I can still reproduce the timing capture even with this patch applied
(in 3.9-rc8).
The grsec patch instead introdues another test within the inotify code
(is_sidechannel_device()-related bits) -- untested by me, but probably
more relevant.
Even 37b7f3c76595e23257f61bd80b223de8658617ee, the "regression fix",
which Linus merged in for the 3.9 release, is still a regression for me.
60 seconds means somebody is asleep in my environment, and so is still
the kind of thing that just pisses me off. I'd rather revert this whole
thing.
I'd stand maybe 1 seconds as maximum granularity. You could do that with
less code and no test.
"watch -n.1 ls --full-time /dev/pts/1" shows that the exposed resolution
(without inotify) is to the nanosecond.
Simon-
> --
>
> From: Jiri Slaby
>
> commit b0de59b5733d18b0d1974a060860a8b5c1b36a2e upstream.
>
> On http://vladz.devzero.fr/013_ptmx-timing.php, we can see how to find
> out length of a password using timestamps of /dev/ptmx. It is
> documented in "Timing Analysis of Keystrokes and Timing Attacks on
> SSH". To avoid that problem, do not update time when reading
> from/writing to a TTY.
>
> I am afraid of regressions as this is a behavior we have since 0.97
> and apps may expect the time to be current, e.g. for monitoring
> whether there was a change on the TTY. Now, there is no change. So
> this would better have a lot of testing before it goes upstream.
>
> References: CVE-2013-0160
>
> Signed-off-by: Jiri Slaby
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/tty/tty_io.c |8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -977,8 +977,7 @@ static ssize_t tty_read(struct file *fil
> else
> i = -EIO;
> tty_ldisc_deref(ld);
> - if (i > 0)
> - inode->i_atime = current_fs_time(inode->i_sb);
> +
> return i;
> }
>
> @@ -1079,11 +1078,8 @@ static inline ssize_t do_tty_write(
> break;
> cond_resched();
> }
> - if (written) {
> - struct inode *inode = file->f_path.dentry->d_inode;
> - inode->i_mtime = current_fs_time(inode->i_sb);
> + if (written)
> ret = written;
> - }
> out:
> tty_write_unlock(tty);
> return ret;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ 02/42] TTY: do not update atime/mtime on read/write
On Mon, Apr 29, 2013 at 05:14:45PM -0700, Simon Kirby wrote: > On Mon, Apr 29, 2013 at 12:01:44PM -0700, Greg Kroah-Hartman wrote: > > > 3.8-stable review patch. If anyone has any objections, please let me know. > > I object. This breaks functionality I use every day (seeing who else is > working on stuff with "w"). > > Furthermore, the patch does not actually fix the hole referenced (see > ptmx-keystroke-latency.c on http://vladz.devzero.fr/013_ptmx-timing.php). > I can still reproduce the timing capture even with this patch applied > (in 3.9-rc8). How? There are no keystrokes being reported to other users, or did we miss something with this patch? > The grsec patch instead introdues another test within the inotify code > (is_sidechannel_device()-related bits) -- untested by me, but probably > more relevant. > > Even 37b7f3c76595e23257f61bd80b223de8658617ee, the "regression fix", > which Linus merged in for the 3.9 release, is still a regression for me. And I applied that one as well. > 60 seconds means somebody is asleep in my environment, and so is still > the kind of thing that just pisses me off. I'd rather revert this whole > thing. Users taking a break for longer than a minute upset you? What are you really trying to keep track of here? > I'd stand maybe 1 seconds as maximum granularity. You could do that with > less code and no test. Patch to show this? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
