[ACK][Trusty][pull request] (upstream) n_tty: Fix buffer overruns with larger-than-4k pastes

[Chris J Arges]

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 at 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 at users.sourceforge.net>
>>
>> OriginalAuthor: Peter Hurley <peter at hurleysoftware.com>
>> Reported-by: Margarita Manterola <margamanterola at gmail.com>
>> Acked-by: Stas Sergeev <stsp at users.sourceforge.net>
>> Signed-off-by: Peter Hurley <peter at hurleysoftware.com>
>> (cherry-picked from commit 4d0ed18277cc6f07513ee0b04475f19cd69e75ef v3.14-rc1)
>> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> Signed-off-by: Rafael David Tinoco <rafael.tinoco at 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 at 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 at canonical.com>

> -apw
>