* [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open
@ 2020-05-20 6:47 Raghavendra Rao Ananta
2020-05-20 8:59 ` Jiri Slaby
0 siblings, 1 reply; 3+ messages in thread
From: Raghavendra Rao Ananta @ 2020-05-20 6:47 UTC (permalink / raw)
To: gregkh, jslaby, andrew
Cc: linuxppc-dev, linux-kernel, Raghavendra Rao Ananta, stable
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open
2020-05-20 6:47 [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open Raghavendra Rao Ananta
@ 2020-05-20 8:59 ` Jiri Slaby
2020-05-20 12:43 ` rananta
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Slaby @ 2020-05-20 8:59 UTC (permalink / raw)
To: Raghavendra Rao Ananta, gregkh, andrew; +Cc: linuxppc-dev, linux-kernel, 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,
--
js
suse labs
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open
2020-05-20 8:59 ` Jiri Slaby
@ 2020-05-20 12:43 ` rananta
0 siblings, 0 replies; 3+ messages in thread
From: rananta @ 2020-05-20 12:43 UTC (permalink / raw)
To: Jiri Slaby; +Cc: gregkh, andrew, linuxppc-dev, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-20 12:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 6:47 [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open Raghavendra Rao Ananta
2020-05-20 8:59 ` Jiri Slaby
2020-05-20 12:43 ` rananta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).