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; + spin_lock_irqsave(&hp->port.lock, flags); /* Check and then increment for fast path open. */ if (hp->port.count++ > 0) { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
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@su...
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@su...
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
linux-stable-mirror@lists.linaro.org
-
Jiri Slaby -
Raghavendra Rao Ananta -
rananta＠codeaurora.org