linux-kernel - Re: [PATCH] tty: fix data race in tty_buffer

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]

Date:	Tue, 15 Sep 2015 21:38:14 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	Linux kernel mailing list <linux-kernel@...r.kernel.org>,
	Jiri Slaby <jslaby@...e.cz>,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Hans Boehm <hboehm@...gle.com>
Subject: Re: [PATCH] tty: fix data race in tty_buffer_flush

On Tue, Sep 8, 2015 at 8:48 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> tty_buffer_flush frees not acquired buffers.
> As the result, for example, read of b->size in tty_buffer_free
> can return garbage value which will lead to a huge buffer
> hanging in the freelist. This is just the benignest
> manifestation of freeing of a not acquired object.
> If the object is passed to kfree, heap can be corrupted.
>
> Acquire visibility over the buffer before freeing it.
>
> The data race was found with KernelThreadSanitizer (KTSAN).

Dmitry,

Looks good. Thanks for splitting this from the other fix.

Like Greg said, the patch needs revision info to help maintainers, et al
track which is most current/relevant/etc.

Typical format for $subject is something like:

[PATCH v4] tty: fix data race in tty_buffer_flush


Notes re: changes from other patch versions are added below the
git changelog separator (example below).

> Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> ---

v4: Corrected commit log and patch revision notes

v3: Added code comment re: paired smp barrier

v2: Split from 'tty: fix data races on tty_buffer.commit'

>  drivers/tty/tty_buffer.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 5a3fa89..a2a8cd0 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -242,7 +242,10 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld)
>         atomic_inc(&buf->priority);
>
>         mutex_lock(&buf->lock);
> -       while ((next = buf->head->next) != NULL) {
> +       /* paired w/ release in __tty_buffer_request_room; ensures there are
> +        * no pending memory accesses to the freed buffer
> +        */
> +       while ((next = smp_load_acquire(&buf->head->next)) != NULL) {
>                 tty_buffer_free(port, buf->head);
>                 buf->head = next;
>         }
> --
> 2.5.0.457.gab17608
>
--
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/