linux-kernel - Re: tty: WARNING in n_hdlc_tty

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] [day] [month] [year] [list]

Message-ID: <20151211143404.73e0540e@www.etchedpixels.co.uk>
Date:	Fri, 11 Dec 2015 14:34:04 +0000
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Paul Fulghum <paulkf@...rogate.com>,
	Arnd Bergmann <arnd@...db.de>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: tty: WARNING in n_hdlc_tty_read

On Fri, 11 Dec 2015 14:44:00 +0100
Dmitry Vyukov <dvyukov@...gle.com> wrote:

> Hello,
> 
> I am hitting the following WARNING on commit
> aa53685549a2cfb5f175b0c4a20bc9aa1e5a1b85 (Dec 8):

What surprises me is that the warnings didn't trigger for whoever
tested the code.

It also looks to me like the claim in the commit that

" 1. neither BKL or tty mutex are required for correct operation"

is probably not entirely true (though close).

Consider the case where two reads occur in parallel. In that situation
nothing protects hdlc->rx_free_buf_list.count during the decision whether
to kfree or n_hdlc_buf_put.

The more specific problem you are hitting I think those is the attempt to
make use of set_current_state and add_wait_queue directly.

After we set the task to "uninterruptible sleep" we dequeue a frame which
is fine and correctly locked, but then call copy_to_user which can block.
No can do.

My first thought would be to rip out all the raw wait queue messing about
and replace the lot with something slightly more 21st century. Something
a bit like

	if (file->f_flags & O_NONBLOCK)
		rbuf = n_hdlc_get(...)
	else
		wait_event_interruptible(&tty->read_wait, (rbuf =
			n_hdlc_get(&n_hdlc->rx_buf_list)) != NULL);

	if (rbuf)
		...

	if (signal_pending(current))
		...

ought to get close - but that doesn't fit the list.count race.

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