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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8bbd1a77-5f88-bb97-db88-6842df2e3e3c@kernel.org>
Date:   Tue, 14 Dec 2021 07:43:10 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>,
        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

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?

> - 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);


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

thanks,
-- 
js
suse labs