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]

Date:   Mon, 4 Jul 2022 09:29:45 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     arnd@...db.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable

On Mon, Jul 04, 2022 at 03:44:07PM +0900, Tetsuo Handa wrote:
> syzbot is reporting hung task at misc_open() [1], for snapshot_open() from
> misc_open() might sleep for long with misc_mtx held whereas userspace can
> flood with concurrent misc_open() requests. Mitigate this problem by making
> misc_open() and misc_register() killable.

I do not understand, why not just fix snapshot_open()?  Why add this
complexity to the misc core for a foolish individual misc device?  Why
not add the fix there where it is spinning instead?

> Link: https://syzkaller.appspot.com/bug?extid=358c9ab4c93da7b7238c [1]
> Reported-by: syzbot <syzbot+358c9ab4c93da7b7238c@...kaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> ---
>  drivers/char/misc.c | 57 +++++++++++++++++++--------------------------
>  1 file changed, 24 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/misc.c b/drivers/char/misc.c
> index cba19bfdc44d..b9a494bc4228 100644
> --- a/drivers/char/misc.c
> +++ b/drivers/char/misc.c
> @@ -100,49 +100,39 @@ static const struct seq_operations misc_seq_ops = {
>  static int misc_open(struct inode *inode, struct file *file)
>  {
>  	int minor = iminor(inode);
> -	struct miscdevice *c = NULL, *iter;
> +	struct miscdevice *iter;
>  	int err = -ENODEV;
>  	const struct file_operations *new_fops = NULL;
> +	bool retried = false;
>  
> -	mutex_lock(&misc_mtx);
> -
> + retry:
> +	if (mutex_lock_killable(&misc_mtx))
> +		return -EINTR;

I really don't want to add this type of thing to the misc core if at all
possible.  Again, please just fix up the misbehaving misc driver
instead, don't penalize the core with thie complexity.

thanks,

greg k-h