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: <YsL9Ny3Vx9Q2uvpJ@google.com>
Date:   Mon, 4 Jul 2022 14:46:15 +0000
From:   Wedson Almeida Filho <wedsonaf@...gle.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        arnd@...db.de, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable

On Mon, Jul 04, 2022 at 11:07:54PM +0900, Tetsuo Handa wrote:
> On 2022/07/04 22:57, Wedson Almeida Filho wrote:
> > On Mon, Jul 04, 2022 at 10:48:32PM +0900, Tetsuo Handa wrote:
> >> On 2022/07/04 21:59, Wedson Almeida Filho wrote:
> >>>> @@ -139,6 +139,10 @@ static int misc_open(struct inode *inode, struct file *file)
> >>>>  
> >>>>  	err = 0;
> >>>>  	replace_fops(file, new_fops);
> >>>> +	if (iter->unlocked_open && file->f_op->open) {
> >>>> +		mutex_unlock(&misc_mtx);
> >>>> +		return file->f_op->open(inode, file);
> >>>> +	}
> >>>
> >>> One of the invariants of miscdev is that once misc_deregister() returns,
> >>> no new calls to f_op->open() are made. (Although, of course, you can
> >>> still have open files but that's a whole different problem.)
> >>
> >> The point of this change is that file->f_op after mutex_unlock(&misc_mtx) is
> >>  from new_fops which is guaranteed to hold a ref on "struct file_operations"
> >> via new_fops = fops_get("struct miscdevice"->fops).
> >> That is, a module ref remains valid even after mutex_unlock(&misc_mtx).
> >>
> >> And as with major_names_lock case quoted below, this change assumes that
> >> misc_deregister() is called from module's __exit function, and fops_get()
> >> is preventing the module owning new_fops from calling __exit function.
> > 
> > Your assumption is not sound. misc_deregister() can be (and is)
> > legitimately called from other places, for example, a driver's remove()
> > callback. In fact, when I grep for misc_deregister(), the second
> > instance is such a case.
> 
> OK, the frequency of calling misc_deregister() can be much higher than
> unregister_blkdev(), which means that misc_mtx is more prone to trigger
> hung task warnings. I'm more inclined to avoid sleeping with misc_mtx held.

Tetsuo, I'm sorry if I'm not making myself clear. I'm arguing that your
patch is buggy and therefore should not be accepted.

Here's one example of an issue that your patch would introduce. In
binder init, we have the following error path:

err_init_binder_device_failed:
        hlist_for_each_entry_safe(device, tmp, &binder_devices, hlist) {
                misc_deregister(&device->miscdev);
                hlist_del(&device->hlist);
                kfree(device);
        }

Note that open() for binder touches the `device` pointer. If open() can
be called _after_ misc_deregister(), then there is a race condition
where open() is touching `device` while this error path is freeing it.

Right now it isn't a bug because misc_deregister() guarantees that after
it returns, open() won't be called anymore. Your patch breaks this
guarantee.