| 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
| ||
|
Message-ID: <4AD3F769.5080405@gmail.com>
Date: Mon, 12 Oct 2009 20:43:37 -0700
From: "Justin P. Mattock" <justinmattock@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
CC: Nix <nix@...eri.org.uk>, Alan Cox <alan@...rguk.ukuu.org.uk>,
Paul Fulghum <paulkf@...rogate.com>,
"Rafael J. Wysocki" <rjw@...k.pl>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Kernel Testers List <kernel-testers@...r.kernel.org>,
Boyan <btanastasov@...oo.co.uk>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Ed Tomlinson <edt@....ca>,
"Frédéric L. W. Meunier"
<fredlwm@...il.com>, OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Subject: Re: [Bug #14388] keyboard under X with 2.6.31
Linus Torvalds wrote:
> [ Alan, Paulkf - the tty buffering and locking is originally your code,
> although from about three years ago, when it used to be in tty_io.c..
> Any comment? ]
>
> On Mon, 12 Oct 2009, Linus Torvalds wrote:
>
>> Alan, Ogawa-san, do either of you see some problem in tty_buffer.c,
>> perhaps?
>>
>
> Hmm. I see one, at least.
>
> The "tty_insert_flip_string()" locking seems totally bogus.
>
> It does that "tty_buffer_request_room()" call and subsequent copying with
> no locking at all - sure, the tty_buffer_request_room() function itself
> locks the buffers, but then unlocks it when returning, so when we actually
> do the memcpy() etc, we can race with anybody.
>
> I don't really see who would care, but it does look totally broken.
>
> I dunno, this patch seems to make sense to me. Am I missing something?
>
> [ NOTE! The patch is totally untested. It compiled for me on x86-64, and
> apart from that I'm just going to say that it looks obvious, and the old
> code looks obviously buggy. Also, any remaining users of
>
> tty_prepare_flip_string
> tty_prepare_flip_string_flags
>
> are still fundamentally broken and buggy, while users of
>
> tty_buffer_request_room
>
> are pretty damn odd and suspect (but a lot of them seem to be just
> pointless: they then call tty_insert_flip_string(), which means that the
> tty_buffer_request_room() call was totally redundant ]
>
> Comments? Does this work? Does it make any difference? It seems fairly
> unlikely, but it's the only obvious problem I've seen in the tty buffering
> code so far.
>
> And that code is literally 3 years old, and it seems unlikely that a
> regular _keyboard_ buffer would be able to hit the (rather small) race
> condition. But other serialization may have hidden it, and timing
> differences could certainly have caused it to trigger much more easily.
>
> Linus
>
> ---
> drivers/char/tty_buffer.c | 33 +++++++++++++++++++++++++--------
> 1 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/tty_buffer.c b/drivers/char/tty_buffer.c
> index 3108991..25ab538 100644
> --- a/drivers/char/tty_buffer.c
> +++ b/drivers/char/tty_buffer.c
> @@ -196,13 +196,10 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
> *
> * Locking: Takes tty->buf.lock
> */
> -int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> +static int locked_tty_buffer_request_room(struct tty_struct *tty, size_t size)
> {
> struct tty_buffer *b, *n;
> int left;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&tty->buf.lock, flags);
>
> /* OPTIMISATION: We could keep a per tty "zero" sized buffer to
> remove this conditional if its worth it. This would be invisible
> @@ -225,9 +222,20 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> size = left;
> }
>
> - spin_unlock_irqrestore(&tty->buf.lock, flags);
> return size;
> }
> +
> +int tty_buffer_request_room(struct tty_struct *tty, size_t size)
> +{
> + int retval;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tty->buf.lock, flags);
> + retval = locked_tty_buffer_request_room(tty, size);
> + spin_unlock_irqrestore(&tty->buf.lock, flags);
> + return retval;
> +}
> +
> EXPORT_SYMBOL_GPL(tty_buffer_request_room);
>
> /**
> @@ -239,16 +247,20 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room);
> * Queue a series of bytes to the tty buffering. All the characters
> * passed are marked as without error. Returns the number added.
> *
> - * Locking: Called functions may take tty->buf.lock
> + * Locking: We take tty->buf.lock
> */
>
> int tty_insert_flip_string(struct tty_struct *tty, const unsigned char *chars,
> size_t size)
> {
> int copied = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&tty->buf.lock, flags);
> do {
> - int space = tty_buffer_request_room(tty, size - copied);
> + int space = locked_tty_buffer_request_room(tty, size - copied);
> struct tty_buffer *tb = tty->buf.tail;
> +
> /* If there is no space then tb may be NULL */
> if (unlikely(space == 0))
> break;
> @@ -260,6 +272,7 @@ int tty_insert_flip_string(struct tty_struct *tty, const unsigned char *chars,
> /* There is a small chance that we need to split the data over
> several buffers. If this is the case we must loop */
> } while (unlikely(size> copied));
> + spin_unlock_irqrestore(&tty->buf.lock, flags);
> return copied;
> }
> EXPORT_SYMBOL(tty_insert_flip_string);
> @@ -282,8 +295,11 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
> const unsigned char *chars, const char *flags, size_t size)
> {
> int copied = 0;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&tty->buf.lock, irqflags);
> do {
> - int space = tty_buffer_request_room(tty, size - copied);
> + int space = locked_tty_buffer_request_room(tty, size - copied);
> struct tty_buffer *tb = tty->buf.tail;
> /* If there is no space then tb may be NULL */
> if (unlikely(space == 0))
> @@ -297,6 +313,7 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
> /* There is a small chance that we need to split the data over
> several buffers. If this is the case we must loop */
> } while (unlikely(size> copied));
> + spin_unlock_irqrestore(&tty->buf.lock, irqflags);
> return copied;
> }
> EXPORT_SYMBOL(tty_insert_flip_string_flags);
>
>
I can throw your patch in over here for the heck of it.
If there's somebody who's really hitting this bug
then the results would be better if this is the area that causing
this bug.(from here the only issue I'm seeing is spinning
history commands in the terminal from time to time,
nothing of any unusable keys like others are reporting).
Justin P. Mattock
--
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/
Powered by blists - more mailing lists