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

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: <20190424225641.GQ2217@ZenIV.linux.org.uk>
Date:   Wed, 24 Apr 2019 23:56:42 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Mukesh Ojha <mojha@...eaurora.org>
Cc:     "dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        Gaurav Kohli <gkohli@...eaurora.org>,
        Peter Hutterer <peter.hutterer@...-t.net>,
        Martin Kepplinger <martink@...teo.de>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>
Subject: Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a
 global lock

On Wed, Apr 24, 2019 at 07:39:03PM +0530, Mukesh Ojha wrote:

> This was my simple program no multithreading just to understand f_counting
> 
> int main()
> {
>         int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
>         ioctl(fd, UI_SET_EVBIT, EV_KEY);
>         close(fd);
>         return 0;
> }
> 
>            uinput-532   [002] ....    45.312044: SYSC_ioctl: 2   <= f_count

Er...  So how does it manage to hit ioctl(2) before open(2)?  Confused...

> >    <After fdget()
>           uinput-532   [002] ....    45.312055: SYSC_ioctl: 2           
> <After fdput()
>           uinput-532   [004] ....    45.313766: uinput_open: uinput: 1   /*
> This is from the uinput driver uinput_open()*/
> 
>   =>>>>                         /* All the above calls happened for the
> open() in userspace*/
> 
>           uinput-532   [004] ....    45.313783: SYSC_ioctl: 1 /* This print
> is for the trace, i put after fdget */
>           uinput-532   [004] ....    45.313788: uinput_ioctl_handler:
> uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl
> driver */
> 
>           uinput-532   [004] ....    45.313835: SYSC_ioctl: 1 /* This print
> is for the trace, i put after fdput*/
>           uinput-532   [004] ....    45.313843: uinput_release: uinput:  0
> /* And this is from the close()  */
> 
> 
> Should fdget not suppose to increment the f_count here, as it is coming 1 ?
> This f_count to one is done at the open, but i have no idea how this  below
> f_count 2 came before open() for
> this simple program.

If descriptor table is not shared, fdget() will simply return you the reference
from there, without bothering to bump the refcount.  _And_ having it marked
"don't drop refcount" in struct fd.

Rationale: since it's not shared, nobody other than our process can modify
it.  So unless we remove (and drop) the reference from it ourselves (which
we certainly have no business doing in ->ioctl() and do not do anywhere
in drivers/input), it will remain there until we return from syscall.

Nobody is allowed to modify descriptor table other than their own.
And if it's not shared, no other owners can appear while the only
existing one is in the middle of syscall other than clone() (with
CLONE_FILES in flags, at that).

For shared descriptor table fdget() bumps file refcount, since there
the reference in descriptor table itself could be removed and dropped
by another thread.

And fdget() marks whether fput() is needed in fd.flags, so that
fdput() does the right thing.