Messages in this thread | | | Date | Wed, 20 May 2020 05:43:46 -0700 | From | rananta@codeauro ... | Subject | Re: [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open |
| |
On 2020-05-20 01:59, Jiri Slaby wrote: > On 20. 05. 20, 8:47, 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, do a NULL check at the beginning, before proceeding ahead. >> >> The issue can be easily reproduced by launching two tasks >> simultaneously >> that does an open() call on /dev/hvcX. >> For example: >> $ cat /dev/hvc0 & cat /dev/hvc0 & >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Raghavendra Rao Ananta <rananta@codeaurora.org> >> --- >> drivers/tty/hvc/hvc_console.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/tty/hvc/hvc_console.c >> b/drivers/tty/hvc/hvc_console.c >> index 436cc51c92c3..80709f754cc8 100644 >> --- a/drivers/tty/hvc/hvc_console.c >> +++ b/drivers/tty/hvc/hvc_console.c >> @@ -350,6 +350,9 @@ static int hvc_open(struct tty_struct *tty, struct >> file * filp) >> unsigned long flags; >> int rc = 0; >> >> + if (!hp) >> + return -ENODEV; >> + > > This is still not fixing the bug properly. See: > https://lore.kernel.org/linuxppc-dev/0f7791f5-0a53-59f6-7277-247a789f30c2@suse.cz/ > > In particular, the paragraph starting "IOW". > You are right. This doesn't fix the problem entirely. There are other parts to it which is not handled in a clean way by the driver. Apart from the things you've mentioned, it doesn't seem to handle the hp->port.count correctly as well.
hvc_open: hp->port.count++ hp->ops->notifier_add(hp, hp->data) fails tty->driver_data = NULL
hvc_close: returns immediately as tty->driver_data == NULL, without hp->port.count--
This would leave the port in a stale state, and the second caller of hvc_open doesn't get a chance to call/retry hp->ops->notifier_add(hp, hp->data);
However, the patch is not trying to address the logical issues with hvc_open and hvc_close. It's only trying to eliminate the potential NULL pointer dereference, leading to a panic. From what I see, the hvc_open is serialized by tty_lock, and adding a NULL check here is preventing the second caller. > thanks,
Thank you. Raghavendra
| |