Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open

[rananta]

On 2020-05-20 02:38, Jiri Slaby wrote:
> On 15. 05. 20, 1:22, rananta@xxxxxxxxxxxxxx wrote:
> > On 2020-05-13 00:04, Greg KH wrote:
> > > On Tue, May 12, 2020 at 02:39:50PM -0700, rananta@xxxxxxxxxxxxxx 
> > > 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@xxxxxxx>
> > > > > > 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?
Yeah, it looks good. I think it also eliminates the port.count reference
issue discussed on the v2 patch. Are you planning to mainline this?
> 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
hvc_console driver defines port->ops->destruct(). Upon tty_port_put(), 
the
tty_port_destructor() calls port->ops->destruct(), rather than 
kfree(port).
>   -> 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)
Since port is not free'd, I think we should be good.
> What am I missing?
>
> thanks,

Thank you.
Raghavendra