Re: [3.14.y][3.16.y-ckt][3.18.y][3.19.y][PATCH 1/1] n_tty: Fix read buffer overwrite when no newline

[Joseph Salisbury <joseph.salisbury@canonical.com>]

On 04/15/2015 02:25 PM, Peter Hurley wrote:
> Hi Joseph,
>
> On 04/15/2015 01:39 PM, Joseph Salisbury wrote:
>> From: Peter Hurley <peter@hurleysoftware.com>
>>
>> BugLink: http://bugs.launchpad.net/bugs/1381005
>>
>> In canon mode, the read buffer head will advance over the buffer tail
>> if the input > 4095 bytes without receiving a line termination char.
>>
>> Discard additional input until a line termination is received.
>> Before evaluating for overflow, the 'room' value is normalized for
>> I_PARMRK and 1 byte is reserved for line termination (even in !icanon
>> mode, in case the mode is switched). The following table shows the
>> transform:
>>
>>  actual buffer |  'room' value before overflow calc
>>   space avail  |    !I_PARMRK    |    I_PARMRK
>>  --------------------------------------------------
>>       0        |       -1        |       -1
>>       1        |        0        |        0
>>       2        |        1        |        0
>>       3        |        2        |        0
>>       4+       |        3        |        1
>>
>> When !icanon or when icanon and the read buffer contains newlines,
>> normalized 'room' values of -1 and 0 are clamped to 0, and
>> 'overflow' is 0, so read_head is not adjusted and the input i/o loop
>> exits (setting no_room if called from flush_to_ldisc()). No input
>> is discarded since the reader does have input available to read
>> which ensures forward progress.
>>
>> When icanon and the read buffer does not contain newlines and the
>> normalized 'room' value is 0, then overflow and room are reset to 1,
>> so that the i/o loop will process the next input char normally
>> (except for parity errors which are ignored). Thus, erasures, signalling
>> chars, 7-bit mode, etc. will continue to be handled properly.
>>
>> If the input char processed was not a line termination char, then
>> the canon_head index will not have advanced, so the normalized 'room'
>> value will now be -1 and 'overflow' will be set, which indicates the
>> read_head can safely be reset, effectively erasing the last char
>> processed.
>>
>> If the input char processed was a line termination, then the
>> canon_head index will have advanced, so 'overflow' is cleared to 0,
>> the read_head is not reset, and 'room' is cleared to 0, which exits
>> the i/o loop (because the reader now have input available to read
>> which ensures forward progress).
>>
>> Note that it is possible for a line termination to be received, and
>> for the reader to copy the line to the user buffer before the
>> input i/o loop is ready to process the next input char. This is
>> why the i/o loop recomputes the room/overflow state with every
>> input char while handling overflow.
>>
>> Finally, if the input data was processed without receiving
>> a line termination (so that overflow is still set), the pty
>> driver must receive a write wakeup. A pty writer may be waiting
>> to write more data in n_tty_write() but without unthrottling
>> here that wakeup will not arrive, and forward progress will halt.
>> (Normally, the pty writer is woken when the reader reads data out
>> of the buffer and more space become available).
> Thanks for doing this!
> (I can now cross off the 1st item on my TODO list)
>
> comments below.
>
>> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> (backported from commit fb5ef9e7da39968fec6d6f37f20a23d23740c75e)
> Please note this is essentially also a backport of commit
> 06c49f9fa31f ("n_tty: Fix PARMRK over-throttling") as well, since
> it incorporates the results.
>
>> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
>> ---
>>  drivers/tty/n_tty.c | 108 +++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 81 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
>> index 4ddfa60..f190136 100644
>> --- a/drivers/tty/n_tty.c
>> +++ b/drivers/tty/n_tty.c
>> @@ -247,8 +247,8 @@ static void n_tty_write_wakeup(struct tty_struct *tty)
>>  
>>  static void n_tty_check_throttle(struct tty_struct *tty)
>>  {
>> -	if (tty->driver->type == TTY_DRIVER_TYPE_PTY)
>> -		return;
> Ok.
>
>> +	struct n_tty_data *ldata = tty->disc_data;
>> +
> Drop this. Nothing in n_tty_check_throttle() uses 'ldata' as a result
> of this commit.
>
>>  	/*
>>  	 * Check the remaining room for the input canonicalization
>>  	 * mode.  We don't want to throttle the driver if we're in
>> @@ -1512,23 +1512,6 @@ n_tty_receive_char_lnext(struct tty_struct *tty, unsigned char c, char flag)
>>  		n_tty_receive_char_flagged(tty, c, flag);
>>  }
>>  
>> -/**
>> - *	n_tty_receive_buf	-	data receive
>> - *	@tty: terminal device
>> - *	@cp: buffer
>> - *	@fp: flag buffer
>> - *	@count: characters
>> - *
>> - *	Called by the terminal driver when a block of characters has
>> - *	been received. This function must be called from soft contexts
>> - *	not from interrupt context. The driver is responsible for making
>> - *	calls one at a time and in order (or using flush_to_ldisc)
>> - *
>> - *	n_tty_receive_buf()/producer path:
>> - *		claims non-exclusive termios_rwsem
>> - *		publishes read_head and canon_head
>> - */
>> -
>>  static void
>>  n_tty_receive_buf_real_raw(struct tty_struct *tty, const unsigned char *cp,
>>  			   char *fp, int count)
>> @@ -1684,24 +1667,85 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
>>  	}
>>  }
>>  
>> +/**
>> + *	n_tty_receive_buf_common	-	process input
>> + *	@tty: device to receive input
>> + *	@cp: input chars
>> + *	@fp: flags for each char (if NULL, all chars are TTY_NORMAL)
>> + *	@count: number of input chars in @cp
>> + *
>> + *	Called by the terminal driver when a block of characters has
>> + *	been received. This function must be called from soft contexts
>> + *	not from interrupt context. The driver is responsible for making
>> + *	calls one at a time and in order (or using flush_to_ldisc)
>> + *
>> + *	Returns the # of input chars from @cp which were processed.
>> + *
>> + *	In canonical mode, the maximum line length is 4096 chars (including
>> + *	the line termination char); lines longer than 4096 chars are
>> + *	truncated. After 4095 chars, input data is still processed but
>> + *	not stored. Overflow processing ensures the tty can always
>> + *	receive more input until at least one line can be read.
>> + *
>> + *	In non-canonical mode, the read buffer will only accept 4095 chars;
>> + *	this provides the necessary space for a newline char if the input
>> + *	mode is switched to canonical.
>> + *
>> + *	Note it is possible for the read buffer to _contain_ 4096 chars
>> + *	in non-canonical mode: the read buffer could already contain the
>> + *	maximum canon line of 4096 chars when the mode is switched to
>> + *	non-canonical.
>> + *
>> + *	n_tty_receive_buf()/producer path:
>> + *		claims non-exclusive termios_rwsem
>> + *		publishes commit_head or canon_head
>> + */
>>  static int
>>  n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
>>  			 char *fp, int count, int flow)
>>  {
>>  	struct n_tty_data *ldata = tty->disc_data;
>> -	int room, n, rcvd = 0;
>> +	int room, n, rcvd = 0, overflow;
>>  
>>  	down_read(&tty->termios_rwsem);
>>  
>>  	while (1) {
>> -		room = receive_room(tty);
>> +		/*
>> +		 * When PARMRK is set, each input char may take up to 3 chars
>> +		 * in the read buf; reduce the buffer space avail by 3x
>> +		 *
>> +		 * If we are doing input canonicalization, and there are no
>> +		 * pending newlines, let characters through without limit, so
>> +		 * that erase characters will be handled.  Other excess
>> +		 * characters will be beeped.
>> +		 *
>> +		 * paired with store in *_copy_from_read_buf() -- guarantees
>> +		 * the consumer has loaded the data in read_buf up to the new
>> +		 * read_tail (so this producer will not overwrite unread data)
>> +		 */
>> +		size_t tail = smp_load_acquire(&ldata->read_tail);
> smp_load_acquire() is part of another fix not associated with this problem.
> This line should simply be
>
> 		size_t tail = ldata->read_tail;
>
> Then this fix can be applied across 3.12~3.19, inclusive.
>
> Thanks again.
>
> Regards,
> Peter Hurley
>
>> +
>> +		room = N_TTY_BUF_SIZE - (ldata->read_head - tail);
>> +		if (I_PARMRK(tty))
>> +			room = (room + 2) / 3;
>> +		room--;
>> +		if (room <= 0) {
>> +			overflow = ldata->icanon && ldata->canon_head == tail;
>> +			if (overflow && room < 0)
>> +				ldata->read_head--;
>> +			room = overflow;
>> +			ldata->no_room = flow && !room;
>> +		} else
>> +			overflow = 0;
>> +
>>  		n = min(count, room);
>> -		if (!n) {
>> -			if (flow && !room)
>> -				ldata->no_room = 1;
>> +		if (!n)
>>  			break;
>> -		}
>> -		__receive_buf(tty, cp, fp, n);
>> +
>> +		/* ignore parity errors if handling overflow */
>> +		if (!overflow || !fp || *fp != TTY_PARITY)
>> +			__receive_buf(tty, cp, fp, n);
>> +
>>  		cp += n;
>>  		if (fp)
>>  			fp += n;
>> @@ -1710,7 +1754,17 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
>>  	}
>>  
>>  	tty->receive_room = room;
>> -	n_tty_check_throttle(tty);
>> +
>> +	/* Unthrottle if handling overflow on pty */
>> +	if (tty->driver->type == TTY_DRIVER_TYPE_PTY) {
>> +		if (overflow) {
>> +			tty_set_flow_change(tty, TTY_UNTHROTTLE_SAFE);
>> +			tty_unthrottle_safe(tty);
>> +			__tty_set_flow_change(tty, 0);
>> +		}
>> +	} else
>> +		n_tty_check_throttle(tty);
>> +
>>  	up_read(&tty->termios_rwsem);
>>  
>>  	return rcvd;
>>
Thanks for the feedback, Peter.  I'll put together a V2 of this with all
your suggestions and resend.

Thanks,

Joe