Re: [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx
Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxxxxxx>
---
 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@xxxxxxx/

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



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux