On 15. 05. 20, 1:22, rana...@codeaurora.org wrote: > On 2020-05-13 00:04, Greg KH wrote: >> On Tue, May 12, 2020 at 02:39:50PM -0700, rana...@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 <jsl...@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
>From d891cdfcbd3b41eb23ddfc8d9e6cbe038ff8fb72 Mon Sep 17 00:00:00 2001 From: Jiri Slaby <jsl...@suse.cz> Date: Wed, 20 May 2020 11:29:25 +0200 Subject: [PATCH] hvc_console: fix open Signed-off-by: Jiri Slaby <jsl...@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