linux-kernel - Re: [PATCH] char: misc: make misc_open() and misc

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: <YsgzHc54onQ1DeFc@kroah.com>
Date:   Fri, 8 Jul 2022 15:37:33 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Oliver Neukum <oneukum@...e.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Wedson Almeida Filho <wedsonaf@...gle.com>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, Dmitry Vyukov <dvyukov@...gle.com>,
        Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable

On Thu, Jul 07, 2022 at 02:06:38PM +0900, Tetsuo Handa wrote:
> Let's summarize current location:
> 
> (1) Greg wants me to fix snapshot_open() not to sleep for too long, instead of
>     making misc_open() killable.
> 
> (2) I found snapshot_open() calls wait_for_device_probe() which might sleep
>     long enough to consider as hung up due to:
> 
>       (a) One of existing probe request got stuck due to unresponsive hardware.
> 
>       (b) New probe requests come in before existing probe requests complete.
> 
> (3) Because of (2), it is difficult to guarantee snapshot_open() not to sleep for
>     too long.
> 
> (4) Because of (3), calling file->f_op->open(inode, file) with misc_mtx held can
>     block mutex_lock(&misc_mtx) too long. This is the phenomenon syzbot is reporting.
> 
> Initial mitigation was to replace mutex_lock(&misc_mtx) with mutex_lock_killable(&misc_mtx)
> so that /dev/raw-gadget users can terminate upon SIGKILL and khungtaskd will not complain
> about misc_mtx.
> 
> Next mitigation was not to call file->f_op->open() with misc_mtx held.
> Wedson worried that this approach breaks modules which call misc_deregister(), but
> I think we can use this approach for modules which do not need to call misc_deregister()
> given that this approach is opt-in basis.
> 
> I also think that we can bring wait_for_device_probe() in snapshot_open() to before
> lock_system_sleep(), for system_transition_mutex will not be required for waiting for
> the image device to appear. If we can accept the "not to call file->f_op->open() with
> misc_mtx held" mitigation, wait_for_device_probe() in snapshot_open() can be called
> without locks.
> 
> Finding universally safe timeout value is beyond what I can do for this report.
> Regarding this report, I think we can lower the risk of regression if we apply
> timeout for atomic_read(&probe_count) == 0 from only snapshot_open().
> Can we make below changes?
> 
> ------------------------------------------------------------
> 
>  drivers/base/dd.c             |   14 ++++++++++++++
>  drivers/char/misc.c           |    4 ++++
>  include/linux/device/driver.h |    1 +
>  include/linux/miscdevice.h    |    1 +
>  kernel/power/user.c           |   31 ++++++++++++++++++-------------
>  5 files changed, 38 insertions(+), 13 deletions(-)

Can you make this a patch series, it's hard to tease out what the
different things are attempting to do here :(

thanks,

greg k-h