Messages in this thread | | | Date | Wed, 20 May 2020 06:49:04 -0700 | From | rananta@codeauro ... | Subject | Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open |
| |
On 2020-05-20 02:38, Jiri Slaby wrote: > 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? 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
| |