| Message ID | 73C05D00-F03A-4279-8587-3177B11E39FD@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 24, 2014 at 05:43:29PM -0300, Rafael David Tinoco wrote: > From 0b8b060c8932c600a240fb1fa055fa103b5e969a Mon Sep 17 00:00:00 2001 > From: Peter Hurley <peter@hurleysoftware.com> > Date: Tue, 10 Dec 2013 17:12:02 -0500 > Subject: n_tty: Fix buffer overruns with larger-than-4k pastes > > BugLink: http://bugs.launchpad.net/bugs/1208740 > > n_tty: Fix buffer overruns with larger-than-4k pastes > > readline() inadvertently triggers an error recovery path when > pastes larger than 4k overrun the line discipline buffer. The > error recovery path discards input when the line discipline buffer > is full and operating in canonical mode and no newline has been > received. Because readline() changes the termios to non-canonical > mode to read the line char-by-char, the line discipline buffer > can become full, and then when readline() restores termios back > to canonical mode for the caller, the now-full line discipline > buffer triggers the error recovery. > > When changing termios from non-canon to canon mode and the read > buffer contains data, simulate an EOF push _without_ the > DISABLED_CHAR in the read buffer. > > Importantly for the readline() problem, the termios can be > changed back to non-canonical mode without changes to the read > buffer occurring; ie., as if the previous termios change had not > happened (as long as no intervening read took place). > > Preserve existing userspace behavior which allows '\0's already > received in non-canon mode to be read as '\0's in canon mode > (rather than trigger add'l EOF pushes or an actual EOF). > > Patch based on original proposal and discussion here > https://bugzilla.kernel.org/show_bug.cgi?id=55991 > by Stas Sergeev <stsp@users.sourceforge.net> > > OriginalAuthor: Peter Hurley <peter@hurleysoftware.com> > Reported-by: Margarita Manterola <margamanterola@gmail.com> > Acked-by: Stas Sergeev <stsp@users.sourceforge.net> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > (cherry-picked from commit 4d0ed18277cc6f07513ee0b04475f19cd69e75ef v3.14-rc1) > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com> > --- > drivers/tty/n_tty.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 86093e2..f591184 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -105,6 +105,7 @@ struct n_tty_data { > > /* must hold exclusive termios_rwsem to reset these */ > unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1; > + unsigned char push:1; > > /* shared by producer and consumer */ > char read_buf[N_TTY_BUF_SIZE]; > @@ -342,6 +343,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata) > > ldata->erasing = 0; > bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); > + ldata->push = 0; > } > > static void n_tty_packet_mode_flush(struct tty_struct *tty) > @@ -1762,7 +1764,16 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old) > > if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) { > bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); > - ldata->line_start = ldata->canon_head = ldata->read_tail; > + ldata->line_start = ldata->read_tail; > + if (!L_ICANON(tty) || !read_cnt(ldata)) { > + ldata->canon_head = ldata->read_tail; > + ldata->push = 0; > + } else { > + set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1), > + ldata->read_flags); > + ldata->canon_head = ldata->read_head; > + ldata->push = 1; > + } > ldata->erasing = 0; > ldata->lnext = 0; > } > @@ -1967,6 +1978,12 @@ static int copy_from_read_buf(struct tty_struct *tty, > * it copies one line of input up to and including the line-delimiting > * character into the user-space buffer. > * > + * NB: When termios is changed from non-canonical to canonical mode and > + * the read buffer contains data, n_tty_set_termios() simulates an EOF > + * push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer. > + * This causes data already processed as input to be immediately available > + * as input although a newline has not been received. > + * > * Called under the atomic_read_lock mutex > * > * n_tty_read()/consumer path: > @@ -2013,7 +2030,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, > n += found; > c = n; > > - if (found && read_buf(ldata, eol) == __DISABLED_CHAR) { > + if (found && !ldata->push && read_buf(ldata, eol) == __DISABLED_CHAR) { > n--; > eof_push = !n && ldata->read_tail != ldata->line_start; > } > @@ -2040,7 +2057,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, > ldata->read_tail += c; > > if (found) { > - ldata->line_start = ldata->read_tail; > + if (!ldata->push) > + ldata->line_start = ldata->read_tail; > + else > + ldata->push = 0; > tty_audit_push(tty); > } > return eof_push ? -EAGAIN : 0; > -- Looks reasonable, I would like Chris Arges to look over this as he was involved in the last batch of tty fixes for "pasting wholy unreasonable amounts of data does not work" and we have some local fixes applied at least in Saucy for this. But assming he is happy: Acked-by: Andy Whitcroft <apw@canonical.com> -apw
On 07/25/2014 04:54 AM, Andy Whitcroft wrote: > On Thu, Jul 24, 2014 at 05:43:29PM -0300, Rafael David Tinoco wrote: >> From 0b8b060c8932c600a240fb1fa055fa103b5e969a Mon Sep 17 00:00:00 2001 >> From: Peter Hurley <peter@hurleysoftware.com> >> Date: Tue, 10 Dec 2013 17:12:02 -0500 >> Subject: n_tty: Fix buffer overruns with larger-than-4k pastes >> >> BugLink: http://bugs.launchpad.net/bugs/1208740 >> >> n_tty: Fix buffer overruns with larger-than-4k pastes >> >> readline() inadvertently triggers an error recovery path when >> pastes larger than 4k overrun the line discipline buffer. The >> error recovery path discards input when the line discipline buffer >> is full and operating in canonical mode and no newline has been >> received. Because readline() changes the termios to non-canonical >> mode to read the line char-by-char, the line discipline buffer >> can become full, and then when readline() restores termios back >> to canonical mode for the caller, the now-full line discipline >> buffer triggers the error recovery. >> >> When changing termios from non-canon to canon mode and the read >> buffer contains data, simulate an EOF push _without_ the >> DISABLED_CHAR in the read buffer. >> >> Importantly for the readline() problem, the termios can be >> changed back to non-canonical mode without changes to the read >> buffer occurring; ie., as if the previous termios change had not >> happened (as long as no intervening read took place). >> >> Preserve existing userspace behavior which allows '\0's already >> received in non-canon mode to be read as '\0's in canon mode >> (rather than trigger add'l EOF pushes or an actual EOF). >> >> Patch based on original proposal and discussion here >> https://bugzilla.kernel.org/show_bug.cgi?id=55991 >> by Stas Sergeev <stsp@users.sourceforge.net> >> >> OriginalAuthor: Peter Hurley <peter@hurleysoftware.com> >> Reported-by: Margarita Manterola <margamanterola@gmail.com> >> Acked-by: Stas Sergeev <stsp@users.sourceforge.net> >> Signed-off-by: Peter Hurley <peter@hurleysoftware.com> >> (cherry-picked from commit 4d0ed18277cc6f07513ee0b04475f19cd69e75ef v3.14-rc1) >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com> >> --- >> drivers/tty/n_tty.c | 26 +++++++++++++++++++++++--- >> 1 file changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c >> index 86093e2..f591184 100644 >> --- a/drivers/tty/n_tty.c >> +++ b/drivers/tty/n_tty.c >> @@ -105,6 +105,7 @@ struct n_tty_data { >> >> /* must hold exclusive termios_rwsem to reset these */ >> unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1; >> + unsigned char push:1; >> >> /* shared by producer and consumer */ >> char read_buf[N_TTY_BUF_SIZE]; >> @@ -342,6 +343,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata) >> >> ldata->erasing = 0; >> bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); >> + ldata->push = 0; >> } >> >> static void n_tty_packet_mode_flush(struct tty_struct *tty) >> @@ -1762,7 +1764,16 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old) >> >> if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) { >> bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); >> - ldata->line_start = ldata->canon_head = ldata->read_tail; >> + ldata->line_start = ldata->read_tail; >> + if (!L_ICANON(tty) || !read_cnt(ldata)) { >> + ldata->canon_head = ldata->read_tail; >> + ldata->push = 0; >> + } else { >> + set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1), >> + ldata->read_flags); >> + ldata->canon_head = ldata->read_head; >> + ldata->push = 1; >> + } >> ldata->erasing = 0; >> ldata->lnext = 0; >> } >> @@ -1967,6 +1978,12 @@ static int copy_from_read_buf(struct tty_struct *tty, >> * it copies one line of input up to and including the line-delimiting >> * character into the user-space buffer. >> * >> + * NB: When termios is changed from non-canonical to canonical mode and >> + * the read buffer contains data, n_tty_set_termios() simulates an EOF >> + * push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer. >> + * This causes data already processed as input to be immediately available >> + * as input although a newline has not been received. >> + * >> * Called under the atomic_read_lock mutex >> * >> * n_tty_read()/consumer path: >> @@ -2013,7 +2030,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, >> n += found; >> c = n; >> >> - if (found && read_buf(ldata, eol) == __DISABLED_CHAR) { >> + if (found && !ldata->push && read_buf(ldata, eol) == __DISABLED_CHAR) { >> n--; >> eof_push = !n && ldata->read_tail != ldata->line_start; >> } >> @@ -2040,7 +2057,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, >> ldata->read_tail += c; >> >> if (found) { >> - ldata->line_start = ldata->read_tail; >> + if (!ldata->push) >> + ldata->line_start = ldata->read_tail; >> + else >> + ldata->push = 0; >> tty_audit_push(tty); >> } >> return eof_push ? -EAGAIN : 0; >> -- > > Looks reasonable, I would like Chris Arges to look over this as he was > involved in the last batch of tty fixes for "pasting wholy unreasonable > amounts of data does not work" and we have some local fixes applied at > least in Saucy for this. But assming he is happy: > > Acked-by: Andy Whitcroft <apw@canonical.com> > I tested with and without this patch and it clearly fixes the issue. FYI, the previous patch "SAUCE: (no-up) Only let characters through when there are active readers." was the preliminary version in < 3.13 versions that fixes this issue. But now that the upstream patch was accepted, we should use this version of this patch as posted. Acked-by: Chris J Arges <chris.j.arges@canonical.com> > -apw >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 07/24/2014 01:43 PM, Rafael David Tinoco wrote: > From 0b8b060c8932c600a240fb1fa055fa103b5e969a Mon Sep 17 00:00:00 2001 > From: Peter Hurley <peter@hurleysoftware.com> > Date: Tue, 10 Dec 2013 17:12:02 -0500 > Subject: n_tty: Fix buffer overruns with larger-than-4k pastes > > BugLink: http://bugs.launchpad.net/bugs/1208740 > > n_tty: Fix buffer overruns with larger-than-4k pastes > > readline() inadvertently triggers an error recovery path when > pastes larger than 4k overrun the line discipline buffer. The > error recovery path discards input when the line discipline buffer > is full and operating in canonical mode and no newline has been > received. Because readline() changes the termios to non-canonical > mode to read the line char-by-char, the line discipline buffer > can become full, and then when readline() restores termios back > to canonical mode for the caller, the now-full line discipline > buffer triggers the error recovery. > > When changing termios from non-canon to canon mode and the read > buffer contains data, simulate an EOF push _without_ the > DISABLED_CHAR in the read buffer. > > Importantly for the readline() problem, the termios can be > changed back to non-canonical mode without changes to the read > buffer occurring; ie., as if the previous termios change had not > happened (as long as no intervening read took place). > > Preserve existing userspace behavior which allows '\0's already > received in non-canon mode to be read as '\0's in canon mode > (rather than trigger add'l EOF pushes or an actual EOF). > > Patch based on original proposal and discussion here > https://bugzilla.kernel.org/show_bug.cgi?id=55991 > by Stas Sergeev <stsp@users.sourceforge.net> > > OriginalAuthor: Peter Hurley <peter@hurleysoftware.com> > Reported-by: Margarita Manterola <margamanterola@gmail.com> > Acked-by: Stas Sergeev <stsp@users.sourceforge.net> > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > (cherry-picked from commit 4d0ed18277cc6f07513ee0b04475f19cd69e75ef v3.14-rc1) > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Rafael David Tinoco <rafael.tinoco@canonical.com> > --- > drivers/tty/n_tty.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 86093e2..f591184 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -105,6 +105,7 @@ struct n_tty_data { > > /* must hold exclusive termios_rwsem to reset these */ > unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1; > + unsigned char push:1; > > /* shared by producer and consumer */ > char read_buf[N_TTY_BUF_SIZE]; > @@ -342,6 +343,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata) > > ldata->erasing = 0; > bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); > + ldata->push = 0; > } > > static void n_tty_packet_mode_flush(struct tty_struct *tty) > @@ -1762,7 +1764,16 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old) > > if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) { > bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); > - ldata->line_start = ldata->canon_head = ldata->read_tail; > + ldata->line_start = ldata->read_tail; > + if (!L_ICANON(tty) || !read_cnt(ldata)) { > + ldata->canon_head = ldata->read_tail; > + ldata->push = 0; > + } else { > + set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1), > + ldata->read_flags); > + ldata->canon_head = ldata->read_head; > + ldata->push = 1; > + } > ldata->erasing = 0; > ldata->lnext = 0; > } > @@ -1967,6 +1978,12 @@ static int copy_from_read_buf(struct tty_struct *tty, > * it copies one line of input up to and including the line-delimiting > * character into the user-space buffer. > * > + * NB: When termios is changed from non-canonical to canonical mode and > + * the read buffer contains data, n_tty_set_termios() simulates an EOF > + * push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer. > + * This causes data already processed as input to be immediately available > + * as input although a newline has not been received. > + * > * Called under the atomic_read_lock mutex > * > * n_tty_read()/consumer path: > @@ -2013,7 +2030,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, > n += found; > c = n; > > - if (found && read_buf(ldata, eol) == __DISABLED_CHAR) { > + if (found && !ldata->push && read_buf(ldata, eol) == __DISABLED_CHAR) { > n--; > eof_push = !n && ldata->read_tail != ldata->line_start; > } > @@ -2040,7 +2057,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, > ldata->read_tail += c; > > if (found) { > - ldata->line_start = ldata->read_tail; > + if (!ldata->push) > + ldata->line_start = ldata->read_tail; > + else > + ldata->push = 0; > tty_audit_push(tty); > } > return eof_push ? -EAGAIN : 0; > > > - -- Brad Figg brad.figg@canonical.com http://www.canonical.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJT0m2sAAoJEAx7WJsQW+f3TxYQAIZVxLQsVeBuDIsjRWgzSoLt WYGIjgC7erWhbziJ54jwZUEGqGoUBIwYMlMV0M7ty0B/J0tbH9VaACkM/MjOwoYZ AhKgZot4GarmZTlelb8hAoq0nNIa25L2P0fKzHU3Rfd2DWJ/ck7el1+YMPc+xxgS Zq6tCTvCq3R51zUxXc4SFe/nNPH/6ljEF/OKRQeuZE89YarOkhT9c8BDCxnxkoKu 5bMCjGZ55vt85N72Hr2C21qufeKGIP8n3s46hQmLe5KVNmgmC9Zh7HK61e3HTUge QkP8LphlaIxWqZYxUsy5Eul1++xqD0r3Y1Lti+U3e70x5YG8TlUsNV5X2dzV00xW nRIlD6gu1d1hPY9IJctseYKqpjVmlP9bhFnuKZsPnsqIFOO1rMO5SELc2PTg7iCH LWF29Ldin1Jz2pyJSCGRaGqaHeKtym4Sb13L1RhfJ64UStvxAJVY8wmd5/ch73+I ZMYCzKfNRFQFTwUT4BBqOcD6ugvhqSYD3ymBm+hIXg/xu2KAh+Vc5KkgLUtfFx8e kf5KKzklVcqDi0bAi19xZ3xzZeghWA9yK3u21yqw1Zg/mWT9P3vYcEC68BtMPipg hqcNACpkebIuqS9MMyVCgSvP0337PxsBk7dAmZpLM/w8aexeKRPk0achm73qO/gV zQjHQfZ8jtNsGCTNnwmH =Dfqh -----END PGP SIGNATURE-----
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 86093e2..f591184 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -105,6 +105,7 @@ struct n_tty_data { /* must hold exclusive termios_rwsem to reset these */ unsigned char lnext:1, erasing:1, raw:1, real_raw:1, icanon:1; + unsigned char push:1; /* shared by producer and consumer */ char read_buf[N_TTY_BUF_SIZE]; @@ -342,6 +343,7 @@ static void reset_buffer_flags(struct n_tty_data *ldata) ldata->erasing = 0; bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); + ldata->push = 0; } static void n_tty_packet_mode_flush(struct tty_struct *tty) @@ -1762,7 +1764,16 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old) if (!old || (old->c_lflag ^ tty->termios.c_lflag) & ICANON) { bitmap_zero(ldata->read_flags, N_TTY_BUF_SIZE); - ldata->line_start = ldata->canon_head = ldata->read_tail; + ldata->line_start = ldata->read_tail; + if (!L_ICANON(tty) || !read_cnt(ldata)) { + ldata->canon_head = ldata->read_tail; + ldata->push = 0; + } else { + set_bit((ldata->read_head - 1) & (N_TTY_BUF_SIZE - 1), + ldata->read_flags); + ldata->canon_head = ldata->read_head; + ldata->push = 1; + } ldata->erasing = 0; ldata->lnext = 0; } @@ -1967,6 +1978,12 @@ static int copy_from_read_buf(struct tty_struct *tty, * it copies one line of input up to and including the line-delimiting * character into the user-space buffer. * + * NB: When termios is changed from non-canonical to canonical mode and + * the read buffer contains data, n_tty_set_termios() simulates an EOF + * push (as if C-d were input) _without_ the DISABLED_CHAR in the buffer. + * This causes data already processed as input to be immediately available + * as input although a newline has not been received. + * * Called under the atomic_read_lock mutex * * n_tty_read()/consumer path: @@ -2013,7 +2030,7 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, n += found; c = n; - if (found && read_buf(ldata, eol) == __DISABLED_CHAR) { + if (found && !ldata->push && read_buf(ldata, eol) == __DISABLED_CHAR) { n--; eof_push = !n && ldata->read_tail != ldata->line_start; } @@ -2040,7 +2057,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty, ldata->read_tail += c; if (found) { - ldata->line_start = ldata->read_tail; + if (!ldata->push) + ldata->line_start = ldata->read_tail; + else + ldata->push = 0; tty_audit_push(tty); } return eof_push ? -EAGAIN : 0;