[linux] 01/01: tty: n_hdlc: get rid of racy n_hdlc.tbuf (CVE-2017-2636)
debian-kernel at lists.debian.org
debian-kernel at lists.debian.org
Tue Mar 7 20:05:44 UTC 2017
This is an automated email from the git hooks/post-receive script.
benh pushed a commit to branch wheezy-security
in repository linux.
commit d135a78fe9575bd0ef552940ee5863245d260557
Author: Ben Hutchings <ben at decadent.org.uk>
Date: Tue Mar 7 17:36:55 2017 +0000
tty: n_hdlc: get rid of racy n_hdlc.tbuf (CVE-2017-2636)
...plus another patch it depends on
---
debian/changelog | 2 +
.../tty-n_hdlc-fix-lockdep-false-positive.patch | 96 +++++++
.../all/tty-n_hdlc-get-rid-of-racy-n_hdlc.patch | 309 +++++++++++++++++++++
debian/patches/series | 2 +
4 files changed, 409 insertions(+)
diff --git a/debian/changelog b/debian/changelog
index a08bf62..e5436a9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -84,6 +84,8 @@ linux (3.2.86-1) UNRELEASED; urgency=medium
* irda: Fix locking in hashbin_delete() (CVE-2017-6348)
* sctp: deny peeloff operation on asocs with threads sleeping on it
(CVE-2017-6353)
+ * TTY: n_hdlc, fix lockdep false positive
+ * tty: n_hdlc: get rid of racy n_hdlc.tbuf (CVE-2017-2636)
-- Ben Hutchings <ben at decadent.org.uk> Tue, 07 Mar 2017 01:47:48 +0000
diff --git a/debian/patches/bugfix/all/tty-n_hdlc-fix-lockdep-false-positive.patch b/debian/patches/bugfix/all/tty-n_hdlc-fix-lockdep-false-positive.patch
new file mode 100644
index 0000000..c65ba4f
--- /dev/null
+++ b/debian/patches/bugfix/all/tty-n_hdlc-fix-lockdep-false-positive.patch
@@ -0,0 +1,96 @@
+From: Jiri Slaby <jslaby at suse.cz>
+Date: Thu, 26 Nov 2015 19:28:26 +0100
+Subject: TTY: n_hdlc, fix lockdep false positive
+Origin: https://git.kernel.org/linus/e9b736d88af1a143530565929390cadf036dc799
+
+The class of 4 n_hdls buf locks is the same because a single function
+n_hdlc_buf_list_init is used to init all the locks. But since
+flush_tx_queue takes n_hdlc->tx_buf_list.spinlock and then calls
+n_hdlc_buf_put which takes n_hdlc->tx_free_buf_list.spinlock, lockdep
+emits a warning:
+=============================================
+[ INFO: possible recursive locking detected ]
+4.3.0-25.g91e30a7-default #1 Not tainted
+---------------------------------------------
+a.out/1248 is trying to acquire lock:
+ (&(&list->spinlock)->rlock){......}, at: [<ffffffffa01fd020>] n_hdlc_buf_put+0x20/0x60 [n_hdlc]
+
+but task is already holding lock:
+ (&(&list->spinlock)->rlock){......}, at: [<ffffffffa01fdc07>] n_hdlc_tty_ioctl+0x127/0x1d0 [n_hdlc]
+
+other info that might help us debug this:
+ Possible unsafe locking scenario:
+
+ CPU0
+ ----
+ lock(&(&list->spinlock)->rlock);
+ lock(&(&list->spinlock)->rlock);
+
+ *** DEADLOCK ***
+
+ May be due to missing lock nesting notation
+
+2 locks held by a.out/1248:
+ #0: (&tty->ldisc_sem){++++++}, at: [<ffffffff814c9eb0>] tty_ldisc_ref_wait+0x20/0x50
+ #1: (&(&list->spinlock)->rlock){......}, at: [<ffffffffa01fdc07>] n_hdlc_tty_ioctl+0x127/0x1d0 [n_hdlc]
+...
+Call Trace:
+...
+ [<ffffffff81738fd0>] _raw_spin_lock_irqsave+0x50/0x70
+ [<ffffffffa01fd020>] n_hdlc_buf_put+0x20/0x60 [n_hdlc]
+ [<ffffffffa01fdc24>] n_hdlc_tty_ioctl+0x144/0x1d0 [n_hdlc]
+ [<ffffffff814c25c1>] tty_ioctl+0x3f1/0xe40
+...
+
+Fix it by initializing the spin_locks separately. This removes also
+reduntand memset of a freshly kzallocated space.
+
+Signed-off-by: Jiri Slaby <jslaby at suse.cz>
+Reported-by: Dmitry Vyukov <dvyukov at google.com>
+Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
+---
+ drivers/tty/n_hdlc.c | 19 ++++---------------
+ 1 file changed, 4 insertions(+), 15 deletions(-)
+
+--- a/drivers/tty/n_hdlc.c
++++ b/drivers/tty/n_hdlc.c
+@@ -160,7 +160,6 @@ struct n_hdlc {
+ /*
+ * HDLC buffer list manipulation functions
+ */
+-static void n_hdlc_buf_list_init(struct n_hdlc_buf_list *list);
+ static void n_hdlc_buf_put(struct n_hdlc_buf_list *list,
+ struct n_hdlc_buf *buf);
+ static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *list);
+@@ -856,10 +855,10 @@ static struct n_hdlc *n_hdlc_alloc(void)
+
+ memset(n_hdlc, 0, sizeof(*n_hdlc));
+
+- n_hdlc_buf_list_init(&n_hdlc->rx_free_buf_list);
+- n_hdlc_buf_list_init(&n_hdlc->tx_free_buf_list);
+- n_hdlc_buf_list_init(&n_hdlc->rx_buf_list);
+- n_hdlc_buf_list_init(&n_hdlc->tx_buf_list);
++ spin_lock_init(&n_hdlc->rx_free_buf_list.spinlock);
++ spin_lock_init(&n_hdlc->tx_free_buf_list.spinlock);
++ spin_lock_init(&n_hdlc->rx_buf_list.spinlock);
++ spin_lock_init(&n_hdlc->tx_buf_list.spinlock);
+
+ /* allocate free rx buffer list */
+ for(i=0;i<DEFAULT_RX_BUF_COUNT;i++) {
+@@ -888,16 +887,6 @@ static struct n_hdlc *n_hdlc_alloc(void)
+ } /* end of n_hdlc_alloc() */
+
+ /**
+- * n_hdlc_buf_list_init - initialize specified HDLC buffer list
+- * @list - pointer to buffer list
+- */
+-static void n_hdlc_buf_list_init(struct n_hdlc_buf_list *list)
+-{
+- memset(list, 0, sizeof(*list));
+- spin_lock_init(&list->spinlock);
+-} /* end of n_hdlc_buf_list_init() */
+-
+-/**
+ * n_hdlc_buf_put - add specified HDLC buffer to tail of specified list
+ * @list - pointer to buffer list
+ * @buf - pointer to buffer
diff --git a/debian/patches/bugfix/all/tty-n_hdlc-get-rid-of-racy-n_hdlc.patch b/debian/patches/bugfix/all/tty-n_hdlc-get-rid-of-racy-n_hdlc.patch
new file mode 100644
index 0000000..fad3db3
--- /dev/null
+++ b/debian/patches/bugfix/all/tty-n_hdlc-get-rid-of-racy-n_hdlc.patch
@@ -0,0 +1,309 @@
+From: Alexander Popov <alex.popov at linux.com>
+Date: Tue, 28 Feb 2017 19:54:40 +0300
+Subject: tty: n_hdlc: get rid of racy n_hdlc.tbuf
+Origin: https://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/commit/?h=tty-linus&id=82f2341c94d270421f383641b7cd670e474db56b
+Bug-Debian-Security: https://security-tracker.debian.org/tracker/CVE-2017-2636
+
+Currently N_HDLC line discipline uses a self-made singly linked list for
+data buffers and has n_hdlc.tbuf pointer for buffer retransmitting after
+an error.
+
+The commit be10eb7589337e5defbe214dae038a53dd21add8
+("tty: n_hdlc add buffer flushing") introduced racy access to n_hdlc.tbuf.
+After tx error concurrent flush_tx_queue() and n_hdlc_send_frames() can put
+one data buffer to tx_free_buf_list twice. That causes double free in
+n_hdlc_release().
+
+Let's use standard kernel linked list and get rid of n_hdlc.tbuf:
+in case of tx error put current data buffer after the head of tx_buf_list.
+
+Signed-off-by: Alexander Popov <alex.popov at linux.com>
+Cc: stable <stable at vger.kernel.org>
+Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
+---
+ drivers/tty/n_hdlc.c | 132 +++++++++++++++++++++++++++------------------------
+ 1 file changed, 69 insertions(+), 63 deletions(-)
+
+--- a/drivers/tty/n_hdlc.c
++++ b/drivers/tty/n_hdlc.c
+@@ -115,7 +115,7 @@
+ #define DEFAULT_TX_BUF_COUNT 3
+
+ struct n_hdlc_buf {
+- struct n_hdlc_buf *link;
++ struct list_head list_item;
+ int count;
+ char buf[1];
+ };
+@@ -123,8 +123,7 @@ struct n_hdlc_buf {
+ #define N_HDLC_BUF_SIZE (sizeof(struct n_hdlc_buf) + maxframe)
+
+ struct n_hdlc_buf_list {
+- struct n_hdlc_buf *head;
+- struct n_hdlc_buf *tail;
++ struct list_head list;
+ int count;
+ spinlock_t spinlock;
+ };
+@@ -137,7 +136,6 @@ struct n_hdlc_buf_list {
+ * @backup_tty - TTY to use if tty gets closed
+ * @tbusy - reentrancy flag for tx wakeup code
+ * @woke_up - FIXME: describe this field
+- * @tbuf - currently transmitting tx buffer
+ * @tx_buf_list - list of pending transmit frame buffers
+ * @rx_buf_list - list of received frame buffers
+ * @tx_free_buf_list - list unused transmit frame buffers
+@@ -150,7 +148,6 @@ struct n_hdlc {
+ struct tty_struct *backup_tty;
+ int tbusy;
+ int woke_up;
+- struct n_hdlc_buf *tbuf;
+ struct n_hdlc_buf_list tx_buf_list;
+ struct n_hdlc_buf_list rx_buf_list;
+ struct n_hdlc_buf_list tx_free_buf_list;
+@@ -160,6 +157,8 @@ struct n_hdlc {
+ /*
+ * HDLC buffer list manipulation functions
+ */
++static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
++ struct n_hdlc_buf *buf);
+ static void n_hdlc_buf_put(struct n_hdlc_buf_list *list,
+ struct n_hdlc_buf *buf);
+ static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *list);
+@@ -209,16 +208,9 @@ static void flush_tx_queue(struct tty_st
+ {
+ struct n_hdlc *n_hdlc = tty2n_hdlc(tty);
+ struct n_hdlc_buf *buf;
+- unsigned long flags;
+
+ while ((buf = n_hdlc_buf_get(&n_hdlc->tx_buf_list)))
+ n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, buf);
+- spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock, flags);
+- if (n_hdlc->tbuf) {
+- n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, n_hdlc->tbuf);
+- n_hdlc->tbuf = NULL;
+- }
+- spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
+ }
+
+ static struct tty_ldisc_ops n_hdlc_ldisc = {
+@@ -284,7 +276,6 @@ static void n_hdlc_release(struct n_hdlc
+ } else
+ break;
+ }
+- kfree(n_hdlc->tbuf);
+ kfree(n_hdlc);
+
+ } /* end of n_hdlc_release() */
+@@ -403,13 +394,7 @@ static void n_hdlc_send_frames(struct n_
+ n_hdlc->woke_up = 0;
+ spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock, flags);
+
+- /* get current transmit buffer or get new transmit */
+- /* buffer from list of pending transmit buffers */
+-
+- tbuf = n_hdlc->tbuf;
+- if (!tbuf)
+- tbuf = n_hdlc_buf_get(&n_hdlc->tx_buf_list);
+-
++ tbuf = n_hdlc_buf_get(&n_hdlc->tx_buf_list);
+ while (tbuf) {
+ if (debuglevel >= DEBUG_LEVEL_INFO)
+ printk("%s(%d)sending frame %p, count=%d\n",
+@@ -421,7 +406,7 @@ static void n_hdlc_send_frames(struct n_
+
+ /* rollback was possible and has been done */
+ if (actual == -ERESTARTSYS) {
+- n_hdlc->tbuf = tbuf;
++ n_hdlc_buf_return(&n_hdlc->tx_buf_list, tbuf);
+ break;
+ }
+ /* if transmit error, throw frame away by */
+@@ -436,10 +421,7 @@ static void n_hdlc_send_frames(struct n_
+
+ /* free current transmit buffer */
+ n_hdlc_buf_put(&n_hdlc->tx_free_buf_list, tbuf);
+-
+- /* this tx buffer is done */
+- n_hdlc->tbuf = NULL;
+-
++
+ /* wait up sleeping writers */
+ wake_up_interruptible(&tty->write_wait);
+
+@@ -449,10 +431,12 @@ static void n_hdlc_send_frames(struct n_
+ if (debuglevel >= DEBUG_LEVEL_INFO)
+ printk("%s(%d)frame %p pending\n",
+ __FILE__,__LINE__,tbuf);
+-
+- /* buffer not accepted by driver */
+- /* set this buffer as pending buffer */
+- n_hdlc->tbuf = tbuf;
++
++ /*
++ * the buffer was not accepted by driver,
++ * return it back into tx queue
++ */
++ n_hdlc_buf_return(&n_hdlc->tx_buf_list, tbuf);
+ break;
+ }
+ }
+@@ -750,7 +734,8 @@ static int n_hdlc_tty_ioctl(struct tty_s
+ int error = 0;
+ int count;
+ unsigned long flags;
+-
++ struct n_hdlc_buf *buf = NULL;
++
+ if (debuglevel >= DEBUG_LEVEL_INFO)
+ printk("%s(%d)n_hdlc_tty_ioctl() called %d\n",
+ __FILE__,__LINE__,cmd);
+@@ -764,8 +749,10 @@ static int n_hdlc_tty_ioctl(struct tty_s
+ /* report count of read data available */
+ /* in next available frame (if any) */
+ spin_lock_irqsave(&n_hdlc->rx_buf_list.spinlock,flags);
+- if (n_hdlc->rx_buf_list.head)
+- count = n_hdlc->rx_buf_list.head->count;
++ buf = list_first_entry_or_null(&n_hdlc->rx_buf_list.list,
++ struct n_hdlc_buf, list_item);
++ if (buf)
++ count = buf->count;
+ else
+ count = 0;
+ spin_unlock_irqrestore(&n_hdlc->rx_buf_list.spinlock,flags);
+@@ -777,8 +764,10 @@ static int n_hdlc_tty_ioctl(struct tty_s
+ count = tty_chars_in_buffer(tty);
+ /* add size of next output frame in queue */
+ spin_lock_irqsave(&n_hdlc->tx_buf_list.spinlock,flags);
+- if (n_hdlc->tx_buf_list.head)
+- count += n_hdlc->tx_buf_list.head->count;
++ buf = list_first_entry_or_null(&n_hdlc->tx_buf_list.list,
++ struct n_hdlc_buf, list_item);
++ if (buf)
++ count += buf->count;
+ spin_unlock_irqrestore(&n_hdlc->tx_buf_list.spinlock,flags);
+ error = put_user(count, (int __user *)arg);
+ break;
+@@ -826,14 +815,14 @@ static unsigned int n_hdlc_tty_poll(stru
+ poll_wait(filp, &tty->write_wait, wait);
+
+ /* set bits for operations that won't block */
+- if (n_hdlc->rx_buf_list.head)
++ if (!list_empty(&n_hdlc->rx_buf_list.list))
+ mask |= POLLIN | POLLRDNORM; /* readable */
+ if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
+ mask |= POLLHUP;
+ if (tty_hung_up_p(filp))
+ mask |= POLLHUP;
+ if (!tty_is_writelocked(tty) &&
+- n_hdlc->tx_free_buf_list.head)
++ !list_empty(&n_hdlc->tx_free_buf_list.list))
+ mask |= POLLOUT | POLLWRNORM; /* writable */
+ }
+ return mask;
+@@ -859,7 +848,12 @@ static struct n_hdlc *n_hdlc_alloc(void)
+ spin_lock_init(&n_hdlc->tx_free_buf_list.spinlock);
+ spin_lock_init(&n_hdlc->rx_buf_list.spinlock);
+ spin_lock_init(&n_hdlc->tx_buf_list.spinlock);
+-
++
++ INIT_LIST_HEAD(&n_hdlc->rx_free_buf_list.list);
++ INIT_LIST_HEAD(&n_hdlc->tx_free_buf_list.list);
++ INIT_LIST_HEAD(&n_hdlc->rx_buf_list.list);
++ INIT_LIST_HEAD(&n_hdlc->tx_buf_list.list);
++
+ /* allocate free rx buffer list */
+ for(i=0;i<DEFAULT_RX_BUF_COUNT;i++) {
+ buf = kmalloc(N_HDLC_BUF_SIZE, GFP_KERNEL);
+@@ -887,53 +881,65 @@ static struct n_hdlc *n_hdlc_alloc(void)
+ } /* end of n_hdlc_alloc() */
+
+ /**
++ * n_hdlc_buf_return - put the HDLC buffer after the head of the specified list
++ * @buf_list - pointer to the buffer list
++ * @buf - pointer to the buffer
++ */
++static void n_hdlc_buf_return(struct n_hdlc_buf_list *buf_list,
++ struct n_hdlc_buf *buf)
++{
++ unsigned long flags;
++
++ spin_lock_irqsave(&buf_list->spinlock, flags);
++
++ list_add(&buf->list_item, &buf_list->list);
++ buf_list->count++;
++
++ spin_unlock_irqrestore(&buf_list->spinlock, flags);
++}
++
++/**
+ * n_hdlc_buf_put - add specified HDLC buffer to tail of specified list
+- * @list - pointer to buffer list
++ * @buf_list - pointer to buffer list
+ * @buf - pointer to buffer
+ */
+-static void n_hdlc_buf_put(struct n_hdlc_buf_list *list,
++static void n_hdlc_buf_put(struct n_hdlc_buf_list *buf_list,
+ struct n_hdlc_buf *buf)
+ {
+ unsigned long flags;
+- spin_lock_irqsave(&list->spinlock,flags);
+-
+- buf->link=NULL;
+- if (list->tail)
+- list->tail->link = buf;
+- else
+- list->head = buf;
+- list->tail = buf;
+- (list->count)++;
+-
+- spin_unlock_irqrestore(&list->spinlock,flags);
+-
++
++ spin_lock_irqsave(&buf_list->spinlock, flags);
++
++ list_add_tail(&buf->list_item, &buf_list->list);
++ buf_list->count++;
++
++ spin_unlock_irqrestore(&buf_list->spinlock, flags);
+ } /* end of n_hdlc_buf_put() */
+
+ /**
+ * n_hdlc_buf_get - remove and return an HDLC buffer from list
+- * @list - pointer to HDLC buffer list
++ * @buf_list - pointer to HDLC buffer list
+ *
+ * Remove and return an HDLC buffer from the head of the specified HDLC buffer
+ * list.
+ * Returns a pointer to HDLC buffer if available, otherwise %NULL.
+ */
+-static struct n_hdlc_buf* n_hdlc_buf_get(struct n_hdlc_buf_list *list)
++static struct n_hdlc_buf *n_hdlc_buf_get(struct n_hdlc_buf_list *buf_list)
+ {
+ unsigned long flags;
+ struct n_hdlc_buf *buf;
+- spin_lock_irqsave(&list->spinlock,flags);
+-
+- buf = list->head;
++
++ spin_lock_irqsave(&buf_list->spinlock, flags);
++
++ buf = list_first_entry_or_null(&buf_list->list,
++ struct n_hdlc_buf, list_item);
+ if (buf) {
+- list->head = buf->link;
+- (list->count)--;
++ list_del(&buf->list_item);
++ buf_list->count--;
+ }
+- if (!list->head)
+- list->tail = NULL;
+-
+- spin_unlock_irqrestore(&list->spinlock,flags);
++
++ spin_unlock_irqrestore(&buf_list->spinlock, flags);
+ return buf;
+-
+ } /* end of n_hdlc_buf_get() */
+
+ static char hdlc_banner[] __initdata =
diff --git a/debian/patches/series b/debian/patches/series
index 8ebf251..8530546 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1112,6 +1112,8 @@ bugfix/all/net-llc-avoid-bug_on-in-skb_orphan.patch
bugfix/all/packet-fix-races-in-fanout_add.patch
bugfix/all/irda-fix-lockdep-annotations-in-hashbin_delete.patch
bugfix/all/sctp-deny-peeloff-operation-on-asocs-with-threads-sleeping-on-it.patch
+bugfix/all/tty-n_hdlc-fix-lockdep-false-positive.patch
+bugfix/all/tty-n_hdlc-get-rid-of-racy-n_hdlc.patch
# ABI maintenance
debian/perf-hide-abi-change-in-3.2.30.patch
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/kernel/linux.git
More information about the Kernel-svn-changes
mailing list