From: Jiri Slaby <jslaby@suse.cz>
To: rananta@codeaurora.org, Greg KH <gregkh@linuxfoundation.org>
Cc: andrew@daynix.com, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open
Date: Wed, 20 May 2020 11:38:54 +0200 [thread overview]
Message-ID: <cb5bd2b2-f33a-b541-ed3c-70da14c7252d@suse.cz> (raw)
In-Reply-To: <0ab0b49f19b824ac1c4db4c4937ed388@codeaurora.org>
[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]
On 15. 05. 20, 1:22, rananta@codeaurora.org wrote:
> On 2020-05-13 00:04, Greg KH wrote:
>> On Tue, May 12, 2020 at 02:39:50PM -0700, rananta@codeaurora.org wrote:
>>> On 2020-05-12 01:25, Greg KH wrote:
>>> > On Tue, May 12, 2020 at 09:22:15AM +0200, Jiri Slaby wrote:
>>> > > commit bdb498c20040616e94b05c31a0ceb3e134b7e829
>>> > > Author: Jiri Slaby <jslaby@suse.cz>
>>> > > Date: Tue Aug 7 21:48:04 2012 +0200
>>> > >
>>> > > TTY: hvc_console, add tty install
>>> > >
>>> > > added hvc_install but did not move 'tty->driver_data = NULL;' from
>>> > > hvc_open's fail path to hvc_cleanup.
>>> > >
>>> > > IOW hvc_open now NULLs tty->driver_data even for another task which
>>> > > opened the tty earlier. The same holds for
>>> > > "tty_port_tty_set(&hp->port,
>>> > > NULL);" there. And actually "tty_port_put(&hp->port);" is also
>>> > > incorrect
>>> > > for the 2nd task opening the tty.
>>> > >
...
> These are the traces you get when the issue happens:
> [ 154.212291] hvc_install called for pid: 666
> [ 154.216705] hvc_open called for pid: 666
> [ 154.233657] hvc_open: request_irq failed with rc -22.
> [ 154.238934] hvc_open called for pid: 678
> [ 154.243012] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000000000c4
> # hvc_install isn't called for pid: 678 as the file wasn't closed yet.
Nice. Does the attached help?
I wonder how comes the tty_port_put in hvc_open does not cause a UAF? I
would say hvc_open fails, tty_port_put is called. It decrements the
reference taken in hvc_install. So far so good.
Now, this should happen IMO:
tty_open
-> hvc_open (fails)
-> tty_port_put
-> tty_release
-> tty_release_struct
-> tty_kref_put
-> queue_release_one_tty
SCHEDULED WORKQUEUE
release_one_tty
-> hvc_cleanup
-> tty_port_put (should die terrible death now)
What am I missing?
thanks,
--
js
suse labs
[-- Attachment #2: 0001-hvc_console-fix-open.patch --]
[-- Type: text/x-patch, Size: 2381 bytes --]
From d891cdfcbd3b41eb23ddfc8d9e6cbe038ff8fb72 Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jslaby@suse.cz>
Date: Wed, 20 May 2020 11:29:25 +0200
Subject: [PATCH] hvc_console: fix open
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
drivers/tty/hvc/hvc_console.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 436cc51c92c3..cdcc64ea2554 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -371,15 +371,14 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
* tty fields and return the kref reference.
*/
if (rc) {
- tty_port_tty_set(&hp->port, NULL);
- tty->driver_data = NULL;
- tty_port_put(&hp->port);
printk(KERN_ERR "hvc_open: request_irq failed with rc %d.\n", rc);
- } else
+ } else {
/* We are ready... raise DTR/RTS */
if (C_BAUD(tty))
if (hp->ops->dtr_rts)
hp->ops->dtr_rts(hp, 1);
+ tty_port_set_initialized(&hp->port, true);
+ }
/* Force wakeup of the polling thread */
hvc_kick();
@@ -389,22 +388,12 @@ static int hvc_open(struct tty_struct *tty, struct file * filp)
static void hvc_close(struct tty_struct *tty, struct file * filp)
{
- struct hvc_struct *hp;
+ struct hvc_struct *hp = tty->driver_data;
unsigned long flags;
if (tty_hung_up_p(filp))
return;
- /*
- * No driver_data means that this close was issued after a failed
- * hvc_open by the tty layer's release_dev() function and we can just
- * exit cleanly because the kref reference wasn't made.
- */
- if (!tty->driver_data)
- return;
-
- hp = tty->driver_data;
-
spin_lock_irqsave(&hp->port.lock, flags);
if (--hp->port.count == 0) {
@@ -412,6 +401,9 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
/* We are done with the tty pointer now. */
tty_port_tty_set(&hp->port, NULL);
+ if (!tty_port_initialized(&hp->port))
+ return;
+
if (C_HUPCL(tty))
if (hp->ops->dtr_rts)
hp->ops->dtr_rts(hp, 0);
@@ -428,6 +420,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
* waking periodically to check chars_in_buffer().
*/
tty_wait_until_sent(tty, HVC_CLOSE_WAIT);
+ tty_port_set_initialized(&hp->port, false);
} else {
if (hp->port.count < 0)
printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
--
2.26.2
next prev parent reply other threads:[~2020-05-20 9:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-28 3:26 [PATCH] tty: hvc: Fix data abort due to race in hvc_open Raghavendra Rao Ananta
2020-05-06 9:48 ` Greg KH
2020-05-10 1:30 ` rananta
2020-05-10 6:48 ` Greg KH
2020-05-11 7:23 ` rananta
2020-05-11 7:34 ` rananta
2020-05-11 7:41 ` Greg KH
2020-05-11 7:39 ` Greg KH
2020-05-12 7:22 ` Jiri Slaby
2020-05-12 8:25 ` Greg KH
2020-05-12 21:39 ` rananta
2020-05-13 7:04 ` Greg KH
2020-05-14 23:22 ` rananta
2020-05-15 7:30 ` Greg KH
2020-05-15 19:21 ` rananta
2020-05-20 9:38 ` Jiri Slaby [this message]
2020-05-20 13:49 ` rananta
2020-04-28 12:48 Markus Elfring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cb5bd2b2-f33a-b541-ed3c-70da14c7252d@suse.cz \
--to=jslaby@suse.cz \
--cc=andrew@daynix.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=rananta@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).