Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 20 May 2020 05:43:46 -0700
From:   rananta@codeaurora.org
To:     Jiri Slaby <jslaby@suse.cz>
Cc:     gregkh@linuxfoundation.org, andrew@daynix.com,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Subject: Re: [PATCH v2] tty: hvc: Fix data abort due to race in hvc_open
In-Reply-To: <f84a9da7-bb0f-7538-fa00-968c9625335b@suse.cz>
References: <20200520064708.24278-1-rananta@codeaurora.org>
 <f84a9da7-bb0f-7538-fa00-968c9625335b@suse.cz>
Message-ID: <5895803be5c8fd4c5e7725b57ffe79e4@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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