Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Al Viro]

On Tue, Apr 23, 2019 at 12:06:12PM +0100, Al Viro wrote:
> On Tue, Apr 23, 2019 at 08:49:44AM +0000, dmitry.torokhov@xxxxxxxxx wrote:
> > On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
> 
> > > I have taken care this case from ioctl and release point of view.
> > > 
> > > Even if the release gets called first it will make the
> > > file->private_data=NULL.
> > > and further call to ioctl will not be a problem as the check is already
> > > there.
> > 
> > Al, do we have any protections in VFS layer from userspace hanging onto
> > a file descriptor and calling ioctl() on it even as another thread
> > calls close() on the same fd?
> > 
> > Should the issue be solved by individual drivers, or more careful
> > accounting for currently running operations is needed at VFS layer?
> 
> Neither.  An overlap of ->release() and ->ioctl() is possible only
> if you've got memory corruption somewhere.
> 
> close() overlapping ioctl() is certainly possible, and won't trigger
> that at all - sys_ioctl() holds onto reference to struct file, so
> its refcount won't reach zero until we are done with it.

I'd like to see a reproducer; _if_ we are really getting such overlaps,
we have something buggering struct file refcounts (e.g. stray fput()
eventually leading to dangling pointer in descriptor table) or an
outright memory corruption mangling descriptor table/kernel stack/
refcount/->private_data/etc.

Details, please - I tried to google some context for that, but hadn't
found anything useful ;-/