linux-kernel - Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2008dd33-5d23-961c-ca61-53ff3e34479a@foss.st.com>
Date:   Tue, 14 Dec 2021 16:38:06 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To:     Jiri Slaby <jirislaby@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH] tty: rpmsg: Fix race condition releasing tty port

Hello Jiri,

On 12/14/21 7:43 AM, Jiri Slaby wrote:
> Hi,
> 
> On 13. 12. 21, 20:53, Arnaud Pouliquen wrote:
>> In current implementation the tty_port struct is part of the
>> rpmsg_tty_port structure.The issue is that the rpmsg_tty_port structure is
>> freed on rpmsg_tty_remove but also referenced in the tty_struct.
>> Its release is not predictable due to workqueues.
>>
>> For instance following ftrace shows that rpmsg_tty_close is called after
>> rpmsg_tty_release_cport:
>>
>>       nr_test.sh-389     [000] .....   212.093752: rpmsg_tty_remove <-rpmsg_dev_
>> remove
>>               cat-1191    [001] .....   212.095697: tty_release <-__fput
>>        nr_test.sh-389     [000] .....   212.099166: rpmsg_tty_release_cport <-rpm
>> sg_tty_remove
>>               cat-1191    [001] .....   212.115352: rpmsg_tty_close <-tty_release
>>               cat-1191    [001] .....   212.115371: release_tty <-tty_release_str
>>
>> As consequence, the port must be free only when user has released the TTY
>> interface.
>>
>> This path (inspired from vcc.c):
>> - moves the management of the port in the install and clean-up tty ops,
>> - allocates the tty_port struct independently of the rpmsg_tty_port structure,
> 
> This looks rather wrong. Why not to use tty_port refcounting?

Please could you detail what seems rather wrong for you? Everything or do the
tty_port port struct independently of the rpmsg_tty_port structure?

Concerning the tty_port refcounting:
Yes it also an option that I have already tried without success, before
implementing this patch.
That said, as you pointed it out, I reimplemented it today in another way, and
this time it seems that it works without any runtime warning or error.
I need to perform more test to confirm, then I will propose a V2 based on
tty_port refcountingt and the .destruct tty_port_operations.

> 
>> - uses tty_vhangup and tty_port_hangup.
> 
> OK, but don't store a tty pointer as it looks racy. You should use
> tty_port_tty_get instead.
> 
> Hm, we look we need tty_port_tty_vhangup (aside from tty_port_tty_hangup). There
> are plenty of drivers doing:
>     tty = tty_port_tty_get(port);
>     if (tty) {
>             tty_vhangup(port->tty);
>             tty_kref_put(tty);

I would like to first fix the issue in rpmsg_tty.c in separate thread.
But yes this should not take me too much time to propose this helper next.

Thanks,
Arnaud

> 
> 
>> Fixes: 7c0408d80579 ("tty: add rpmsg driver")
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
> 
> thanks,