| Message ID | 20200520064708.24278-1-rananta@codeaurora.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] tty: hvc: Fix data abort due to race in hvc_open | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (7b06fb8795ffea9d12be45a172197c3307955479) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch is tagged for stable |
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". thanks,
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
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; + spin_lock_irqsave(&hp->port.lock, flags); /* Check and then increment for fast path open. */ if (hp->port.count++ > 0) {
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(+) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project