linux-kernel - Re: [Bug #14388] keyboard under X with 2.6.31

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: <1255478932.19056.24.camel@localhost.localdomain>
Date:	Tue, 13 Oct 2009 19:08:52 -0500
From:	Paul Fulghum <paulkf@...rogate.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Boyan <btanastasov@...oo.co.uk>,
	"Frédéric "L. W. Meunier\"" 
	<fredlwm@...il.com>, "Justin P. Mattock" <justinmattock@...il.com>,
	Nix <nix@...eri.org.uk>, Alan Cox <alan@...rguk.ukuu.org.uk>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Kernel Testers List <kernel-testers@...r.kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Ed Tomlinson <edt@....ca>,
	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Subject: Re: [Bug #14388] keyboard under X with 2.6.31

On Tue, 2009-10-13 at 15:42 -0700, Linus Torvalds wrote:

> They are added to the tail only if the tail is non-NULL.

True

tail is null only after:
* initialization - no flush_to_ldisc in progress
* tty_flush_buffer - protected from flush_to_ldisc by TTY_FLUSHING

flush_to_ldisc does not currently set tail to null after
flushing data from the last buffer. Each buffer is marked
individually as consumed so no more data will be added to it.
The buffer is marked empty again as it is recycled.

However, looking at this does reveal a problem.

If an individual buffer of 512 bytes or larger gets into the
free and full lists, that buffer is kfreed in tty_buffer_free()
instead of being recycled. That means that tty->buf.tail can
point to a freed block of memory, at least until another buffer
is allocated.

If that buffer is written to while in this state, the fields
will become incoherent and may result in more data being added to it.

I think the patch below fixes this problem. It sets tail to null
when all buffers are flushed. This is only executed after the buffer
has been passed to the ldisc and the spinlock is held so there is
no place for more data to be added incorrectly. I will test it myself
tomorrow when I get back to the office.

> I do object to the whole crazy subtle TTY locking. I'm convinced it's 
> wrong, and I'm convinced it's wrong exactly _because_ it tries to be so 
> subtle and does non-obvious things.

I understand. The patch below should fix the hole above, and I'm not
aware of any other hole. But I you prefer reworking the locking
to make things more obvious, I have no objection.

--- a/drivers/char/tty_buffer.c	2009-09-09 17:13:59.000000000 -0500
+++ b/drivers/char/tty_buffer.c	2009-10-13 18:34:34.000000000 -0500
@@ -423,6 +423,8 @@ static void flush_to_ldisc(struct work_s
 					break;
 				tbuf = head;
 				head = head->next;
+				if (!head)
+					tty->buf.tail = NULL;
 				tty_buffer_free(tty, tbuf);
 				continue;
 			}

--
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/