Messages in this thread | | | Date | Sat, 09 May 2020 18:30:56 -0700 | From | rananta@codeauro ... | Subject | Re: [PATCH] tty: hvc: Fix data abort due to race in hvc_open |
| |
On 2020-05-06 02:48, Greg KH wrote: > On Mon, Apr 27, 2020 at 08:26:01PM -0700, Raghavendra Rao Ananta wrote: >> Potentially, hvc_open() can be called in parallel when two tasks calls >> open() on /dev/hvcX. In such a scenario, if the >> hp->ops->notifier_add() >> callback in the function fails, where it sets the tty->driver_data to >> NULL, the parallel hvc_open() can see this NULL and cause a memory >> abort. >> Hence, serialize hvc_open and check if tty->private_data is NULL >> before >> proceeding ahead. >> >> The issue can be easily reproduced by launching two tasks >> simultaneously >> that does nothing but open() and close() on /dev/hvcX. >> For example: >> $ ./simple_open_close /dev/hvc0 & ./simple_open_close /dev/hvc0 & >> >> Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org> >> --- >> drivers/tty/hvc/hvc_console.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/tty/hvc/hvc_console.c >> b/drivers/tty/hvc/hvc_console.c >> index 436cc51c92c3..ebe26fe5ac09 100644 >> --- a/drivers/tty/hvc/hvc_console.c >> +++ b/drivers/tty/hvc/hvc_console.c >> @@ -75,6 +75,8 @@ static LIST_HEAD(hvc_structs); >> */ >> static DEFINE_MUTEX(hvc_structs_mutex); >> >> +/* Mutex to serialize hvc_open */ >> +static DEFINE_MUTEX(hvc_open_mutex); >> /* >> * This value is used to assign a tty->index value to a hvc_struct >> based >> * upon order of exposure via hvc_probe(), when we can not match it >> to >> @@ -346,16 +348,24 @@ static int hvc_install(struct tty_driver >> *driver, struct tty_struct *tty) >> */ >> static int hvc_open(struct tty_struct *tty, struct file * filp) >> { >> - struct hvc_struct *hp = tty->driver_data; >> + struct hvc_struct *hp; >> unsigned long flags; >> int rc = 0; >> >> + mutex_lock(&hvc_open_mutex); >> + >> + hp = tty->driver_data; >> + if (!hp) { >> + rc = -EIO; >> + goto out; >> + } >> + >> spin_lock_irqsave(&hp->port.lock, flags); >> /* Check and then increment for fast path open. */ >> if (hp->port.count++ > 0) { >> spin_unlock_irqrestore(&hp->port.lock, flags); >> hvc_kick(); >> - return 0; >> + goto out; >> } /* else count == 0 */ >> spin_unlock_irqrestore(&hp->port.lock, flags); > > Wait, why isn't this driver just calling tty_port_open() instead of > trying to open-code all of this? > > Keeping a single mutext for open will not protect it from close, it > will > just slow things down a bit. There should already be a tty lock held > by > the tty core for open() to keep it from racing things, right? The tty lock should have been held, but not likely across ->install() and ->open() callbacks, thus resulting in a race between hvc_install() and hvc_open(), where hvc_install() sets a data and the hvc_open() clears it. hvc_open() doesn't check if the data was set to NULL and proceeds. > > Try just removing all of this logic and replacing it with a call to > tty_port_open() and see if that fixes this issue. > > As "proof" of this, I don't see other serial drivers needing a single > mutex for their open calls, do you? > > thanks, > > greg k-h
Thank you. Raghavendra
| |