linux-kernel - Re: Large pastes into readline enabled programs causes breakage from v2.6.31 onwards

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52120EAE.5060906@hurleysoftware.com>
Date:	Mon, 19 Aug 2013 08:25:18 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Maximiliano Curia <maxy@...ian.org>
CC:	Margarita Manterola <margamanterola@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>,
	Linux kernel <linux-kernel@...r.kernel.org>
Subject: Re: Large pastes into readline enabled programs causes breakage from
 v2.6.31 onwards

On 08/08/2013 01:58 PM, Maximiliano Curia wrote:
> Hi,
>
>> n_tty_set_room() in drivers/tty/n_tty.c (3.10 mainline)
>
>>  From n_tty_set_room():
>
>> 	/*
>> 	 * 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.
>> 	 */
>> 	if (left <= 0)
>> 		left = ldata->icanon && !ldata->canon_data;
>> 	old_left = tty->receive_room;
>> 	tty->receive_room = left;
>
> I took a long look at this code and thought about how it could be made to work
> for readline's case and also for the canonical readers.  I came up with this
> simple patch:
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 4bf0fc0..2ba7f4e 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -149,7 +149,8 @@ static int set_room(struct tty_struct *tty)
>           * characters will be beeped.
>           */
>          if (left <= 0)
> -               left = ldata->icanon && !ldata->canon_data;
> +               if (waitqueue_active(&tty->read_wait))
> +                       left = ldata->icanon && !ldata->canon_data;
>          old_left = tty->receive_room;
>          tty->receive_room = left;
>
> This is of course just an idea, but I tested this and it worked correctly for
> the cases I was testing.
>
> The effect of this patch is that when there is a canonical reader waiting for
> input, it maintains the previous behavior, but when there's no reader (like
> when readline is changing modes), it blocks and doesn't lose any characters.

Apologies for taking so long to reply.

My primary concern is canonical readers not become stuck with a full
read buffer, even with bogus input data (IOW, that an error condition will
not prevent a reader from making forward progress). I believe that won't
happen with this change, but what I really need in this case is a detailed
analysis from you of why that won't happen. That analysis should be in
the patch changelog. (Feel free to send me private mail if you need help
preparing a patch.)

And the patch above has a bug that allows a negative 'left' to be
assigned to tty->receive_room which will be interpreted as a very large
positive value.

This approach still has several drawbacks.

1) Since additional state is reset when the termios is changed by
readline(), the canonical line buffer state will be bogus.
This renders the termios change by readline() pointless; the
caller will not be able to retrieve expected input properly.

2) Since the input data is interpreted with the current termios when
data is received, any embedded control characters will not be
interpreted properly; again, the caller will not be able to retrieve
expected input properly.

> Another approach would be to recalculate the size of canon_data when the mode
> is changed, but this would probably be much more invasive, and awfully less
> efficient since it would imply going through the buffer.

This approach is not possible prior to linux-next since the input worker
thread and the reader thread are not locked out of access to the read buffer
while changing the termios.

And while rescanning the read buffer is possible in linux-next (eg, to
compute the read_flags bitmap indicating the position of NLs), this doesn't
address embedded control characters not being reinterpreted. And completely
reinterpreting the read buffer makes interpreting when receiving pointless.

> What do you think? Is the proposed solution, or something along those lines,
> acceptable?

I'm wondering if this problem might be best addressed on the paste side
instead of the read side. Although this wouldn't be a magic bullet, it
would be easier to control when more paste data is added.

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/